On Thu, Aug 08, 2019 at 01:08:59PM -0700, Reinette Chatre wrote:
> With the goal of following these guidelines exactly I came up with the
> below that is an incremental diff on top of what this review started out as.
Thanks but pls in the future, do not use windoze to send a diff - it
mangles it to inapplicability.
> Some changes to highlight that may be of concern:
> * In your previous email you do mention that this will be a "single bit
> of information". Please note that I did not specifically use an actual
> bit to capture this information but an unsigned int (I am very aware
> that you also commented on this initially). If you do mean that this
> should be stored as an actual bit, could you please help me by
> elaborating how you would like to see this implemented?
See below for a possible way to do it.
> * Please note that I moved the initialization to init_intel_cacheinfo()
> to be specific to Intel. I did so because from what I understand there
> are some AMD platforms for which this information cannot be determined
> and I thought it simpler to make it specific to Intel with the new
> single static variable.
Yeah, I renamed your function to cacheinfo_l3_inclusive() in case the
other vendors would want to use it someday.
> * Please note that while this is a single global static variable it will
> be set over and over for each CPU on the system.
That's fine.
Also, the bits in include/linux/cacheinfo.h need to go too. Here's a diff ontop
of your patchset:
---
diff --git a/arch/x86/include/asm/cacheinfo.h b/arch/x86/include/asm/cacheinfo.h
index 86b63c7feab7..87eca716e03d 100644
--- a/arch/x86/include/asm/cacheinfo.h
+++ b/arch/x86/include/asm/cacheinfo.h
@@ -5,4 +5,6 @@
void cacheinfo_amd_init_llc_id(struct cpuinfo_x86 *c, int cpu, u8 node_id);
void cacheinfo_hygon_init_llc_id(struct cpuinfo_x86 *c, int cpu, u8 node_id);
+bool cacheinfo_l3_inclusive(void);
+
#endif /* _ASM_X86_CACHEINFO_H */
diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
index 3b678f46be53..418a6f7392d0 100644
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -188,6 +188,13 @@ struct _cpuid4_info_regs {
static unsigned short num_cache_leaves;
+struct cache_attributes {
+ u64 l3_inclusive : 1,
+ __resv : 63;
+};
+
+static struct cache_attributes cache_attrs;
+
/* AMD doesn't have CPUID4. Emulate it here to report the same
information to the user. This makes some assumptions about the machine:
L2 not shared, no SMT etc. that is currently true on AMD CPUs.
@@ -745,6 +752,14 @@ void init_hygon_cacheinfo(struct cpuinfo_x86 *c)
num_cache_leaves = find_num_cache_leaves(c);
}
+bool cacheinfo_l3_inclusive(void)
+{
+ if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
+ return false;
+
+ return cache_attrs.l3_inclusive;
+}
+
void init_intel_cacheinfo(struct cpuinfo_x86 *c)
{
/* Cache sizes */
@@ -795,6 +810,7 @@ void init_intel_cacheinfo(struct cpuinfo_x86 *c)
num_threads_sharing = 1 +
this_leaf.eax.split.num_threads_sharing;
index_msb =
get_count_order(num_threads_sharing);
l3_id = c->apicid & ~((1 << index_msb) - 1);
+ cache_attrs.l3_inclusive =
this_leaf.edx.split.inclusive;
break;
default:
break;
@@ -1009,13 +1025,6 @@ static void ci_leaf_init(struct cacheinfo *this_leaf,
this_leaf->number_of_sets = base->ecx.split.number_of_sets + 1;
this_leaf->physical_line_partition =
base->ebx.split.physical_line_partition + 1;
- if ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
- boot_cpu_has(X86_FEATURE_TOPOEXT)) ||
- boot_cpu_data.x86_vendor == X86_VENDOR_HYGON ||
- boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) {
- this_leaf->attributes |= CACHE_INCLUSIVE_SET;
- this_leaf->inclusive = base->edx.split.inclusive;
- }
this_leaf->priv = base->nb;
}
diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
index b4fff88572bd..644d1780671e 100644
--- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
+++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
@@ -26,6 +26,7 @@
#include <asm/intel-family.h>
#include <asm/resctrl_sched.h>
#include <asm/perf_event.h>
+#include <asm/cacheinfo.h>
#include "../../events/perf_event.h" /* For X86_CONFIG() */
#include "internal.h"
@@ -125,30 +126,6 @@ static unsigned int get_cache_line_size(unsigned int cpu,
int level)
return 0;
}
-/**
- * get_cache_inclusive - Determine if cache is inclusive of lower levels
- * @cpu: CPU with which cache is associated
- * @level: Cache level
- *
- * Context: @cpu has to be online.
- * Return: 1 if cache is inclusive of lower cache levels, 0 if cache is not
- * inclusive of lower cache levels or on failure.
- */
-static unsigned int get_cache_inclusive(unsigned int cpu, int level)
-{
- struct cpu_cacheinfo *ci;
- int i;
-
- ci = get_cpu_cacheinfo(cpu);
-
- for (i = 0; i < ci->num_leaves; i++) {
- if (ci->info_list[i].level == level)
- return ci->info_list[i].inclusive;
- }
-
- return 0;
-}
-
/**
* pseudo_lock_minor_get - Obtain available minor number
* @minor: Pointer to where new minor number will be stored
@@ -341,8 +318,7 @@ static int pseudo_lock_single_portion_valid(struct
pseudo_lock_region *plr,
goto err_cpu;
}
- if (p->r->cache_level == 3 &&
- !get_cache_inclusive(plr->cpu, p->r->cache_level)) {
+ if (p->r->cache_level == 3 && !cacheinfo_l3_inclusive()) {
rdt_last_cmd_puts("L3 cache not inclusive\n");
goto err_cpu;
}
@@ -448,7 +424,7 @@ static int pseudo_lock_l2_l3_portions_valid(struct
pseudo_lock_region *plr,
goto err_cpu;
}
- if (!get_cache_inclusive(plr->cpu, l3_p->r->cache_level)) {
+ if (!cacheinfo_l3_inclusive()) {
rdt_last_cmd_puts("L3 cache not inclusive\n");
goto err_cpu;
}
diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
index cdc7a9d6923f..46b92cd61d0c 100644
--- a/include/linux/cacheinfo.h
+++ b/include/linux/cacheinfo.h
@@ -33,8 +33,6 @@ extern unsigned int coherency_max_size;
* @physical_line_partition: number of physical cache lines sharing the
* same cachetag
* @size: Total size of the cache
- * @inclusive: Cache is inclusive of lower level caches. Only valid if
- * CACHE_INCLUSIVE_SET attribute is set.
* @shared_cpu_map: logical cpumask representing all the cpus sharing
* this cache node
* @attributes: bitfield representing various cache attributes
@@ -57,7 +55,6 @@ struct cacheinfo {
unsigned int ways_of_associativity;
unsigned int physical_line_partition;
unsigned int size;
- unsigned int inclusive;
cpumask_t shared_cpu_map;
unsigned int attributes;
#define CACHE_WRITE_THROUGH BIT(0)
@@ -69,7 +66,6 @@ struct cacheinfo {
#define CACHE_ALLOCATE_POLICY_MASK \
(CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE)
#define CACHE_ID BIT(4)
-#define CACHE_INCLUSIVE_SET BIT(5)
void *fw_token;
bool disable_sysfs;
void *priv;
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
