[PATCH V2 00/10] x86/CPU and x86/resctrl: Support pseudo-lock regions spanning L2 and L3 cache

[PATCH V2 00/10] x86/CPU and x86/resctrl: Support pseudo-lock regions spanning L2 and L3 cache

[Reinette Chatre]

Changes since V1:
- Rebase onto v5.3-rc2

Dear Maintainers,

Cache pseudo-locking involves preloading a region of physical memory into a
reserved portion of cache that no task or CPU can subsequently fill into and
from that point on will only serve cache hits. At this time it is only
possible to create cache pseudo-locked regions in either L2 or L3 cache,
supporting systems that support either L2 Cache Allocation Technology (CAT)
or L3 CAT because CAT is the mechanism used to manage reservations of cache
portions.

This series introduces support for cache pseudo-locked regions that can span
L2 and L3 cache in preparation for systems that may support CAT on L2 and
L3 cache. Only systems with L3 inclusive cache is supported at this time
because if the L3 cache is not inclusive then pseudo-locked memory within
the L3 cache would be evicted when migrated to L2. Because of this
constraint the first patch in this series introduces support in cacheinfo.c
for resctrl to discover if the L3 cache is inclusive. All other patches in
this series are to the resctrl subsystem.

In support of cache pseudo-locked regions spanning L2 and L3 cache the term
"cache pseudo-lock portion" is introduced. Each portion of a cache
pseudo-locked region spans one level of cache and a cache pseudo-locked
region can be made up of one or two cache pseudo-lock portions.

On systems supporting L2 and L3 CAT where L3 cache is inclusive it is
possible to create two types of pseudo-locked regions:
1) A pseudo-locked region spanning just L3 cache, consisting out of a
single pseudo-locked portion.
2) A pseudo-locked region spanning L2 and L3 cache, consisting out of two
pseudo-locked portions.

In an L3 inclusive cache system a L2 pseudo-locked portion is required to
be matched with an L3 pseudo-locked portion to prevent a cache line from
being evicted from L2 when it is evicted from L3.

Patches 2 to 8 to the resctrl subsystem are preparing for the new feature
and should result in no functional change, but some comments do refer to
the new feature. Support for pseudo-locked regions spanning L2 and L3 cache
is introduced in patches 9 and 10.

Your feedback will be greatly appreciated.

Regards,

Reinette

Reinette Chatre (10):
  x86/CPU: Expose if cache is inclusive of lower level caches
  x86/resctrl: Remove unnecessary size compute
  x86/resctrl: Constrain C-states during pseudo-lock region init
  x86/resctrl: Set cache line size using new utility
  x86/resctrl: Associate pseudo-locked region's cache instance by id
  x86/resctrl: Introduce utility to return pseudo-locked cache portion
  x86/resctrl: Remove unnecessary pointer to pseudo-locked region
  x86/resctrl: Support pseudo-lock regions spanning resources
  x86/resctrl: Pseudo-lock portions of multiple resources
  x86/resctrl: Only pseudo-lock L3 cache when inclusive

 arch/x86/kernel/cpu/cacheinfo.c           |  42 +-
 arch/x86/kernel/cpu/resctrl/core.c        |   7 -
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c |  37 +-
 arch/x86/kernel/cpu/resctrl/internal.h    |  39 +-
 arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 444 +++++++++++++++++++---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c    |  61 ++-
 include/linux/cacheinfo.h                 |   4 +
 7 files changed, 512 insertions(+), 122 deletions(-)

-- 
2.17.2

[PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower level caches

[Reinette Chatre]

Deterministic cache parameters can be learned from CPUID leaf 04H.
Executing CPUID with a particular index in EAX would return the cache
parameters associated with that index in the EAX, EBX, ECX, and EDX
registers.

At this time, when discovering cache parameters for a particular cache
index, only the parameters returned in EAX, EBX, and ECX are parsed.
Parameters returned in EDX are ignored. One of the parameters in EDX,
whether the cache is inclusive of lower level caches, is valuable to
know when determining if a system can support L3 cache pseudo-locking.
If the L3 cache is not inclusive then pseudo-locked data within the L3
cache would be evicted when migrated to L2.

Add support for parsing the cache parameters obtained from EDX and make
the inclusive cache parameter available via the cacheinfo that can be
queried from the cache pseudo-locking code.

Do not expose this information to user space at this time. At this time
this information is required within the kernel only. Also, it is
not obvious what the best formatting of this information should be in
support of the variety of ways users may use this information.

Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 arch/x86/kernel/cpu/cacheinfo.c | 42 +++++++++++++++++++++++++++++----
 include/linux/cacheinfo.h       |  4 ++++
 2 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
index c7503be92f35..3b678f46be53 100644
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -154,10 +154,33 @@ union _cpuid4_leaf_ecx {
 	u32 full;
 };
 
+/*
+ * According to details about CPUID instruction documented in Intel SDM
+ * the third bit of the EDX register is used to indicate if complex
+ * cache indexing is in use.
+ * According to AMD specification (Open Source Register Reference For AMD
+ * Family 17h processors Models 00h-2Fh 56255 Rev 3.03 - July, 2018), only
+ * the first two bits are in use. Since HYGON is based on AMD the
+ * assumption is that it supports the same.
+ *
+ * There is no consumer for the complex indexing information so this bit is
+ * not added to the declaration of what processor can provide in EDX
+ * register. The declaration thus only considers bits supported by all
+ * architectures.
+ */
+union _cpuid4_leaf_edx {
+	struct {
+		unsigned int		wbinvd_no_guarantee:1;
+		unsigned int		inclusive:1;
+	} split;
+	u32 full;
+};
+
 struct _cpuid4_info_regs {
 	union _cpuid4_leaf_eax eax;
 	union _cpuid4_leaf_ebx ebx;
 	union _cpuid4_leaf_ecx ecx;
+	union _cpuid4_leaf_edx edx;
 	unsigned int id;
 	unsigned long size;
 	struct amd_northbridge *nb;
@@ -595,21 +618,24 @@ cpuid4_cache_lookup_regs(int index, struct _cpuid4_info_regs *this_leaf)
 	union _cpuid4_leaf_eax	eax;
 	union _cpuid4_leaf_ebx	ebx;
 	union _cpuid4_leaf_ecx	ecx;
-	unsigned		edx;
+	union _cpuid4_leaf_edx	edx;
+
+	edx.full = 0;
 
 	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
 		if (boot_cpu_has(X86_FEATURE_TOPOEXT))
 			cpuid_count(0x8000001d, index, &eax.full,
-				    &ebx.full, &ecx.full, &edx);
+				    &ebx.full, &ecx.full, &edx.full);
 		else
 			amd_cpuid4(index, &eax, &ebx, &ecx);
 		amd_init_l3_cache(this_leaf, index);
 	} else if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
 		cpuid_count(0x8000001d, index, &eax.full,
-			    &ebx.full, &ecx.full, &edx);
+			    &ebx.full, &ecx.full, &edx.full);
 		amd_init_l3_cache(this_leaf, index);
 	} else {
-		cpuid_count(4, index, &eax.full, &ebx.full, &ecx.full, &edx);
+		cpuid_count(4, index, &eax.full, &ebx.full, &ecx.full,
+			    &edx.full);
 	}
 
 	if (eax.split.type == CTYPE_NULL)
@@ -618,6 +644,7 @@ cpuid4_cache_lookup_regs(int index, struct _cpuid4_info_regs *this_leaf)
 	this_leaf->eax = eax;
 	this_leaf->ebx = ebx;
 	this_leaf->ecx = ecx;
+	this_leaf->edx = edx;
 	this_leaf->size = (ecx.split.number_of_sets          + 1) *
 			  (ebx.split.coherency_line_size     + 1) *
 			  (ebx.split.physical_line_partition + 1) *
@@ -982,6 +1009,13 @@ 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/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
index 46b92cd61d0c..cdc7a9d6923f 100644
--- a/include/linux/cacheinfo.h
+++ b/include/linux/cacheinfo.h
@@ -33,6 +33,8 @@ 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
@@ -55,6 +57,7 @@ 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)
@@ -66,6 +69,7 @@ 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;
-- 
2.17.2

[PATCH V2 02/10] x86/resctrl: Remove unnecessary size compute

[Reinette Chatre]

Information about a cache pseudo-locked region is maintained in its
struct pseudo_lock_region. One of these properties is the size of the
region that is computed before it is created and does not change over
the pseudo-locked region's lifetime.

When displaying the size of the pseudo-locked region to the user it is
thus not necessary to compute the size again from other properties, it
could just be printed directly from its struct.

The code being changed is entered only when the resource group is
pseudo-locked. At this time the pseudo-locked region has already been
created and the size property would thus be valid.

Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index a46dee8e78db..3985097ce728 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1308,10 +1308,8 @@ static int rdtgroup_size_show(struct kernfs_open_file *of,
 		} else {
 			seq_printf(s, "%*s:", max_name_width,
 				   rdtgrp->plr->r->name);
-			size = rdtgroup_cbm_to_size(rdtgrp->plr->r,
-						    rdtgrp->plr->d,
-						    rdtgrp->plr->cbm);
-			seq_printf(s, "%d=%u\n", rdtgrp->plr->d->id, size);
+			seq_printf(s, "%d=%u\n", rdtgrp->plr->d->id,
+				   rdtgrp->plr->size);
 		}
 		goto out;
 	}
-- 
2.17.2

[PATCH V2 03/10] x86/resctrl: Constrain C-states during pseudo-lock region init

[Reinette Chatre]

CPUs associated with a pseudo-locked cache region are prevented
from entering C6 and deeper C-states to ensure that the
power savings associated with those C-states cannot impact
the pseudo-locked region by forcing the pseudo-locked memory to
be evicted.

When supporting pseudo-locked regions that span L2 and L3 cache
levels it is not necessary to prevent all CPUs associated with both
cache levels from entering deeper C-states. Instead, only the
CPUs associated with the L2 cache need to be limited. This would
potentially result in more power savings since another L2 cache
that does not have a pseudo-locked region but share the L3 cache
would be able to enter power savings.

In preparation for limiting the C-states only where required the code to
do so is moved to earlier in the pseudo-lock region initialization.
Moving this code has the consequence that its actions need to be undone
in more error paths. This is accommodated by moving the C-state cleanup
code to the generic cleanup code (pseudo_lock_region_clear()) and
ensuring that the C-state cleanup code can handle the case when C-states
have not yet been constrained.

Also in preparation for limiting the C-states only on required CPUs the
function now accepts a parameter that specifies which CPUs should have
their C-states constrained - at this time the parameter is still used
for all CPUs associated with the pseudo-locked region.

Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 30 ++++++++++++-----------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
index d7623e1b927d..110ae4b4f2e4 100644
--- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
+++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
@@ -175,6 +175,9 @@ static void pseudo_lock_cstates_relax(struct pseudo_lock_region *plr)
 {
 	struct pseudo_lock_pm_req *pm_req, *next;
 
+	if (list_empty(&plr->pm_reqs))
+		return;
+
 	list_for_each_entry_safe(pm_req, next, &plr->pm_reqs, list) {
 		dev_pm_qos_remove_request(&pm_req->req);
 		list_del(&pm_req->list);
@@ -184,6 +187,8 @@ static void pseudo_lock_cstates_relax(struct pseudo_lock_region *plr)
 
 /**
  * pseudo_lock_cstates_constrain - Restrict cores from entering C6
+ * @plr: pseudo-lock region requiring the C-states to be restricted
+ * @cpu_mask: the CPUs that should have their C-states restricted
  *
  * To prevent the cache from being affected by power management entering
  * C6 has to be avoided. This is accomplished by requesting a latency
@@ -197,13 +202,14 @@ static void pseudo_lock_cstates_relax(struct pseudo_lock_region *plr)
  * may be set to map to deeper sleep states. In this case the latency
  * requirement needs to prevent entering C2 also.
  */
-static int pseudo_lock_cstates_constrain(struct pseudo_lock_region *plr)
+static int pseudo_lock_cstates_constrain(struct pseudo_lock_region *plr,
+					 struct cpumask *cpu_mask)
 {
 	struct pseudo_lock_pm_req *pm_req;
 	int cpu;
 	int ret;
 
-	for_each_cpu(cpu, &plr->d->cpu_mask) {
+	for_each_cpu(cpu, cpu_mask) {
 		pm_req = kzalloc(sizeof(*pm_req), GFP_KERNEL);
 		if (!pm_req) {
 			rdt_last_cmd_puts("Failure to allocate memory for PM QoS\n");
@@ -251,6 +257,7 @@ static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
 		plr->d->plr = NULL;
 	plr->d = NULL;
 	plr->cbm = 0;
+	pseudo_lock_cstates_relax(plr);
 	plr->debugfs_dir = NULL;
 }
 
@@ -292,6 +299,10 @@ static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
 
 	plr->size = rdtgroup_cbm_to_size(plr->r, plr->d, plr->cbm);
 
+	ret = pseudo_lock_cstates_constrain(plr, &plr->d->cpu_mask);
+	if (ret < 0)
+		goto out_region;
+
 	for (i = 0; i < ci->num_leaves; i++) {
 		if (ci->info_list[i].level == plr->r->cache_level) {
 			plr->line_size = ci->info_list[i].coherency_line_size;
@@ -1280,12 +1291,6 @@ int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp)
 	if (ret < 0)
 		return ret;
 
-	ret = pseudo_lock_cstates_constrain(plr);
-	if (ret < 0) {
-		ret = -EINVAL;
-		goto out_region;
-	}
-
 	plr->thread_done = 0;
 
 	thread = kthread_create_on_node(pseudo_lock_fn, rdtgrp,
@@ -1294,7 +1299,7 @@ int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp)
 	if (IS_ERR(thread)) {
 		ret = PTR_ERR(thread);
 		rdt_last_cmd_printf("Locking thread returned error %d\n", ret);
-		goto out_cstates;
+		goto out_region;
 	}
 
 	kthread_bind(thread, plr->cpu);
@@ -1312,13 +1317,13 @@ int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp)
 		 * empty pseudo-locking loop.
 		 */
 		rdt_last_cmd_puts("Locking thread interrupted\n");
-		goto out_cstates;
+		goto out_region;
 	}
 
 	ret = pseudo_lock_minor_get(&new_minor);
 	if (ret < 0) {
 		rdt_last_cmd_puts("Unable to obtain a new minor number\n");
-		goto out_cstates;
+		goto out_region;
 	}
 
 	/*
@@ -1375,8 +1380,6 @@ int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp)
 out_debugfs:
 	debugfs_remove_recursive(plr->debugfs_dir);
 	pseudo_lock_minor_release(new_minor);
-out_cstates:
-	pseudo_lock_cstates_relax(plr);
 out_region:
 	pseudo_lock_region_clear(plr);
 out:
@@ -1410,7 +1413,6 @@ void rdtgroup_pseudo_lock_remove(struct rdtgroup *rdtgrp)
 		goto free;
 	}
 
-	pseudo_lock_cstates_relax(plr);
 	debugfs_remove_recursive(rdtgrp->plr->debugfs_dir);
 	device_destroy(pseudo_lock_class, MKDEV(pseudo_lock_major, plr->minor));
 	pseudo_lock_minor_release(plr->minor);
-- 
2.17.2

[PATCH V2 04/10] x86/resctrl: Set cache line size using new utility

[Reinette Chatre]

In preparation for support of pseudo-locked regions spanning two
cache levels the cache line size computation is moved to a utility.

Setting of the cache line size is moved a few lines earlier, before
the C-states are constrained, to reduce the amount of cleanup needed
on failure.

Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 42 +++++++++++++++++------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
index 110ae4b4f2e4..884976913326 100644
--- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
+++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
@@ -101,6 +101,30 @@ static u64 get_prefetch_disable_bits(void)
 	return 0;
 }
 
+/**
+ * get_cache_line_size - Determine the cache coherency line size
+ * @cpu: CPU with which cache is associated
+ * @level: Cache level
+ *
+ * Context: @cpu has to be online.
+ * Return: The cache coherency line size for cache level @level associated
+ * with CPU @cpu. Zero on failure.
+ */
+static unsigned int get_cache_line_size(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].coherency_line_size;
+	}
+
+	return 0;
+}
+
 /**
  * pseudo_lock_minor_get - Obtain available minor number
  * @minor: Pointer to where new minor number will be stored
@@ -281,9 +305,7 @@ static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
  */
 static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
 {
-	struct cpu_cacheinfo *ci;
 	int ret;
-	int i;
 
 	/* Pick the first cpu we find that is associated with the cache. */
 	plr->cpu = cpumask_first(&plr->d->cpu_mask);
@@ -295,7 +317,12 @@ static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
 		goto out_region;
 	}
 
-	ci = get_cpu_cacheinfo(plr->cpu);
+	plr->line_size = get_cache_line_size(plr->cpu, plr->r->cache_level);
+	if (plr->line_size == 0) {
+		rdt_last_cmd_puts("Unable to determine cache line size\n");
+		ret = -1;
+		goto out_region;
+	}
 
 	plr->size = rdtgroup_cbm_to_size(plr->r, plr->d, plr->cbm);
 
@@ -303,15 +330,8 @@ static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
 	if (ret < 0)
 		goto out_region;
 
-	for (i = 0; i < ci->num_leaves; i++) {
-		if (ci->info_list[i].level == plr->r->cache_level) {
-			plr->line_size = ci->info_list[i].coherency_line_size;
-			return 0;
-		}
-	}
+	return 0;
 
-	ret = -1;
-	rdt_last_cmd_puts("Unable to determine cache line size\n");
 out_region:
 	pseudo_lock_region_clear(plr);
 	return ret;
-- 
2.17.2

[PATCH V2 05/10] x86/resctrl: Associate pseudo-locked region's cache instance by id

[Reinette Chatre]

The properties of a cache pseudo-locked region that are maintained in
its struct pseudo_lock_region include a pointer to the cache domain to
which it belongs. A cache domain is a structure that is associated with
a cache instance and when all CPUs associated with the cache instance go
offline the cache domain associated with it is removed. When a cache
domain is removed care is taken to not point to it anymore from the
pseudo-locked region and all possible references to this removed
information are ensured to be safe, often resulting in an error message
to the user.

Replace the cache domain pointer in the properties of the cache
pseudo-locked region with the actual cache ID to eliminate the special
care that needs to be taken when using this data while also making it
possible to keep displaying cache pseudo-locked region information to
the user even when the cache domain structure has been removed.
Associating the cache ID with the pseudo-locked region will also
simplify the restoration of the pseudo-locked region when the
cache domain is restored when the CPUs come back online.

Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 arch/x86/kernel/cpu/resctrl/core.c        |  7 -----
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 14 ++-------
 arch/x86/kernel/cpu/resctrl/internal.h    |  4 +--
 arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 38 +++++++++++++++++------
 arch/x86/kernel/cpu/resctrl/rdtgroup.c    | 21 +++++--------
 5 files changed, 40 insertions(+), 44 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 03eb90d00af0..e043074a88cc 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -633,13 +633,6 @@ static void domain_remove_cpu(int cpu, struct rdt_resource *r)
 			cancel_delayed_work(&d->cqm_limbo);
 		}
 
-		/*
-		 * rdt_domain "d" is going to be freed below, so clear
-		 * its pointer from pseudo_lock_region struct.
-		 */
-		if (d->plr)
-			d->plr->d = NULL;
-
 		kfree(d->ctrl_val);
 		kfree(d->mbps_val);
 		bitmap_free(d->rmid_busy_llc);
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index efbd54cc4e69..072f584cb238 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -291,7 +291,7 @@ static int parse_line(char *line, struct rdt_resource *r,
 				 * region and return.
 				 */
 				rdtgrp->plr->r = r;
-				rdtgrp->plr->d = d;
+				rdtgrp->plr->d_id = d->id;
 				rdtgrp->plr->cbm = d->new_ctrl;
 				d->plr = rdtgrp->plr;
 				return 0;
@@ -471,16 +471,8 @@ int rdtgroup_schemata_show(struct kernfs_open_file *of,
 			for_each_alloc_enabled_rdt_resource(r)
 				seq_printf(s, "%s:uninitialized\n", r->name);
 		} else if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED) {
-			if (!rdtgrp->plr->d) {
-				rdt_last_cmd_clear();
-				rdt_last_cmd_puts("Cache domain offline\n");
-				ret = -ENODEV;
-			} else {
-				seq_printf(s, "%s:%d=%x\n",
-					   rdtgrp->plr->r->name,
-					   rdtgrp->plr->d->id,
-					   rdtgrp->plr->cbm);
-			}
+			seq_printf(s, "%s:%d=%x\n", rdtgrp->plr->r->name,
+				   rdtgrp->plr->d_id, rdtgrp->plr->cbm);
 		} else {
 			closid = rdtgrp->closid;
 			for_each_alloc_enabled_rdt_resource(r) {
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index e49b77283924..65f558a2e806 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -149,7 +149,7 @@ struct mongroup {
  * struct pseudo_lock_region - pseudo-lock region information
  * @r:			RDT resource to which this pseudo-locked region
  *			belongs
- * @d:			RDT domain to which this pseudo-locked region
+ * @d_id:		ID of cache instance to which this pseudo-locked region
  *			belongs
  * @cbm:		bitmask of the pseudo-locked region
  * @lock_thread_wq:	waitqueue used to wait on the pseudo-locking thread
@@ -169,7 +169,7 @@ struct mongroup {
  */
 struct pseudo_lock_region {
 	struct rdt_resource	*r;
-	struct rdt_domain	*d;
+	int			d_id;
 	u32			cbm;
 	wait_queue_head_t	lock_thread_wq;
 	int			thread_done;
diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
index 884976913326..e7d1fdd76161 100644
--- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
+++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
@@ -272,14 +272,19 @@ static int pseudo_lock_cstates_constrain(struct pseudo_lock_region *plr,
  */
 static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
 {
+	struct rdt_domain *d;
+
 	plr->size = 0;
 	plr->line_size = 0;
 	kfree(plr->kmem);
 	plr->kmem = NULL;
+	if (plr->r && plr->d_id >= 0) {
+		d = rdt_find_domain(plr->r, plr->d_id, NULL);
+		if (!IS_ERR_OR_NULL(d))
+			d->plr = NULL;
+	}
 	plr->r = NULL;
-	if (plr->d)
-		plr->d->plr = NULL;
-	plr->d = NULL;
+	plr->d_id = -1;
 	plr->cbm = 0;
 	pseudo_lock_cstates_relax(plr);
 	plr->debugfs_dir = NULL;
@@ -305,10 +310,18 @@ static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
  */
 static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
 {
+	struct rdt_domain *d;
 	int ret;
 
 	/* Pick the first cpu we find that is associated with the cache. */
-	plr->cpu = cpumask_first(&plr->d->cpu_mask);
+	d = rdt_find_domain(plr->r, plr->d_id, NULL);
+	if (IS_ERR_OR_NULL(d)) {
+		rdt_last_cmd_puts("Cache domain offline\n");
+		ret = -ENODEV;
+		goto out_region;
+	}
+
+	plr->cpu = cpumask_first(&d->cpu_mask);
 
 	if (!cpu_online(plr->cpu)) {
 		rdt_last_cmd_printf("CPU %u associated with cache not online\n",
@@ -324,9 +337,9 @@ static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
 		goto out_region;
 	}
 
-	plr->size = rdtgroup_cbm_to_size(plr->r, plr->d, plr->cbm);
+	plr->size = rdtgroup_cbm_to_size(plr->r, d, plr->cbm);
 
-	ret = pseudo_lock_cstates_constrain(plr, &plr->d->cpu_mask);
+	ret = pseudo_lock_cstates_constrain(plr, &d->cpu_mask);
 	if (ret < 0)
 		goto out_region;
 
@@ -358,6 +371,7 @@ static int pseudo_lock_init(struct rdtgroup *rdtgrp)
 
 	init_waitqueue_head(&plr->lock_thread_wq);
 	INIT_LIST_HEAD(&plr->pm_reqs);
+	plr->d_id = -1;
 	rdtgrp->plr = plr;
 	return 0;
 }
@@ -1183,6 +1197,7 @@ static int pseudo_lock_measure_cycles(struct rdtgroup *rdtgrp, int sel)
 {
 	struct pseudo_lock_region *plr = rdtgrp->plr;
 	struct task_struct *thread;
+	struct rdt_domain *d;
 	unsigned int cpu;
 	int ret = -1;
 
@@ -1194,13 +1209,14 @@ static int pseudo_lock_measure_cycles(struct rdtgroup *rdtgrp, int sel)
 		goto out;
 	}
 
-	if (!plr->d) {
+	d = rdt_find_domain(plr->r, plr->d_id, NULL);
+	if (IS_ERR_OR_NULL(d)) {
 		ret = -ENODEV;
 		goto out;
 	}
 
 	plr->thread_done = 0;
-	cpu = cpumask_first(&plr->d->cpu_mask);
+	cpu = cpumask_first(&d->cpu_mask);
 	if (!cpu_online(cpu)) {
 		ret = -ENODEV;
 		goto out;
@@ -1497,6 +1513,7 @@ static int pseudo_lock_dev_mmap(struct file *filp, struct vm_area_struct *vma)
 	struct pseudo_lock_region *plr;
 	struct rdtgroup *rdtgrp;
 	unsigned long physical;
+	struct rdt_domain *d;
 	unsigned long psize;
 
 	mutex_lock(&rdtgroup_mutex);
@@ -1510,7 +1527,8 @@ static int pseudo_lock_dev_mmap(struct file *filp, struct vm_area_struct *vma)
 
 	plr = rdtgrp->plr;
 
-	if (!plr->d) {
+	d = rdt_find_domain(plr->r, plr->d_id, NULL);
+	if (IS_ERR_OR_NULL(d)) {
 		mutex_unlock(&rdtgroup_mutex);
 		return -ENODEV;
 	}
@@ -1521,7 +1539,7 @@ static int pseudo_lock_dev_mmap(struct file *filp, struct vm_area_struct *vma)
 	 * may be scheduled elsewhere and invalidate entries in the
 	 * pseudo-locked region.
 	 */
-	if (!cpumask_subset(current->cpus_ptr, &plr->d->cpu_mask)) {
+	if (!cpumask_subset(current->cpus_ptr, &d->cpu_mask)) {
 		mutex_unlock(&rdtgroup_mutex);
 		return -EINVAL;
 	}
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 3985097ce728..c4bf6ed8b031 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -262,22 +262,23 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of,
 			      struct seq_file *s, void *v)
 {
 	struct rdtgroup *rdtgrp;
-	struct cpumask *mask;
+	struct rdt_domain *d;
 	int ret = 0;
 
 	rdtgrp = rdtgroup_kn_lock_live(of->kn);
 
 	if (rdtgrp) {
 		if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED) {
-			if (!rdtgrp->plr->d) {
+			d = rdt_find_domain(rdtgrp->plr->r, rdtgrp->plr->d_id,
+					    NULL);
+			if (IS_ERR_OR_NULL(d)) {
 				rdt_last_cmd_clear();
 				rdt_last_cmd_puts("Cache domain offline\n");
 				ret = -ENODEV;
 			} else {
-				mask = &rdtgrp->plr->d->cpu_mask;
 				seq_printf(s, is_cpu_list(of) ?
 					   "%*pbl\n" : "%*pb\n",
-					   cpumask_pr_args(mask));
+					   cpumask_pr_args(&d->cpu_mask));
 			}
 		} else {
 			seq_printf(s, is_cpu_list(of) ? "%*pbl\n" : "%*pb\n",
@@ -1301,16 +1302,8 @@ static int rdtgroup_size_show(struct kernfs_open_file *of,
 	}
 
 	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED) {
-		if (!rdtgrp->plr->d) {
-			rdt_last_cmd_clear();
-			rdt_last_cmd_puts("Cache domain offline\n");
-			ret = -ENODEV;
-		} else {
-			seq_printf(s, "%*s:", max_name_width,
-				   rdtgrp->plr->r->name);
-			seq_printf(s, "%d=%u\n", rdtgrp->plr->d->id,
-				   rdtgrp->plr->size);
-		}
+		seq_printf(s, "%*s:", max_name_width, rdtgrp->plr->r->name);
+		seq_printf(s, "%d=%u\n", rdtgrp->plr->d_id, rdtgrp->plr->size);
 		goto out;
 	}
 
-- 
2.17.2

[PATCH V2 06/10] x86/resctrl: Introduce utility to return pseudo-locked cache portion

[Reinette Chatre]

To prevent eviction of pseudo-locked memory it is required that no
other resource group uses any portion of a cache that is in use by
a cache pseudo-locked region.

Introduce a utility that will return a Capacity BitMask (CBM) indicating
all portions of a provided cache instance being used for cache
pseudo-locking. This CBM can be used in overlap checking as well as
cache usage reporting.

Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 arch/x86/kernel/cpu/resctrl/internal.h    |  1 +
 arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 65f558a2e806..f17633cf4776 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -568,6 +568,7 @@ int rdtgroup_tasks_assigned(struct rdtgroup *r);
 int rdtgroup_locksetup_enter(struct rdtgroup *rdtgrp);
 int rdtgroup_locksetup_exit(struct rdtgroup *rdtgrp);
 bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_domain *d, unsigned long cbm);
+u32 rdtgroup_pseudo_locked_bits(struct rdt_resource *r, struct rdt_domain *d);
 bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d);
 int rdt_pseudo_lock_init(void);
 void rdt_pseudo_lock_release(void);
diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
index e7d1fdd76161..f16702a076a3 100644
--- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
+++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
@@ -1626,3 +1626,26 @@ void rdt_pseudo_lock_release(void)
 	unregister_chrdev(pseudo_lock_major, "pseudo_lock");
 	pseudo_lock_major = 0;
 }
+
+/**
+ * rdt_pseudo_locked_bits - Portions of cache instance used for pseudo-locking
+ * @r:		RDT resource to which cache instance belongs
+ * @d:		Cache instance
+ *
+ * Return: bits in CBM of @d that are used for cache pseudo-locking
+ */
+u32 rdtgroup_pseudo_locked_bits(struct rdt_resource *r, struct rdt_domain *d)
+{
+	struct rdtgroup *rdtgrp;
+	u32 pseudo_locked = 0;
+
+	list_for_each_entry(rdtgrp, &rdt_all_groups, rdtgroup_list) {
+		if (!rdtgrp->plr)
+			continue;
+		if (rdtgrp->plr->r && rdtgrp->plr->r->rid == r->rid &&
+		    rdtgrp->plr->d_id == d->id)
+			pseudo_locked |= rdtgrp->plr->cbm;
+	}
+
+	return pseudo_locked;
+}
-- 
2.17.2

[PATCH V2 07/10] x86/resctrl: Remove unnecessary pointer to pseudo-locked region

[Reinette Chatre]

Each cache domain (struct rdt_domain) contains a pointer to a
pseudo-locked region that (if set) is associated with it. At the
same time each resource group (struct rdtgroup) also contains a
pointer to a pseudo-locked region that (if set) is associated with
it.

If a pointer from a cache domain to its pseudo-locked region is
maintained then multiple cache domains could point to a single
pseudo-locked region when a pseudo-locked region spans multiple
resources. Such an arrangement would make it harder to support the
current mechanism of iterating over cache domains in order to find all
pseudo-locked regions.

In preparation for pseudo-locked regions that could span multiple
resources the pointer from a cache domain to a pseudo-locked region is
removed. The pointer to a pseudo-locked region from a resource
group remains - when needing to process all pseudo-locked regions on the
system an iteration over all resource groups is used instead of an
iteration over all cache domains.

Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c |  3 +-
 arch/x86/kernel/cpu/resctrl/internal.h    |  6 +--
 arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 46 +++++++++++------------
 arch/x86/kernel/cpu/resctrl/rdtgroup.c    |  8 ++--
 4 files changed, 32 insertions(+), 31 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index 072f584cb238..a0383ff80afe 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -217,7 +217,7 @@ int parse_cbm(struct rdt_parse_data *data, struct rdt_resource *r,
 
 	if ((rdtgrp->mode == RDT_MODE_EXCLUSIVE ||
 	     rdtgrp->mode == RDT_MODE_SHAREABLE) &&
-	    rdtgroup_cbm_overlaps_pseudo_locked(d, cbm_val)) {
+	    rdtgroup_cbm_overlaps_pseudo_locked(r, d, cbm_val)) {
 		rdt_last_cmd_puts("CBM overlaps with pseudo-locked region\n");
 		return -EINVAL;
 	}
@@ -293,7 +293,6 @@ static int parse_line(char *line, struct rdt_resource *r,
 				rdtgrp->plr->r = r;
 				rdtgrp->plr->d_id = d->id;
 				rdtgrp->plr->cbm = d->new_ctrl;
-				d->plr = rdtgrp->plr;
 				return 0;
 			}
 			goto next;
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index f17633cf4776..892f38899dda 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -309,7 +309,6 @@ struct mbm_state {
  * @mbps_val:	When mba_sc is enabled, this holds the bandwidth in MBps
  * @new_ctrl:	new ctrl value to be loaded
  * @have_new_ctrl: did user provide new_ctrl for this domain
- * @plr:	pseudo-locked region (if any) associated with domain
  */
 struct rdt_domain {
 	struct list_head		list;
@@ -326,7 +325,6 @@ struct rdt_domain {
 	u32				*mbps_val;
 	u32				new_ctrl;
 	bool				have_new_ctrl;
-	struct pseudo_lock_region	*plr;
 };
 
 /**
@@ -567,7 +565,9 @@ enum rdtgrp_mode rdtgroup_mode_by_closid(int closid);
 int rdtgroup_tasks_assigned(struct rdtgroup *r);
 int rdtgroup_locksetup_enter(struct rdtgroup *rdtgrp);
 int rdtgroup_locksetup_exit(struct rdtgroup *rdtgrp);
-bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_domain *d, unsigned long cbm);
+bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_resource *r,
+					 struct rdt_domain *d,
+					 unsigned long cbm);
 u32 rdtgroup_pseudo_locked_bits(struct rdt_resource *r, struct rdt_domain *d);
 bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d);
 int rdt_pseudo_lock_init(void);
diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
index f16702a076a3..733cb7f34948 100644
--- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
+++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
@@ -272,17 +272,10 @@ static int pseudo_lock_cstates_constrain(struct pseudo_lock_region *plr,
  */
 static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
 {
-	struct rdt_domain *d;
-
 	plr->size = 0;
 	plr->line_size = 0;
 	kfree(plr->kmem);
 	plr->kmem = NULL;
-	if (plr->r && plr->d_id >= 0) {
-		d = rdt_find_domain(plr->r, plr->d_id, NULL);
-		if (!IS_ERR_OR_NULL(d))
-			d->plr = NULL;
-	}
 	plr->r = NULL;
 	plr->d_id = -1;
 	plr->cbm = 0;
@@ -822,6 +815,7 @@ int rdtgroup_locksetup_exit(struct rdtgroup *rdtgrp)
 
 /**
  * rdtgroup_cbm_overlaps_pseudo_locked - Test if CBM or portion is pseudo-locked
+ * @r: RDT resource to which @d belongs
  * @d: RDT domain
  * @cbm: CBM to test
  *
@@ -835,17 +829,17 @@ int rdtgroup_locksetup_exit(struct rdtgroup *rdtgrp)
  * Return: true if @cbm overlaps with pseudo-locked region on @d, false
  * otherwise.
  */
-bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_domain *d, unsigned long cbm)
+bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_resource *r,
+					 struct rdt_domain *d,
+					 unsigned long cbm)
 {
+	unsigned long pseudo_locked;
 	unsigned int cbm_len;
-	unsigned long cbm_b;
 
-	if (d->plr) {
-		cbm_len = d->plr->r->cache.cbm_len;
-		cbm_b = d->plr->cbm;
-		if (bitmap_intersects(&cbm, &cbm_b, cbm_len))
-			return true;
-	}
+	pseudo_locked = rdtgroup_pseudo_locked_bits(r, d);
+	cbm_len = r->cache.cbm_len;
+	if (bitmap_intersects(&cbm, &pseudo_locked, cbm_len))
+		return true;
 	return false;
 }
 
@@ -859,13 +853,13 @@ bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_domain *d, unsigned long cbm
  * attempts to create new pseudo-locked regions in the same hierarchy.
  *
  * Return: true if a pseudo-locked region exists in the hierarchy of @d or
- *         if it is not possible to test due to memory allocation issue,
- *         false otherwise.
+ *         if it is not possible to test due to memory allocation or other
+ *         failure, false otherwise.
  */
 bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d)
 {
 	cpumask_var_t cpu_with_psl;
-	struct rdt_resource *r;
+	struct rdtgroup *rdtgrp;
 	struct rdt_domain *d_i;
 	bool ret = false;
 
@@ -876,11 +870,16 @@ bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d)
 	 * First determine which cpus have pseudo-locked regions
 	 * associated with them.
 	 */
-	for_each_alloc_enabled_rdt_resource(r) {
-		list_for_each_entry(d_i, &r->domains, list) {
-			if (d_i->plr)
-				cpumask_or(cpu_with_psl, cpu_with_psl,
-					   &d_i->cpu_mask);
+	list_for_each_entry(rdtgrp, &rdt_all_groups, rdtgroup_list) {
+		if (rdtgrp->plr && rdtgrp->plr->d_id >= 0) {
+			d_i = rdt_find_domain(rdtgrp->plr->r, rdtgrp->plr->d_id,
+					      NULL);
+			if (IS_ERR_OR_NULL(d_i)) {
+				ret = true;
+				goto out;
+			}
+			cpumask_or(cpu_with_psl, cpu_with_psl,
+				   &d_i->cpu_mask);
 		}
 	}
 
@@ -891,6 +890,7 @@ bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d)
 	if (cpumask_intersects(&d->cpu_mask, cpu_with_psl))
 		ret = true;
 
+out:
 	free_cpumask_var(cpu_with_psl);
 	return ret;
 }
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index c4bf6ed8b031..0c1786f09963 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -845,8 +845,10 @@ static int rdt_bit_usage_show(struct kernfs_open_file *of,
 				break;
 			}
 		}
+
+		pseudo_locked = rdtgroup_pseudo_locked_bits(r, dom);
+
 		for (i = r->cache.cbm_len - 1; i >= 0; i--) {
-			pseudo_locked = dom->plr ? dom->plr->cbm : 0;
 			hwb = test_bit(i, &hw_shareable);
 			swb = test_bit(i, &sw_shareable);
 			excl = test_bit(i, &exclusive);
@@ -2541,8 +2543,8 @@ static int __init_one_rdt_domain(struct rdt_domain *d, struct rdt_resource *r,
 				d->new_ctrl |= *ctrl | peer_ctl;
 		}
 	}
-	if (d->plr && d->plr->cbm > 0)
-		used_b |= d->plr->cbm;
+
+	used_b |= rdtgroup_pseudo_locked_bits(r, d);
 	unused_b = used_b ^ (BIT_MASK(r->cache.cbm_len) - 1);
 	unused_b &= BIT_MASK(r->cache.cbm_len) - 1;
 	d->new_ctrl |= unused_b;
-- 
2.17.2

[PATCH V2 08/10] x86/resctrl: Support pseudo-lock regions spanning resources

[Reinette Chatre]

Currently cache pseudo-locked regions only consider one cache level but
cache pseudo-locked regions may span multiple cache levels.

In preparation for support of pseudo-locked regions spanning multiple
cache levels pseudo-lock 'portions' are introduced. A 'portion' of a
pseudo-locked region is the portion of a pseudo-locked region that
belongs to a specific resource. Each pseudo-locked portion is identified
with the resource (for example, L2 or L3 cache), the domain (the
specific cache instance), and the capacity bitmask that specifies which
region of the cache is used by the pseudo-locked region.

In support of pseudo-locked regions spanning multiple cache levels a
pseudo-locked region could have multiple 'portions' but in this
introduction only single portions are allowed.

Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c |  26 +++-
 arch/x86/kernel/cpu/resctrl/internal.h    |  32 ++--
 arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 180 ++++++++++++++++------
 arch/x86/kernel/cpu/resctrl/rdtgroup.c    |  44 ++++--
 4 files changed, 211 insertions(+), 71 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index a0383ff80afe..a60fb38a4d20 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -207,7 +207,7 @@ int parse_cbm(struct rdt_parse_data *data, struct rdt_resource *r,
 	 * hierarchy.
 	 */
 	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP &&
-	    rdtgroup_pseudo_locked_in_hierarchy(d)) {
+	    rdtgroup_pseudo_locked_in_hierarchy(rdtgrp, d)) {
 		rdt_last_cmd_puts("Pseudo-locked region in hierarchy\n");
 		return -EINVAL;
 	}
@@ -282,6 +282,7 @@ static int parse_line(char *line, struct rdt_resource *r,
 			if (r->parse_ctrlval(&data, r, d))
 				return -EINVAL;
 			if (rdtgrp->mode ==  RDT_MODE_PSEUDO_LOCKSETUP) {
+				struct pseudo_lock_portion *p;
 				/*
 				 * In pseudo-locking setup mode and just
 				 * parsed a valid CBM that should be
@@ -290,9 +291,15 @@ static int parse_line(char *line, struct rdt_resource *r,
 				 * the required initialization for single
 				 * region and return.
 				 */
-				rdtgrp->plr->r = r;
-				rdtgrp->plr->d_id = d->id;
-				rdtgrp->plr->cbm = d->new_ctrl;
+				p = kzalloc(sizeof(*p), GFP_KERNEL);
+				if (!p) {
+					rdt_last_cmd_puts("Unable to allocate memory for pseudo-lock portion\n");
+					return -ENOMEM;
+				}
+				p->r = r;
+				p->d_id = d->id;
+				p->cbm = d->new_ctrl;
+				list_add(&p->list, &rdtgrp->plr->portions);
 				return 0;
 			}
 			goto next;
@@ -410,8 +417,11 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
 			goto out;
 		}
 		ret = rdtgroup_parse_resource(resname, tok, rdtgrp);
-		if (ret)
+		if (ret) {
+			if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP)
+				pseudo_lock_region_clear(rdtgrp->plr);
 			goto out;
+		}
 	}
 
 	for_each_alloc_enabled_rdt_resource(r) {
@@ -459,6 +469,7 @@ static void show_doms(struct seq_file *s, struct rdt_resource *r, int closid)
 int rdtgroup_schemata_show(struct kernfs_open_file *of,
 			   struct seq_file *s, void *v)
 {
+	struct pseudo_lock_portion *p;
 	struct rdtgroup *rdtgrp;
 	struct rdt_resource *r;
 	int ret = 0;
@@ -470,8 +481,9 @@ int rdtgroup_schemata_show(struct kernfs_open_file *of,
 			for_each_alloc_enabled_rdt_resource(r)
 				seq_printf(s, "%s:uninitialized\n", r->name);
 		} else if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED) {
-			seq_printf(s, "%s:%d=%x\n", rdtgrp->plr->r->name,
-				   rdtgrp->plr->d_id, rdtgrp->plr->cbm);
+			list_for_each_entry(p, &rdtgrp->plr->portions, list)
+				seq_printf(s, "%s:%d=%x\n", p->r->name, p->d_id,
+					   p->cbm);
 		} else {
 			closid = rdtgrp->closid;
 			for_each_alloc_enabled_rdt_resource(r) {
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 892f38899dda..b041029d4de1 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -145,13 +145,27 @@ struct mongroup {
 	u32			rmid;
 };
 
+/**
+ * struct pseudo_lock_portion - portion of a pseudo-lock region on one resource
+ * @r:		RDT resource to which this pseudo-locked portion
+ *		belongs
+ * @d_id:	ID of cache instance to which this pseudo-locked portion
+ *		belongs
+ * @cbm:	bitmask of the pseudo-locked portion
+ * @list:	Entry in the list of pseudo-locked portion
+ *		belonging to the pseudo-locked region
+ */
+struct pseudo_lock_portion {
+	struct rdt_resource	*r;
+	int			d_id;
+	u32			cbm;
+	struct list_head	list;
+};
+
 /**
  * struct pseudo_lock_region - pseudo-lock region information
- * @r:			RDT resource to which this pseudo-locked region
- *			belongs
- * @d_id:		ID of cache instance to which this pseudo-locked region
- *			belongs
- * @cbm:		bitmask of the pseudo-locked region
+ * @portions:		list of portions across different resources that
+ *			are associated with this pseudo-locked region
  * @lock_thread_wq:	waitqueue used to wait on the pseudo-locking thread
  *			completion
  * @thread_done:	variable used by waitqueue to test if pseudo-locking
@@ -168,9 +182,7 @@ struct mongroup {
  * @pm_reqs:		Power management QoS requests related to this region
  */
 struct pseudo_lock_region {
-	struct rdt_resource	*r;
-	int			d_id;
-	u32			cbm;
+	struct list_head	portions;
 	wait_queue_head_t	lock_thread_wq;
 	int			thread_done;
 	int			cpu;
@@ -569,11 +581,13 @@ bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_resource *r,
 					 struct rdt_domain *d,
 					 unsigned long cbm);
 u32 rdtgroup_pseudo_locked_bits(struct rdt_resource *r, struct rdt_domain *d);
-bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d);
+bool rdtgroup_pseudo_locked_in_hierarchy(struct rdtgroup *selfgrp,
+					 struct rdt_domain *d);
 int rdt_pseudo_lock_init(void);
 void rdt_pseudo_lock_release(void);
 int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp);
 void rdtgroup_pseudo_lock_remove(struct rdtgroup *rdtgrp);
+void pseudo_lock_region_clear(struct pseudo_lock_region *plr);
 struct rdt_domain *get_domain_from_cpu(int cpu, struct rdt_resource *r);
 int update_domains(struct rdt_resource *r, int closid);
 int closids_supported(void);
diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
index 733cb7f34948..717ea26e325b 100644
--- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
+++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
@@ -270,28 +270,85 @@ static int pseudo_lock_cstates_constrain(struct pseudo_lock_region *plr,
  *
  * Return: void
  */
-static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
+void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
 {
+	struct pseudo_lock_portion *p, *tmp;
+
 	plr->size = 0;
 	plr->line_size = 0;
 	kfree(plr->kmem);
 	plr->kmem = NULL;
-	plr->r = NULL;
-	plr->d_id = -1;
-	plr->cbm = 0;
 	pseudo_lock_cstates_relax(plr);
+	if (!list_empty(&plr->portions)) {
+		list_for_each_entry_safe(p, tmp, &plr->portions, list) {
+			list_del(&p->list);
+			kfree(p);
+		}
+	}
 	plr->debugfs_dir = NULL;
 }
 
+/**
+ * pseudo_lock_single_portion_valid - Verify properties of pseudo-lock region
+ * @plr: the main pseudo-lock region
+ * @p: the single portion that makes up the pseudo-locked region
+ *
+ * Verify and initialize properties of the pseudo-locked region.
+ *
+ * Return: -1 if portion of cache unable to be used for pseudo-locking
+ *         0 if portion of cache can be used for pseudo-locking, in
+ *         addition the CPU on which pseudo-locking will be performed will
+ *         be initialized as well as the size and cache line size of the region
+ */
+static int pseudo_lock_single_portion_valid(struct pseudo_lock_region *plr,
+					    struct pseudo_lock_portion *p)
+{
+	struct rdt_domain *d;
+
+	d = rdt_find_domain(p->r, p->d_id, NULL);
+	if (IS_ERR_OR_NULL(d)) {
+		rdt_last_cmd_puts("Cannot find cache domain\n");
+		return -1;
+	}
+
+	plr->cpu = cpumask_first(&d->cpu_mask);
+	if (!cpu_online(plr->cpu)) {
+		rdt_last_cmd_printf("CPU %u not online\n", plr->cpu);
+		goto err_cpu;
+	}
+
+	plr->line_size = get_cache_line_size(plr->cpu, p->r->cache_level);
+	if (plr->line_size == 0) {
+		rdt_last_cmd_puts("Unable to compute cache line length\n");
+		goto err_cpu;
+	}
+
+	if (pseudo_lock_cstates_constrain(plr, &d->cpu_mask)) {
+		rdt_last_cmd_puts("Cannot limit C-states\n");
+		goto err_line;
+	}
+
+	plr->size = rdtgroup_cbm_to_size(p->r, d, p->cbm);
+
+	return 0;
+
+err_line:
+	plr->line_size = 0;
+err_cpu:
+	plr->cpu = 0;
+	return -1;
+}
+
 /**
  * pseudo_lock_region_init - Initialize pseudo-lock region information
  * @plr: pseudo-lock region
  *
  * Called after user provided a schemata to be pseudo-locked. From the
  * schemata the &struct pseudo_lock_region is on entry already initialized
- * with the resource, domain, and capacity bitmask. Here the information
- * required for pseudo-locking is deduced from this data and &struct
- * pseudo_lock_region initialized further. This information includes:
+ * with the resource, domain, and capacity bitmask. Here the
+ * provided data is validated and information required for pseudo-locking
+ * deduced, and &struct pseudo_lock_region initialized further. This
+ * information includes:
  * - size in bytes of the region to be pseudo-locked
  * - cache line size to know the stride with which data needs to be accessed
  *   to be pseudo-locked
@@ -303,44 +360,50 @@ static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
  */
 static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
 {
-	struct rdt_domain *d;
+	struct rdt_resource *l3_resource = &rdt_resources_all[RDT_RESOURCE_L3];
+	struct pseudo_lock_portion *p;
 	int ret;
 
-	/* Pick the first cpu we find that is associated with the cache. */
-	d = rdt_find_domain(plr->r, plr->d_id, NULL);
-	if (IS_ERR_OR_NULL(d)) {
-		rdt_last_cmd_puts("Cache domain offline\n");
-		ret = -ENODEV;
+	if (list_empty(&plr->portions)) {
+		rdt_last_cmd_puts("No pseudo-lock portions provided\n");
 		goto out_region;
 	}
 
-	plr->cpu = cpumask_first(&d->cpu_mask);
-
-	if (!cpu_online(plr->cpu)) {
-		rdt_last_cmd_printf("CPU %u associated with cache not online\n",
-				    plr->cpu);
-		ret = -ENODEV;
-		goto out_region;
+	/* Cache Pseudo-Locking only supported on L2 and L3 resources */
+	list_for_each_entry(p, &plr->portions, list) {
+		if (p->r->rid != RDT_RESOURCE_L2 &&
+		    p->r->rid != RDT_RESOURCE_L3) {
+			rdt_last_cmd_puts("Unsupported resource\n");
+			goto out_region;
+		}
 	}
 
-	plr->line_size = get_cache_line_size(plr->cpu, plr->r->cache_level);
-	if (plr->line_size == 0) {
-		rdt_last_cmd_puts("Unable to determine cache line size\n");
-		ret = -1;
-		goto out_region;
+	/*
+	 * If only one resource requested to be pseudo-locked then:
+	 * - Just a L3 cache portion is valid
+	 * - Just a L2 cache portion on system without L3 cache is valid
+	 */
+	if (list_is_singular(&plr->portions)) {
+		p = list_first_entry(&plr->portions, struct pseudo_lock_portion,
+				     list);
+		if (p->r->rid == RDT_RESOURCE_L3 ||
+		    (p->r->rid == RDT_RESOURCE_L2 &&
+		     !l3_resource->alloc_capable)) {
+			ret = pseudo_lock_single_portion_valid(plr, p);
+			if (ret < 0)
+				goto out_region;
+			return 0;
+		} else {
+			rdt_last_cmd_puts("Invalid resource or just L2 provided when L3 is required\n");
+			goto out_region;
+		}
+	} else {
+		rdt_last_cmd_puts("Multiple pseudo-lock portions unsupported\n");
 	}
 
-	plr->size = rdtgroup_cbm_to_size(plr->r, d, plr->cbm);
-
-	ret = pseudo_lock_cstates_constrain(plr, &d->cpu_mask);
-	if (ret < 0)
-		goto out_region;
-
-	return 0;
-
 out_region:
 	pseudo_lock_region_clear(plr);
-	return ret;
+	return -1;
 }
 
 /**
@@ -362,9 +425,9 @@ static int pseudo_lock_init(struct rdtgroup *rdtgrp)
 	if (!plr)
 		return -ENOMEM;
 
+	INIT_LIST_HEAD(&plr->portions);
 	init_waitqueue_head(&plr->lock_thread_wq);
 	INIT_LIST_HEAD(&plr->pm_reqs);
-	plr->d_id = -1;
 	rdtgrp->plr = plr;
 	return 0;
 }
@@ -845,6 +908,7 @@ bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_resource *r,
 
 /**
  * rdtgroup_pseudo_locked_in_hierarchy - Pseudo-locked region in cache hierarchy
+ * @selfgrp: current resource group testing for overlap
  * @d: RDT domain under test
  *
  * The setup of a pseudo-locked region affects all cache instances within
@@ -856,8 +920,10 @@ bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_resource *r,
  *         if it is not possible to test due to memory allocation or other
  *         failure, false otherwise.
  */
-bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d)
+bool rdtgroup_pseudo_locked_in_hierarchy(struct rdtgroup *selfgrp,
+					 struct rdt_domain *d)
 {
+	struct pseudo_lock_portion *p;
 	cpumask_var_t cpu_with_psl;
 	struct rdtgroup *rdtgrp;
 	struct rdt_domain *d_i;
@@ -871,15 +937,15 @@ bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d)
 	 * associated with them.
 	 */
 	list_for_each_entry(rdtgrp, &rdt_all_groups, rdtgroup_list) {
-		if (rdtgrp->plr && rdtgrp->plr->d_id >= 0) {
-			d_i = rdt_find_domain(rdtgrp->plr->r, rdtgrp->plr->d_id,
-					      NULL);
+		if (!rdtgrp->plr || rdtgrp == selfgrp)
+			continue;
+		list_for_each_entry(p, &rdtgrp->plr->portions, list) {
+			d_i = rdt_find_domain(p->r, p->d_id, NULL);
 			if (IS_ERR_OR_NULL(d_i)) {
 				ret = true;
 				goto out;
 			}
-			cpumask_or(cpu_with_psl, cpu_with_psl,
-				   &d_i->cpu_mask);
+			cpumask_or(cpu_with_psl, cpu_with_psl, &d_i->cpu_mask);
 		}
 	}
 
@@ -1196,6 +1262,7 @@ static int measure_l3_residency(void *_plr)
 static int pseudo_lock_measure_cycles(struct rdtgroup *rdtgrp, int sel)
 {
 	struct pseudo_lock_region *plr = rdtgrp->plr;
+	struct pseudo_lock_portion *p;
 	struct task_struct *thread;
 	struct rdt_domain *d;
 	unsigned int cpu;
@@ -1209,7 +1276,16 @@ static int pseudo_lock_measure_cycles(struct rdtgroup *rdtgrp, int sel)
 		goto out;
 	}
 
-	d = rdt_find_domain(plr->r, plr->d_id, NULL);
+	/*
+	 * Ensure test is run on CPU associated with the pseudo-locked
+	 * region. If pseudo-locked region spans L2 and L3, L2 portion
+	 * would be first in the list and all CPUs associated with the
+	 * L2 cache instance would be associated with pseudo-locked
+	 * region.
+	 */
+	p = list_first_entry(&plr->portions, struct pseudo_lock_portion, list);
+
+	d = rdt_find_domain(p->r, p->d_id, NULL);
 	if (IS_ERR_OR_NULL(d)) {
 		ret = -ENODEV;
 		goto out;
@@ -1511,6 +1587,7 @@ static int pseudo_lock_dev_mmap(struct file *filp, struct vm_area_struct *vma)
 	unsigned long vsize = vma->vm_end - vma->vm_start;
 	unsigned long off = vma->vm_pgoff << PAGE_SHIFT;
 	struct pseudo_lock_region *plr;
+	struct pseudo_lock_portion *p;
 	struct rdtgroup *rdtgrp;
 	unsigned long physical;
 	struct rdt_domain *d;
@@ -1527,7 +1604,16 @@ static int pseudo_lock_dev_mmap(struct file *filp, struct vm_area_struct *vma)
 
 	plr = rdtgrp->plr;
 
-	d = rdt_find_domain(plr->r, plr->d_id, NULL);
+	/*
+	 * If pseudo-locked region spans one cache level, there will be
+	 * only one portion to consider. If pseudo-locked region spans two
+	 * cache levels then the L2 cache portion will be the first entry
+	 * and a CPU associated with it is what a task is required to be run
+	 * on.
+	 */
+	p = list_first_entry(&plr->portions, struct pseudo_lock_portion, list);
+
+	d = rdt_find_domain(p->r, p->d_id, NULL);
 	if (IS_ERR_OR_NULL(d)) {
 		mutex_unlock(&rdtgroup_mutex);
 		return -ENODEV;
@@ -1636,15 +1722,17 @@ void rdt_pseudo_lock_release(void)
  */
 u32 rdtgroup_pseudo_locked_bits(struct rdt_resource *r, struct rdt_domain *d)
 {
+	struct pseudo_lock_portion *p;
 	struct rdtgroup *rdtgrp;
 	u32 pseudo_locked = 0;
 
 	list_for_each_entry(rdtgrp, &rdt_all_groups, rdtgroup_list) {
 		if (!rdtgrp->plr)
 			continue;
-		if (rdtgrp->plr->r && rdtgrp->plr->r->rid == r->rid &&
-		    rdtgrp->plr->d_id == d->id)
-			pseudo_locked |= rdtgrp->plr->cbm;
+		list_for_each_entry(p, &rdtgrp->plr->portions, list) {
+			if (p->r->rid == r->rid && p->d_id == d->id)
+				pseudo_locked |= p->cbm;
+		}
 	}
 
 	return pseudo_locked;
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 0c1786f09963..0039c8087548 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -269,17 +269,31 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of,
 
 	if (rdtgrp) {
 		if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED) {
-			d = rdt_find_domain(rdtgrp->plr->r, rdtgrp->plr->d_id,
-					    NULL);
+			struct pseudo_lock_portion *p;
+
+			/*
+			 * User space needs to know all CPUs associated
+			 * with the pseudo-locked region. When the
+			 * pseudo-locked region spans multiple resources it
+			 * is possible that not all CPUs are associated with
+			 * all portions of the pseudo-locked region.
+			 * Only display CPUs that are associated with _all_
+			 * portions of the region. The first portion in the
+			 * list will be the L2 cache if the region spans
+			 * multiple resource, it is thus only needed to
+			 * print CPUs associated with the first portion.
+			 */
+			p = list_first_entry(&rdtgrp->plr->portions,
+					     struct pseudo_lock_portion, list);
+			d = rdt_find_domain(p->r, p->d_id, NULL);
 			if (IS_ERR_OR_NULL(d)) {
 				rdt_last_cmd_clear();
 				rdt_last_cmd_puts("Cache domain offline\n");
 				ret = -ENODEV;
-			} else {
-				seq_printf(s, is_cpu_list(of) ?
-					   "%*pbl\n" : "%*pb\n",
-					   cpumask_pr_args(&d->cpu_mask));
+				goto out;
 			}
+			seq_printf(s, is_cpu_list(of) ?  "%*pbl\n" : "%*pb\n",
+				   cpumask_pr_args(&d->cpu_mask));
 		} else {
 			seq_printf(s, is_cpu_list(of) ? "%*pbl\n" : "%*pb\n",
 				   cpumask_pr_args(&rdtgrp->cpu_mask));
@@ -287,8 +301,9 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of,
 	} else {
 		ret = -ENOENT;
 	}
-	rdtgroup_kn_unlock(of->kn);
 
+out:
+	rdtgroup_kn_unlock(of->kn);
 	return ret;
 }
 
@@ -1304,8 +1319,19 @@ static int rdtgroup_size_show(struct kernfs_open_file *of,
 	}
 
 	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED) {
-		seq_printf(s, "%*s:", max_name_width, rdtgrp->plr->r->name);
-		seq_printf(s, "%d=%u\n", rdtgrp->plr->d_id, rdtgrp->plr->size);
+		struct pseudo_lock_portion *portion;
+
+		/*
+		 * While the portions of the L2 and L3 caches allocated for a
+		 * pseudo-locked region may be different, the size used for
+		 * the pseudo-locked region is the same.
+		 */
+		list_for_each_entry(portion, &rdtgrp->plr->portions, list) {
+			seq_printf(s, "%*s:", max_name_width,
+				   portion->r->name);
+			seq_printf(s, "%d=%u\n", portion->d_id,
+				   rdtgrp->plr->size);
+		}
 		goto out;
 	}
 
-- 
2.17.2

[PATCH V2 09/10] x86/resctrl: Pseudo-lock portions of multiple resources

[Reinette Chatre]

A cache pseudo-locked region may span more than one level of cache. A
part of the pseudo-locked region that falls on one cache level is
referred to as a pseudo-lock portion that was introduced previously.

Now a pseudo-locked region is allowed to have two portions instead of
the previous limit of one. When a pseudo-locked region consists out of
two portions it can only span a L2 and L3 resctrl resource.
When a pseudo-locked region consists out of a L2 and L3 portion then
there are some requirements:
- the L2 and L3 cache has to be in same cache hierarchy
- the L3 portion must be same size or larger than L2 portion

As documented in previous changes the list of portions are
maintained so that the L2 portion would always appear first in the list
to simplify any information retrieval.

Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 142 +++++++++++++++++++++-
 1 file changed, 139 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
index 717ea26e325b..7ab4e85a33a7 100644
--- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
+++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
@@ -339,13 +339,104 @@ static int pseudo_lock_single_portion_valid(struct pseudo_lock_region *plr,
 	return -1;
 }
 
+/**
+ * pseudo_lock_l2_l3_portions_valid - Verify region across L2 and L3
+ * @plr: Pseudo-Locked region
+ * @l2_portion: L2 Cache portion of pseudo-locked region
+ * @l3_portion: L3 Cache portion of pseudo-locked region
+ *
+ * User requested a pseudo-locked region consisting of a L2 as well as L3
+ * cache portion. The portions are tested as follows:
+ *   - L2 and L3 cache instances have to be in the same cache hierarchy.
+ *     This is tested by ensuring that the L2 portion's cpumask is a
+ *     subset of the L3 portion's cpumask.
+ *   - L3 portion must be same size or larger than L2 portion.
+ *
+ * Return: -1 if the portions are unable to be used for a pseudo-locked
+ *         region, 0 if the portions could be used for a pseudo-locked
+ *         region. When returning 0:
+ *         - the pseudo-locked region's size, line_size (cache line length)
+ *           and CPU on which locking thread will be run are set.
+ *         - CPUs associated with L2 cache portion are constrained from
+ *           entering C-state that will affect the pseudo-locked region.
+ */
+static int pseudo_lock_l2_l3_portions_valid(struct pseudo_lock_region *plr,
+					    struct pseudo_lock_portion *l2_p,
+					    struct pseudo_lock_portion *l3_p)
+{
+	struct rdt_domain *l2_d, *l3_d;
+	unsigned int l2_size, l3_size;
+
+	l2_d = rdt_find_domain(l2_p->r, l2_p->d_id, NULL);
+	if (IS_ERR_OR_NULL(l2_d)) {
+		rdt_last_cmd_puts("Cannot locate L2 cache domain\n");
+		return -1;
+	}
+
+	l3_d = rdt_find_domain(l3_p->r, l3_p->d_id, NULL);
+	if (IS_ERR_OR_NULL(l3_d)) {
+		rdt_last_cmd_puts("Cannot locate L3 cache domain\n");
+		return -1;
+	}
+
+	if (!cpumask_subset(&l2_d->cpu_mask, &l3_d->cpu_mask)) {
+		rdt_last_cmd_puts("L2 and L3 caches need to be in same hierarchy\n");
+		return -1;
+	}
+
+	if (pseudo_lock_cstates_constrain(plr, &l2_d->cpu_mask)) {
+		rdt_last_cmd_puts("Cannot limit C-states\n");
+		return -1;
+	}
+
+	l2_size = rdtgroup_cbm_to_size(l2_p->r, l2_d, l2_p->cbm);
+	l3_size = rdtgroup_cbm_to_size(l3_p->r, l3_d, l3_p->cbm);
+
+	if (l2_size > l3_size) {
+		rdt_last_cmd_puts("L3 cache portion has to be same size or larger than L2 cache portion\n");
+		goto err_size;
+	}
+
+	plr->size = l2_size;
+
+	l2_size = get_cache_line_size(cpumask_first(&l2_d->cpu_mask),
+				      l2_p->r->cache_level);
+	l3_size = get_cache_line_size(cpumask_first(&l3_d->cpu_mask),
+				      l3_p->r->cache_level);
+	if (l2_size != l3_size) {
+		rdt_last_cmd_puts("L2 and L3 caches have different coherency cache line sizes\n");
+		goto err_line;
+	}
+
+	plr->line_size = l2_size;
+
+	plr->cpu = cpumask_first(&l2_d->cpu_mask);
+
+	if (!cpu_online(plr->cpu)) {
+		rdt_last_cmd_printf("CPU %u associated with cache not online\n",
+				    plr->cpu);
+		goto err_cpu;
+	}
+
+	return 0;
+
+err_cpu:
+	plr->line_size = 0;
+	plr->cpu = 0;
+err_line:
+	plr->size = 0;
+err_size:
+	pseudo_lock_cstates_relax(plr);
+	return -1;
+}
+
 /**
  * pseudo_lock_region_init - Initialize pseudo-lock region information
  * @plr: pseudo-lock region
  *
  * Called after user provided a schemata to be pseudo-locked. From the
  * schemata the &struct pseudo_lock_region is on entry already initialized
- * with the resource, domain, and capacity bitmask. Here the
+ * with the resource(s), domain(s), and capacity bitmask(s). Here the
  * provided data is validated and information required for pseudo-locking
  * deduced, and &struct pseudo_lock_region initialized further. This
  * information includes:
@@ -355,13 +446,24 @@ static int pseudo_lock_single_portion_valid(struct pseudo_lock_region *plr,
  * - a cpu associated with the cache instance on which the pseudo-locking
  *   flow can be executed
  *
+ * A user provides a schemata for a pseudo-locked region. This schemata may
+ * contain portions that span different resources, for example, a cache
+ * pseudo-locked region that spans L2 and L3 cache. After the schemata is
+ * parsed into portions it needs to be verified that the provided portions
+ * are valid with the following tests:
+ *
+ * - L2 only portion on system that has only L2 resource - OK
+ * - L3 only portion on any system that supports it - OK
+ * - L2 portion on system that has L3 resource - require L3 portion
+ **
+ *
  * Return: 0 on success, <0 on failure. Descriptive error will be written
  * to last_cmd_status buffer.
  */
 static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
 {
 	struct rdt_resource *l3_resource = &rdt_resources_all[RDT_RESOURCE_L3];
-	struct pseudo_lock_portion *p;
+	struct pseudo_lock_portion *p, *n_p, *tmp;
 	int ret;
 
 	if (list_empty(&plr->portions)) {
@@ -397,8 +499,42 @@ static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
 			rdt_last_cmd_puts("Invalid resource or just L2 provided when L3 is required\n");
 			goto out_region;
 		}
+	}
+
+	/*
+	 * List is neither empty nor singular, process first and second portions
+	 */
+	p = list_first_entry(&plr->portions, struct pseudo_lock_portion, list);
+	n_p = list_next_entry(p, list);
+
+	/*
+	 * If the second portion is not also the last portion user provided
+	 * more portions than can be supported.
+	 */
+	tmp = list_last_entry(&plr->portions, struct pseudo_lock_portion, list);
+	if (n_p != tmp) {
+		rdt_last_cmd_puts("Only two pseudo-lock portions supported\n");
+		goto out_region;
+	}
+
+	if (p->r->rid == RDT_RESOURCE_L2 && n_p->r->rid == RDT_RESOURCE_L3) {
+		ret = pseudo_lock_l2_l3_portions_valid(plr, p, n_p);
+		if (ret < 0)
+			goto out_region;
+		return 0;
+	} else if (p->r->rid == RDT_RESOURCE_L3 &&
+		   n_p->r->rid == RDT_RESOURCE_L2) {
+		if (pseudo_lock_l2_l3_portions_valid(plr, n_p, p) == 0) {
+			/*
+			 * Let L2 and L3 portions appear in order in the
+			 * portions list in support of consistent output to
+			 * user space.
+			 */
+			list_rotate_left(&plr->portions);
+			return 0;
+		}
 	} else {
-		rdt_last_cmd_puts("Multiple pseudo-lock portions unsupported\n");
+		rdt_last_cmd_puts("Invalid combination of resources\n");
 	}
 
 out_region:
-- 
2.17.2

[PATCH V2 10/10] x86/resctrl: Only pseudo-lock L3 cache when inclusive

[Reinette Chatre]

Cache pseudo-locking is a model specific feature and platforms
supporting this feature are added by adding the x86 model data to the
source code after cache pseudo-locking has been validated for the
particular platform.

Indicating support for cache pseudo-locking for an entire platform is
sufficient when the cache characteristics of the platform is the same
for all instances of the platform. If this is not the case then an
additional check needs to be added. In particular, it is currently only
possible to pseudo-lock an L3 cache region if the L3 cache is inclusive
of lower level caches. If the L3 cache is not inclusive then any
pseudo-locked data would be evicted from the pseudo-locked region when
it is moved to the L2 cache.

When some SKUs of a platform may have inclusive cache while other SKUs
may have non inclusive cache it is necessary to, in addition of checking
if the platform supports cache pseudo-locking, also check if the cache
being pseudo-locked is inclusive.

Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 35 +++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
index 7ab4e85a33a7..b4fff88572bd 100644
--- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
+++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
@@ -125,6 +125,30 @@ 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
@@ -317,6 +341,12 @@ 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)) {
+		rdt_last_cmd_puts("L3 cache not inclusive\n");
+		goto err_cpu;
+	}
+
 	plr->line_size = get_cache_line_size(plr->cpu, p->r->cache_level);
 	if (plr->line_size == 0) {
 		rdt_last_cmd_puts("Unable to compute cache line length\n");
@@ -418,6 +448,11 @@ 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)) {
+		rdt_last_cmd_puts("L3 cache not inclusive\n");
+		goto err_cpu;
+	}
+
 	return 0;
 
 err_cpu:
-- 
2.17.2

Re: [PATCH V2 00/10] x86/CPU and x86/resctrl: Support pseudo-lock regions spanning L2 and L3 cache

[Thomas Gleixner]

Reinette,

On Tue, 30 Jul 2019, Reinette Chatre wrote:
> Patches 2 to 8 to the resctrl subsystem are preparing for the new feature
> and should result in no functional change, but some comments do refer to
> the new feature. Support for pseudo-locked regions spanning L2 and L3 cache
> is introduced in patches 9 and 10.
> 
> Your feedback will be greatly appreciated.

I've already skimmed V1 and did not find something horrible, but I want to
hand the deeper review off to Borislav who should return from his well
earned vacation soon.

Thanks,

	tglx

Re: [PATCH V2 00/10] x86/CPU and x86/resctrl: Support pseudo-lock regions spanning L2 and L3 cache

[Reinette Chatre]

Hi Thomas,

On 7/30/2019 1:00 PM, Thomas Gleixner wrote:
> On Tue, 30 Jul 2019, Reinette Chatre wrote:
>> Patches 2 to 8 to the resctrl subsystem are preparing for the new feature
>> and should result in no functional change, but some comments do refer to
>> the new feature. Support for pseudo-locked regions spanning L2 and L3 cache
>> is introduced in patches 9 and 10.
>>
>> Your feedback will be greatly appreciated.
> 
> I've already skimmed V1 and did not find something horrible, but I want to
> hand the deeper review off to Borislav who should return from his well
> earned vacation soon.

Thank you very much. I look forward to working with Borislav on his return.

Reinette

Re: [PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower level caches

[Borislav Petkov]

On Tue, Jul 30, 2019 at 10:29:35AM -0700, Reinette Chatre wrote:
> Deterministic cache parameters can be learned from CPUID leaf 04H.
> Executing CPUID with a particular index in EAX would return the cache
> parameters associated with that index in the EAX, EBX, ECX, and EDX
> registers.
> 
> At this time, when discovering cache parameters for a particular cache
> index, only the parameters returned in EAX, EBX, and ECX are parsed.
> Parameters returned in EDX are ignored. One of the parameters in EDX,
> whether the cache is inclusive of lower level caches, is valuable to
> know when determining if a system can support L3 cache pseudo-locking.
> If the L3 cache is not inclusive then pseudo-locked data within the L3
> cache would be evicted when migrated to L2.
> 
> Add support for parsing the cache parameters obtained from EDX and make
> the inclusive cache parameter available via the cacheinfo that can be
> queried from the cache pseudo-locking code.
> 
> Do not expose this information to user space at this time. At this time
> this information is required within the kernel only. Also, it is
> not obvious what the best formatting of this information should be in
> support of the variety of ways users may use this information.
> 
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
>  arch/x86/kernel/cpu/cacheinfo.c | 42 +++++++++++++++++++++++++++++----
>  include/linux/cacheinfo.h       |  4 ++++
>  2 files changed, 42 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
> index c7503be92f35..3b678f46be53 100644
> --- a/arch/x86/kernel/cpu/cacheinfo.c
> +++ b/arch/x86/kernel/cpu/cacheinfo.c
> @@ -154,10 +154,33 @@ union _cpuid4_leaf_ecx {
>  	u32 full;
>  };
>  
> +/*
> + * According to details about CPUID instruction documented in Intel SDM
> + * the third bit of the EDX register is used to indicate if complex
> + * cache indexing is in use.
> + * According to AMD specification (Open Source Register Reference For AMD
> + * Family 17h processors Models 00h-2Fh 56255 Rev 3.03 - July, 2018), only
> + * the first two bits are in use. Since HYGON is based on AMD the
> + * assumption is that it supports the same.
> + *
> + * There is no consumer for the complex indexing information so this bit is
> + * not added to the declaration of what processor can provide in EDX
> + * register. The declaration thus only considers bits supported by all
> + * architectures.
> + */

I don't think you need all that commenting in here since it'll become
stale as soon as the other vendors change their respective 0x8000001D
leafs. It is sufficient to say that the union below is going to contain
only the bits shared by all vendors.

> +union _cpuid4_leaf_edx {
> +	struct {
> +		unsigned int		wbinvd_no_guarantee:1;
					^^^^^^^^^^^^^^^^^^^^^

That's unused so no need to name the bit. You only need "inclusive".

> +		unsigned int		inclusive:1;
> +	} split;
> +	u32 full;
> +};
> +
>  struct _cpuid4_info_regs {
>  	union _cpuid4_leaf_eax eax;
>  	union _cpuid4_leaf_ebx ebx;
>  	union _cpuid4_leaf_ecx ecx;
> +	union _cpuid4_leaf_edx edx;
>  	unsigned int id;
>  	unsigned long size;
>  	struct amd_northbridge *nb;
> @@ -595,21 +618,24 @@ cpuid4_cache_lookup_regs(int index, struct _cpuid4_info_regs *this_leaf)
>  	union _cpuid4_leaf_eax	eax;
>  	union _cpuid4_leaf_ebx	ebx;
>  	union _cpuid4_leaf_ecx	ecx;
> -	unsigned		edx;
> +	union _cpuid4_leaf_edx	edx;
> +
> +	edx.full = 0;

Yeah, the proper way to shut up gcc is to do this (diff ontop):

---
diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
index 3b678f46be53..4ff4e95e22b4 100644
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -252,7 +252,8 @@ static const enum cache_type cache_type_map[] = {
 static void
 amd_cpuid4(int leaf, union _cpuid4_leaf_eax *eax,
 		     union _cpuid4_leaf_ebx *ebx,
-		     union _cpuid4_leaf_ecx *ecx)
+		     union _cpuid4_leaf_ecx *ecx,
+		     union _cpuid4_leaf_edx *edx)
 {
 	unsigned dummy;
 	unsigned line_size, lines_per_tag, assoc, size_in_kb;
@@ -264,6 +265,7 @@ amd_cpuid4(int leaf, union _cpuid4_leaf_eax *eax,
 	eax->full = 0;
 	ebx->full = 0;
 	ecx->full = 0;
+	edx->full = 0;
 
 	cpuid(0x80000005, &dummy, &dummy, &l1d.val, &l1i.val);
 	cpuid(0x80000006, &dummy, &dummy, &l2.val, &l3.val);
@@ -620,14 +622,12 @@ cpuid4_cache_lookup_regs(int index, struct _cpuid4_info_regs *this_leaf)
 	union _cpuid4_leaf_ecx	ecx;
 	union _cpuid4_leaf_edx	edx;
 
-	edx.full = 0;
-
 	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
 		if (boot_cpu_has(X86_FEATURE_TOPOEXT))
 			cpuid_count(0x8000001d, index, &eax.full,
 				    &ebx.full, &ecx.full, &edx.full);
 		else
-			amd_cpuid4(index, &eax, &ebx, &ecx);
+			amd_cpuid4(index, &eax, &ebx, &ecx, &edx);
 		amd_init_l3_cache(this_leaf, index);
 	} else if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
 		cpuid_count(0x8000001d, index, &eax.full,

>  
>  	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
>  		if (boot_cpu_has(X86_FEATURE_TOPOEXT))
>  			cpuid_count(0x8000001d, index, &eax.full,
> -				    &ebx.full, &ecx.full, &edx);
> +				    &ebx.full, &ecx.full, &edx.full);
>  		else
>  			amd_cpuid4(index, &eax, &ebx, &ecx);
>  		amd_init_l3_cache(this_leaf, index);
>  	} else if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
>  		cpuid_count(0x8000001d, index, &eax.full,
> -			    &ebx.full, &ecx.full, &edx);
> +			    &ebx.full, &ecx.full, &edx.full);
>  		amd_init_l3_cache(this_leaf, index);
>  	} else {
> -		cpuid_count(4, index, &eax.full, &ebx.full, &ecx.full, &edx);
> +		cpuid_count(4, index, &eax.full, &ebx.full, &ecx.full,
> +			    &edx.full);

Let that line stick out and rename "index" to "idx" so that it fits the
80 cols rule.

>  	}
>  
>  	if (eax.split.type == CTYPE_NULL)
> @@ -618,6 +644,7 @@ cpuid4_cache_lookup_regs(int index, struct _cpuid4_info_regs *this_leaf)
>  	this_leaf->eax = eax;
>  	this_leaf->ebx = ebx;
>  	this_leaf->ecx = ecx;
> +	this_leaf->edx = edx;
>  	this_leaf->size = (ecx.split.number_of_sets          + 1) *
>  			  (ebx.split.coherency_line_size     + 1) *
>  			  (ebx.split.physical_line_partition + 1) *
> @@ -982,6 +1009,13 @@ 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;

<---- newline here.

> +	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;

A whole int for a single bit?

Why don't you define the CACHE_INCLUSIVE_SET thing as CACHE_INCLUSIVE to
mean if set, the cache is inclusive, if not, it isn't or unknown?

> +	}
>  	this_leaf->priv = base->nb;
>  }
>  
> diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
> index 46b92cd61d0c..cdc7a9d6923f 100644
> --- a/include/linux/cacheinfo.h
> +++ b/include/linux/cacheinfo.h
> @@ -33,6 +33,8 @@ 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
> @@ -55,6 +57,7 @@ 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)
> @@ -66,6 +69,7 @@ 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;
> -- 
> 2.17.2

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower level caches

[Reinette Chatre]

Hi Borislav,

On 8/2/2019 11:03 AM, Borislav Petkov wrote:
> On Tue, Jul 30, 2019 at 10:29:35AM -0700, Reinette Chatre wrote:
>> +/*
>> + * According to details about CPUID instruction documented in Intel SDM
>> + * the third bit of the EDX register is used to indicate if complex
>> + * cache indexing is in use.
>> + * According to AMD specification (Open Source Register Reference For AMD
>> + * Family 17h processors Models 00h-2Fh 56255 Rev 3.03 - July, 2018), only
>> + * the first two bits are in use. Since HYGON is based on AMD the
>> + * assumption is that it supports the same.
>> + *
>> + * There is no consumer for the complex indexing information so this bit is
>> + * not added to the declaration of what processor can provide in EDX
>> + * register. The declaration thus only considers bits supported by all
>> + * architectures.
>> + */
> 
> I don't think you need all that commenting in here since it'll become
> stale as soon as the other vendors change their respective 0x8000001D
> leafs. It is sufficient to say that the union below is going to contain
> only the bits shared by all vendors.

Will do.

> 
>> +union _cpuid4_leaf_edx {
>> +	struct {
>> +		unsigned int		wbinvd_no_guarantee:1;
> 					^^^^^^^^^^^^^^^^^^^^^
> 
> That's unused so no need to name the bit. You only need "inclusive".

Will do.

> 
>> +		unsigned int		inclusive:1;
>> +	} split;
>> +	u32 full;
>> +};
>> +
>>  struct _cpuid4_info_regs {
>>  	union _cpuid4_leaf_eax eax;
>>  	union _cpuid4_leaf_ebx ebx;
>>  	union _cpuid4_leaf_ecx ecx;
>> +	union _cpuid4_leaf_edx edx;
>>  	unsigned int id;
>>  	unsigned long size;
>>  	struct amd_northbridge *nb;
>> @@ -595,21 +618,24 @@ cpuid4_cache_lookup_regs(int index, struct _cpuid4_info_regs *this_leaf)
>>  	union _cpuid4_leaf_eax	eax;
>>  	union _cpuid4_leaf_ebx	ebx;
>>  	union _cpuid4_leaf_ecx	ecx;
>> -	unsigned		edx;
>> +	union _cpuid4_leaf_edx	edx;
>> +
>> +	edx.full = 0;
> 
> Yeah, the proper way to shut up gcc is to do this (diff ontop):
> 
> ---
> diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
> index 3b678f46be53..4ff4e95e22b4 100644
> --- a/arch/x86/kernel/cpu/cacheinfo.c
> +++ b/arch/x86/kernel/cpu/cacheinfo.c
> @@ -252,7 +252,8 @@ static const enum cache_type cache_type_map[] = {
>  static void
>  amd_cpuid4(int leaf, union _cpuid4_leaf_eax *eax,
>  		     union _cpuid4_leaf_ebx *ebx,
> -		     union _cpuid4_leaf_ecx *ecx)
> +		     union _cpuid4_leaf_ecx *ecx,
> +		     union _cpuid4_leaf_edx *edx)
>  {
>  	unsigned dummy;
>  	unsigned line_size, lines_per_tag, assoc, size_in_kb;
> @@ -264,6 +265,7 @@ amd_cpuid4(int leaf, union _cpuid4_leaf_eax *eax,
>  	eax->full = 0;
>  	ebx->full = 0;
>  	ecx->full = 0;
> +	edx->full = 0;
>  
>  	cpuid(0x80000005, &dummy, &dummy, &l1d.val, &l1i.val);
>  	cpuid(0x80000006, &dummy, &dummy, &l2.val, &l3.val);
> @@ -620,14 +622,12 @@ cpuid4_cache_lookup_regs(int index, struct _cpuid4_info_regs *this_leaf)
>  	union _cpuid4_leaf_ecx	ecx;
>  	union _cpuid4_leaf_edx	edx;
>  
> -	edx.full = 0;
> -
>  	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
>  		if (boot_cpu_has(X86_FEATURE_TOPOEXT))
>  			cpuid_count(0x8000001d, index, &eax.full,
>  				    &ebx.full, &ecx.full, &edx.full);
>  		else
> -			amd_cpuid4(index, &eax, &ebx, &ecx);
> +			amd_cpuid4(index, &eax, &ebx, &ecx, &edx);
>  		amd_init_l3_cache(this_leaf, index);
>  	} else if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
>  		cpuid_count(0x8000001d, index, &eax.full,
> 

Thank you very much. I'll fix this.

>>  
>>  	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
>>  		if (boot_cpu_has(X86_FEATURE_TOPOEXT))
>>  			cpuid_count(0x8000001d, index, &eax.full,
>> -				    &ebx.full, &ecx.full, &edx);
>> +				    &ebx.full, &ecx.full, &edx.full);
>>  		else
>>  			amd_cpuid4(index, &eax, &ebx, &ecx);
>>  		amd_init_l3_cache(this_leaf, index);
>>  	} else if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
>>  		cpuid_count(0x8000001d, index, &eax.full,
>> -			    &ebx.full, &ecx.full, &edx);
>> +			    &ebx.full, &ecx.full, &edx.full);
>>  		amd_init_l3_cache(this_leaf, index);
>>  	} else {
>> -		cpuid_count(4, index, &eax.full, &ebx.full, &ecx.full, &edx);
>> +		cpuid_count(4, index, &eax.full, &ebx.full, &ecx.full,
>> +			    &edx.full);
> 
> Let that line stick out and rename "index" to "idx" so that it fits the
> 80 cols rule.

Will do. Do you prefer a new prepare patch that does the renaming before
this patch or will you be ok with the renaming done within this patch?

> 
>>  	}
>>  
>>  	if (eax.split.type == CTYPE_NULL)
>> @@ -618,6 +644,7 @@ cpuid4_cache_lookup_regs(int index, struct _cpuid4_info_regs *this_leaf)
>>  	this_leaf->eax = eax;
>>  	this_leaf->ebx = ebx;
>>  	this_leaf->ecx = ecx;
>> +	this_leaf->edx = edx;
>>  	this_leaf->size = (ecx.split.number_of_sets          + 1) *
>>  			  (ebx.split.coherency_line_size     + 1) *
>>  			  (ebx.split.physical_line_partition + 1) *
>> @@ -982,6 +1009,13 @@ 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;
> 
> <---- newline here.
> 

Will do.

>> +	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;
> 
> A whole int for a single bit?
> 
> Why don't you define the CACHE_INCLUSIVE_SET thing as CACHE_INCLUSIVE to
> mean if set, the cache is inclusive, if not, it isn't or unknown?
> 
This patch only makes it possible to determine whether cache is
inclusive for some x86 platforms while all platforms of all
architectures are given visibility into this new "inclusive" cache
information field within the global "struct cacheinfo". I did the above
because I wanted it to be possible to distinguish between the "not
inclusive" vs "unknown" case. With the above the "inclusive" field would
only be considered valid if "CACHE_INCLUSIVE_SET" is set.

You do seem to acknowledge this exact motivation though ... since you
explicitly state "isn't or unknown". Do I understand correctly that you
are ok with it not being possible to distinguish between "not inclusive"
and "unknown"?

Thank you very much for your valuable feedback.

Reinette

Re: [PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower level caches

[Borislav Petkov]

On Fri, Aug 02, 2019 at 01:11:13PM -0700, Reinette Chatre wrote:
> Will do. Do you prefer a new prepare patch that does the renaming before
> this patch or will you be ok with the renaming done within this patch?

Nah, within this patch is ok.

> This patch only makes it possible to determine whether cache is
> inclusive for some x86 platforms while all platforms of all
> architectures are given visibility into this new "inclusive" cache
> information field within the global "struct cacheinfo".

And this begs the question: do the other architectures even need that
thing exposed? As it is now, this is x86-only so I'm wondering whether
adding all that machinery to the generic struct cacheinfo is even needed
at this point.

TBH, I'd do it differently: read CPUID at init time and cache the
information whether the cache is inclusive locally and be done with it.
It is going to be a single system-wide bit anyway as I'd strongly assume
cache settings like inclusivity should be the same across the whole
system.

When the other arches do need it, we can extract that info "up" into the
generic layer.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH V2 04/10] x86/resctrl: Set cache line size using new utility

[Borislav Petkov]

On Tue, Jul 30, 2019 at 10:29:38AM -0700, Reinette Chatre wrote:
> In preparation for support of pseudo-locked regions spanning two
> cache levels the cache line size computation is moved to a utility.

Please write this in active voice: "Move the cache line size computation
to a utility function in preparation... "

And yes, "a utility" solely sounds like you're adding a tool which does
that. But it is simply a separate function. :-)

> Setting of the cache line size is moved a few lines earlier, before
> the C-states are constrained, to reduce the amount of cleanup needed
> on failure.

And in general, that passive voice is kinda hard to read. To quote from
Documentation/process/submitting-patches.rst:

 "Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
  instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
  to do frotz", as if you are giving orders to the codebase to change
  its behaviour."

Please check all your commit messages.

> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
>  arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 42 +++++++++++++++++------
>  1 file changed, 31 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> index 110ae4b4f2e4..884976913326 100644
> --- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> +++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> @@ -101,6 +101,30 @@ static u64 get_prefetch_disable_bits(void)
>  	return 0;
>  }
>  
> +/**
> + * get_cache_line_size - Determine the cache coherency line size
> + * @cpu: CPU with which cache is associated
> + * @level: Cache level
> + *
> + * Context: @cpu has to be online.
> + * Return: The cache coherency line size for cache level @level associated
> + * with CPU @cpu. Zero on failure.
> + */
> +static unsigned int get_cache_line_size(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].coherency_line_size;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * pseudo_lock_minor_get - Obtain available minor number
>   * @minor: Pointer to where new minor number will be stored
> @@ -281,9 +305,7 @@ static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
>   */
>  static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
>  {
> -	struct cpu_cacheinfo *ci;
>  	int ret;
> -	int i;
>  
>  	/* Pick the first cpu we find that is associated with the cache. */
>  	plr->cpu = cpumask_first(&plr->d->cpu_mask);
> @@ -295,7 +317,12 @@ static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
>  		goto out_region;
>  	}
>  
> -	ci = get_cpu_cacheinfo(plr->cpu);
> +	plr->line_size = get_cache_line_size(plr->cpu, plr->r->cache_level);
> +	if (plr->line_size == 0) {

	if (!plr->...)

> +		rdt_last_cmd_puts("Unable to determine cache line size\n");
> +		ret = -1;
> +		goto out_region;
> +	}
>  
>  	plr->size = rdtgroup_cbm_to_size(plr->r, plr->d, plr->cbm);
>  
> @@ -303,15 +330,8 @@ static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
>  	if (ret < 0)
>  		goto out_region;
>  
> -	for (i = 0; i < ci->num_leaves; i++) {
> -		if (ci->info_list[i].level == plr->r->cache_level) {
> -			plr->line_size = ci->info_list[i].coherency_line_size;
> -			return 0;
> -		}
> -	}
> +	return 0;
>  
> -	ret = -1;
> -	rdt_last_cmd_puts("Unable to determine cache line size\n");
>  out_region:
>  	pseudo_lock_region_clear(plr);
>  	return ret;
> -- 
> 2.17.2
> 

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH V2 06/10] x86/resctrl: Introduce utility to return pseudo-locked cache portion

[Borislav Petkov]

On Tue, Jul 30, 2019 at 10:29:40AM -0700, Reinette Chatre wrote:
> To prevent eviction of pseudo-locked memory it is required that no
> other resource group uses any portion of a cache that is in use by
> a cache pseudo-locked region.
> 
> Introduce a utility that will return a Capacity BitMask (CBM) indicating
> all portions of a provided cache instance being used for cache
> pseudo-locking. This CBM can be used in overlap checking as well as
> cache usage reporting.
> 
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
>  arch/x86/kernel/cpu/resctrl/internal.h    |  1 +
>  arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 23 +++++++++++++++++++++++
>  2 files changed, 24 insertions(+)

s/utility/utility function/g

so that there's no ambiguity...

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower level caches

[Reinette Chatre]

Hi Borislav,

On 8/3/2019 2:44 AM, Borislav Petkov wrote:
> On Fri, Aug 02, 2019 at 01:11:13PM -0700, Reinette Chatre wrote:
>> This patch only makes it possible to determine whether cache is
>> inclusive for some x86 platforms while all platforms of all
>> architectures are given visibility into this new "inclusive" cache
>> information field within the global "struct cacheinfo".
> 
> And this begs the question: do the other architectures even need that
> thing exposed? As it is now, this is x86-only so I'm wondering whether
> adding all that machinery to the generic struct cacheinfo is even needed
> at this point.

I do not know if more users would appear in the future but the goal of
this patch is to make this information available to resctrl. Since there
are a few ways to do so I really appreciate your guidance on how to do
this right. I'd be happy to make needed changes.

> TBH, I'd do it differently: read CPUID at init time and cache the
> information whether the cache is inclusive locally and be done with it.
> It is going to be a single system-wide bit anyway as I'd strongly assume
> cache settings like inclusivity should be the same across the whole
> system.

I've been digesting your comment and tried a few options but I have been
unable to come up with something that fulfill all your suggestions -
specifically the "single system-wide bit" one. These areas of code are
unfamiliar to me so I am not confident what I came up with for other
suggestions are the right way either.

So far it seems I can do the following:
Introduce a new bitfield into struct cpuinfo_x86. There is one existing
bitfield in this structure ("initialized") so we could add a new one
("x86_cache_l3_inclusive"?) just before it. With this information
included in this struct it becomes available via the global
boot_cpu_data, this seems to address the "system-wide bit" but this
struct is also maintained for all other CPUs on the system so may not be
what you had in mind (not a "single system-wide bit")?

If proceeding with inclusion into struct cpuinfo_x86 this new bitfield
can be initialized within init_intel_cacheinfo(). There are a few cache
properties within cpuinfo_x86 and they are initialized in a variety of
paths. init_intel_cacheinfo() is initialized via init_intel() and it
already has the needed CPUID information available making initialization
here straight forward. Alternatively there is also identify_cpu() that
also initializes many cache properties ... but would need some more code
to obtain needed values.

Finally, if the above is done, the resctrl code could just refer to this
new property directly as obtained from boot_cpu_data.x86_cache_l3_inclusive

What do you think?

> 
> When the other arches do need it, we can extract that info "up" into the
> generic layer.

Reinette

Re: [PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower level caches

[Borislav Petkov]

On Mon, Aug 05, 2019 at 10:57:04AM -0700, Reinette Chatre wrote:
> What do you think?

Actually, I was thinking about something a lot simpler: something
along the lines of adding the CPUID check in a helper function which
rdt_pseudo_lock_init() calls. If the cache is not inclusive - and my
guess is it would suffice to check any cache but I'd prefer you correct
me on that - you simply return error and rdt_pseudo_lock_init() returns
early without doing any futher init.

How does that sound?

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower level caches

[Reinette Chatre]

Hi Borislav,

On 8/6/2019 8:57 AM, Borislav Petkov wrote:
> On Mon, Aug 05, 2019 at 10:57:04AM -0700, Reinette Chatre wrote:
>> What do you think?
> 
> Actually, I was thinking about something a lot simpler: something
> along the lines of adding the CPUID check in a helper function which
> rdt_pseudo_lock_init() calls. If the cache is not inclusive - and my
> guess is it would suffice to check any cache but I'd prefer you correct
> me on that - you simply return error and rdt_pseudo_lock_init() returns
> early without doing any futher init.
> 
> How does that sound?

I am a bit cautious about this. When I started this work I initially
added a helper function to resctrl that calls CPUID to determine if the
cache is inclusive. At that time I became aware of a discussion
motivating against scattered CPUID calls and motivating for one instance
of CPUID information:
http://lkml.kernel.org/r/alpine.DEB.2.21.1906162141301.1760@nanos.tec.linutronix.de

My interpretation of the above resulted in a move away from calling
CPUID in resctrl to the patch you are reviewing now.

I do indeed prefer a simple approach and would change to what you
suggest if you find it to be best.

To answer your question about checking any cache: this seems to be
different between L2 and L3. On the Atom systems where L2 pseudo-locking
works well the L2 cache is not inclusive. We are also working on
supporting cache pseudo-locking on L3 cache that is not inclusive.

I could add the single CPUID check during rdt_pseudo_lock_init(),
checking on any CPU should work. I think it would be simpler (reasons
below) to initialize that single system-wide setting in
rdt_pseudo_lock_init() and keep the result locally in the pseudo-locking
code, that can be referred to when the user requests the pseudo-locked
region.

Simpler for two reasons:
* Prevent future platform specific code within rdt_pseudo_lock_init()
trying to pick when L3 is allowed to be inclusive or not.
* rdt_pseudo_lock_init() does not currently make decision whether
platform supports pseudo-locking - this is currently done when user
requests a pseudo-lock region. rdt_pseudo_lock_init() does
initialization of things that should not fail and for which resctrl's
mount should not proceed if it fails. A platform not supporting
pseudo-locking should not prevent resctrl from being mounted and used
for cache allocation.

Thank you very much

Reinette

Re: [PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower level caches

[Borislav Petkov]

On Tue, Aug 06, 2019 at 09:55:56AM -0700, Reinette Chatre wrote:
> I am a bit cautious about this. When I started this work I initially
> added a helper function to resctrl that calls CPUID to determine if the
> cache is inclusive. At that time I became aware of a discussion
> motivating against scattered CPUID calls and motivating for one instance
> of CPUID information:
> http://lkml.kernel.org/r/alpine.DEB.2.21.1906162141301.1760@nanos.tec.linutronix.de

Ah, there's that. That's still somewhat a work/discussion in progress
thing. Let me discuss it with tglx.

> To answer your question about checking any cache: this seems to be

I meant the CPUID on any CPU and thus any cache - i.e., all L3s on the
system should be inclusive and identical in that respect. Can't work
otherwise, I'd strongly presume.

> different between L2 and L3. On the Atom systems where L2 pseudo-locking
> works well the L2 cache is not inclusive. We are also working on
> supporting cache pseudo-locking on L3 cache that is not inclusive.

Hmm, so why are you enforcing the inclusivity now:

+       if (p->r->cache_level == 3 &&
+           !get_cache_inclusive(plr->cpu, p->r->cache_level)) {
+               rdt_last_cmd_puts("L3 cache not inclusive\n");

but then will remove this requirement in the future? Why are we even
looking at cache inclusivity then and not make pseudo-locking work
regardless of that cache property?

Because if we're going to go and model this cache inclusivity property
properly in struct cpuinfo_x86 or struct cacheinfo or wherever, and do
that for all cache levels because apparently we're going to need that;
but then later it turns out we won't need it after all, why are we even
bothering?

Or am I missing some aspect?

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower level caches

[Reinette Chatre]

Hi Borislav,

On 8/6/2019 10:33 AM, Borislav Petkov wrote:
> On Tue, Aug 06, 2019 at 09:55:56AM -0700, Reinette Chatre wrote:
>> I am a bit cautious about this. When I started this work I initially
>> added a helper function to resctrl that calls CPUID to determine if the
>> cache is inclusive. At that time I became aware of a discussion
>> motivating against scattered CPUID calls and motivating for one instance
>> of CPUID information:
>> http://lkml.kernel.org/r/alpine.DEB.2.21.1906162141301.1760@nanos.tec.linutronix.de
> 
> Ah, there's that. That's still somewhat a work/discussion in progress
> thing. Let me discuss it with tglx.
> 
>> To answer your question about checking any cache: this seems to be
> 
> I meant the CPUID on any CPU and thus any cache - i.e., all L3s on the
> system should be inclusive and identical in that respect. Can't work
> otherwise, I'd strongly presume.

This is my understanding, yes. While this patch supports knowing whether
each L3 is inclusive or not, I expect this information to be the same
for all L3 instances as will be supported by a single query in
rdt_pseudo_lock_init(). This definitely is the case on the platforms we
are enabling in this round.

>> different between L2 and L3. On the Atom systems where L2 pseudo-locking
>> works well the L2 cache is not inclusive. We are also working on
>> supporting cache pseudo-locking on L3 cache that is not inclusive.
> 
> Hmm, so why are you enforcing the inclusivity now:
> 
> +       if (p->r->cache_level == 3 &&
> +           !get_cache_inclusive(plr->cpu, p->r->cache_level)) {
> +               rdt_last_cmd_puts("L3 cache not inclusive\n");
> 
> but then will remove this requirement in the future? Why are we even
> looking at cache inclusivity then and not make pseudo-locking work
> regardless of that cache property?

Some platforms being enabled in this round have SKUs with inclusive
cache and also SKUs with non-inclusive cache. The non-inclusive cache
SKUs do not support cache pseudo-locking and cannot be made to support
cache pseudo-locking with software changes. Needing to know if cache is
inclusive or not will thus remain a requirement to distinguish between
these different SKUs. Supporting cache pseudo-locking on platforms with
non inclusive cache will require new hardware features.
> Because if we're going to go and model this cache inclusivity property
> properly in struct cpuinfo_x86 or struct cacheinfo or wherever, and do
> that for all cache levels because apparently we're going to need that;
> but then later it turns out we won't need it after all, why are we even
> bothering?
> 
> Or am I missing some aspect?

Reinette

Re: [PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower level caches

[Borislav Petkov]

On Tue, Aug 06, 2019 at 11:13:22AM -0700, Reinette Chatre wrote:
> Some platforms being enabled in this round have SKUs with inclusive
> cache and also SKUs with non-inclusive cache. The non-inclusive cache
> SKUs do not support cache pseudo-locking and cannot be made to support
> cache pseudo-locking with software changes. Needing to know if cache is
> inclusive or not will thus remain a requirement to distinguish between
> these different SKUs. Supporting cache pseudo-locking on platforms with
> non inclusive cache will require new hardware features.

Is there another way/CPUID bit or whatever to tell us whether the
platform supports cache pseudo-locking or is the cache inclusivity the
only one?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower level caches

[Reinette Chatre]

Hi Borislav,

On 8/6/2019 11:33 AM, Borislav Petkov wrote:
> On Tue, Aug 06, 2019 at 11:13:22AM -0700, Reinette Chatre wrote:
>> Some platforms being enabled in this round have SKUs with inclusive
>> cache and also SKUs with non-inclusive cache. The non-inclusive cache
>> SKUs do not support cache pseudo-locking and cannot be made to support
>> cache pseudo-locking with software changes. Needing to know if cache is
>> inclusive or not will thus remain a requirement to distinguish between
>> these different SKUs. Supporting cache pseudo-locking on platforms with
>> non inclusive cache will require new hardware features.
> 
> Is there another way/CPUID bit or whatever to tell us whether the
> platform supports cache pseudo-locking or is the cache inclusivity the
> only one?

Unfortunately there are no hardware bits that software can use to
determine if cache pseudo-locking is supported. The way software
currently determines if cache pseudo-locking is supported is through
initialization of the hardware prefetch disable bits. Cache
pseudo-locking requires the hardware prefetch bits to be disabled
(during the locking flow only), those cannot be discovered either and
thus software hardcodes the hardware prefetch disable bits only for
those platforms that support cache pseudo-locking.

What you will see in the code is this:

int rdtgroup_locksetup_enter(struct rdtgroup *rdtgrp)
{
...

        prefetch_disable_bits = get_prefetch_disable_bits();
        if (prefetch_disable_bits == 0) {
                rdt_last_cmd_puts("Pseudo-locking not supported\n");
                return -EINVAL;
        }
...
}


In get_prefetch_disable_bits() the platforms that support cache
pseudo-locking are hardcoded as part of configuring the hardware
prefetch disable bits to use.

The current problem is that an upcoming platform has this difference
between SKUs so a single platform-wide decision is not sufficient.

Reinette

Re: [PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower level caches

[Borislav Petkov]

On Tue, Aug 06, 2019 at 11:53:40AM -0700, Reinette Chatre wrote:
> In get_prefetch_disable_bits() the platforms that support cache
> pseudo-locking are hardcoded as part of configuring the hardware
> prefetch disable bits to use.

Ok, so there is already a way to check pseudo-locking support. Now, why
do we have to look at cache inclusivity too?

Your 0/10 mail says:

"Only systems with L3 inclusive cache is supported at this time because
if the L3 cache is not inclusive then pseudo-locked memory within the L3
cache would be evicted when migrated to L2."

but then a couple of mails earlier you said:

"... this seems to be different between L2 and L3. On the Atom systems
where L2 pseudo-locking works well the L2 cache is not inclusive. We are
also working on supporting cache pseudo-locking on L3 cache that is not
inclusive."

which leads me to still think that we don't really need L3 cache
inclusivity and theoretically, you could do without it.

Or are you saying that cache pseudo-locking on non-inclusive L3 is not
supported yet so no need to enable it yet?

Hmmm?

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower level caches

[Reinette Chatre]

Hi Borislav,

On 8/6/2019 12:16 PM, Borislav Petkov wrote:
> On Tue, Aug 06, 2019 at 11:53:40AM -0700, Reinette Chatre wrote:
>> In get_prefetch_disable_bits() the platforms that support cache
>> pseudo-locking are hardcoded as part of configuring the hardware
>> prefetch disable bits to use.
> 
> Ok, so there is already a way to check pseudo-locking support.

Correct. This check is per platform though.

> Now, why
> do we have to look at cache inclusivity too?

... because some platforms differ in which SKUs support cache
pseudo-locking. On these platforms only the SKUs with inclusive cache
support cache pseudo-locking, thus the additional check.

> 
> Your 0/10 mail says:
> 
> "Only systems with L3 inclusive cache is supported at this time because
> if the L3 cache is not inclusive then pseudo-locked memory within the L3
> cache would be evicted when migrated to L2."
> 
> but then a couple of mails earlier you said:
> 
> "... this seems to be different between L2 and L3. On the Atom systems
> where L2 pseudo-locking works well the L2 cache is not inclusive. We are
> also working on supporting cache pseudo-locking on L3 cache that is not
> inclusive."
> 
> which leads me to still think that we don't really need L3 cache
> inclusivity and theoretically, you could do without it.
> 
> Or are you saying that cache pseudo-locking on non-inclusive L3 is not
> supported yet so no need to enable it yet?

Correct. Hardware and software changes will be needed to support this.

Reinette

Re: [PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower level caches

[Borislav Petkov]

On Tue, Aug 06, 2019 at 01:22:22PM -0700, Reinette Chatre wrote:
> ... because some platforms differ in which SKUs support cache
> pseudo-locking. On these platforms only the SKUs with inclusive cache
> support cache pseudo-locking, thus the additional check.

Ok, so it sounds to me like that check in get_prefetch_disable_bits()
should be extended (and maybe renamed) to check for cache inclusivity
too, in order to know which platforms support cache pseudo-locking.
I'd leave it to tglx to say how we should mirror cache inclusivity in
cpuinfo_x86: whether a synthetic X86_FEATURE bit or cache the respective
CPUID words which state whether L2/L3 is inclusive...

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower level caches

[Reinette Chatre]

Hi Borislav,

On 8/6/2019 1:40 PM, Borislav Petkov wrote:
> On Tue, Aug 06, 2019 at 01:22:22PM -0700, Reinette Chatre wrote:
>> ... because some platforms differ in which SKUs support cache
>> pseudo-locking. On these platforms only the SKUs with inclusive cache
>> support cache pseudo-locking, thus the additional check.
> 
> Ok, so it sounds to me like that check in get_prefetch_disable_bits()
> should be extended (and maybe renamed) to check for cache inclusivity
> too, in order to know which platforms support cache pseudo-locking.

Indeed. As you pointed out this would be same system-wide and the check
thus need not be delayed until it is known which cache is being
pseudo-locked.

> I'd leave it to tglx to say how we should mirror cache inclusivity in
> cpuinfo_x86: whether a synthetic X86_FEATURE bit or cache the respective
> CPUID words which state whether L2/L3 is inclusive...

Thank you very much. I appreciate your guidance here.

Reinette

Re: [PATCH V2 08/10] x86/resctrl: Support pseudo-lock regions spanning resources

[Borislav Petkov]

On Tue, Jul 30, 2019 at 10:29:42AM -0700, Reinette Chatre wrote:
> Currently cache pseudo-locked regions only consider one cache level but
> cache pseudo-locked regions may span multiple cache levels.
> 
> In preparation for support of pseudo-locked regions spanning multiple
> cache levels pseudo-lock 'portions' are introduced. A 'portion' of a
> pseudo-locked region is the portion of a pseudo-locked region that
> belongs to a specific resource. Each pseudo-locked portion is identified
> with the resource (for example, L2 or L3 cache), the domain (the
> specific cache instance), and the capacity bitmask that specifies which
> region of the cache is used by the pseudo-locked region.
> 
> In support of pseudo-locked regions spanning multiple cache levels a
> pseudo-locked region could have multiple 'portions' but in this
> introduction only single portions are allowed.
> 
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
>  arch/x86/kernel/cpu/resctrl/ctrlmondata.c |  26 +++-
>  arch/x86/kernel/cpu/resctrl/internal.h    |  32 ++--
>  arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 180 ++++++++++++++++------
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c    |  44 ++++--
>  4 files changed, 211 insertions(+), 71 deletions(-)

This patch kinda got pretty big and is hard to review. Can
you split it pls? The addition of pseudo_lock_portion and
pseudo_lock_single_portion_valid() look like a separate patch to me.

> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index a0383ff80afe..a60fb38a4d20 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -207,7 +207,7 @@ int parse_cbm(struct rdt_parse_data *data, struct rdt_resource *r,
>  	 * hierarchy.
>  	 */
>  	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP &&
> -	    rdtgroup_pseudo_locked_in_hierarchy(d)) {
> +	    rdtgroup_pseudo_locked_in_hierarchy(rdtgrp, d)) {
>  		rdt_last_cmd_puts("Pseudo-locked region in hierarchy\n");
>  		return -EINVAL;
>  	}
> @@ -282,6 +282,7 @@ static int parse_line(char *line, struct rdt_resource *r,
>  			if (r->parse_ctrlval(&data, r, d))
>  				return -EINVAL;
>  			if (rdtgrp->mode ==  RDT_MODE_PSEUDO_LOCKSETUP) {
> +				struct pseudo_lock_portion *p;
>  				/*
>  				 * In pseudo-locking setup mode and just
>  				 * parsed a valid CBM that should be
> @@ -290,9 +291,15 @@ static int parse_line(char *line, struct rdt_resource *r,
>  				 * the required initialization for single
>  				 * region and return.
>  				 */
> -				rdtgrp->plr->r = r;
> -				rdtgrp->plr->d_id = d->id;
> -				rdtgrp->plr->cbm = d->new_ctrl;
> +				p = kzalloc(sizeof(*p), GFP_KERNEL);
> +				if (!p) {
> +					rdt_last_cmd_puts("Unable to allocate memory for pseudo-lock portion\n");
> +					return -ENOMEM;
> +				}
> +				p->r = r;
> +				p->d_id = d->id;
> +				p->cbm = d->new_ctrl;
> +				list_add(&p->list, &rdtgrp->plr->portions);
>  				return 0;
>  			}

Looking at the indentation level of this, it is basically begging to
become a separate, helper function...

>  			goto next;
> @@ -410,8 +417,11 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
>  			goto out;
>  		}
>  		ret = rdtgroup_parse_resource(resname, tok, rdtgrp);
> -		if (ret)
> +		if (ret) {
> +			if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP)
> +				pseudo_lock_region_clear(rdtgrp->plr);
>  			goto out;
> +		}
>  	}
>  
>  	for_each_alloc_enabled_rdt_resource(r) {
> @@ -459,6 +469,7 @@ static void show_doms(struct seq_file *s, struct rdt_resource *r, int closid)
>  int rdtgroup_schemata_show(struct kernfs_open_file *of,
>  			   struct seq_file *s, void *v)
>  {
> +	struct pseudo_lock_portion *p;
>  	struct rdtgroup *rdtgrp;
>  	struct rdt_resource *r;
>  	int ret = 0;
> @@ -470,8 +481,9 @@ int rdtgroup_schemata_show(struct kernfs_open_file *of,
>  			for_each_alloc_enabled_rdt_resource(r)
>  				seq_printf(s, "%s:uninitialized\n", r->name);
>  		} else if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED) {
> -			seq_printf(s, "%s:%d=%x\n", rdtgrp->plr->r->name,
> -				   rdtgrp->plr->d_id, rdtgrp->plr->cbm);
> +			list_for_each_entry(p, &rdtgrp->plr->portions, list)
> +				seq_printf(s, "%s:%d=%x\n", p->r->name, p->d_id,
> +					   p->cbm);

Shouldn't this say that those are portions now?

>  		} else {
>  			closid = rdtgrp->closid;
>  			for_each_alloc_enabled_rdt_resource(r) {
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 892f38899dda..b041029d4de1 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -145,13 +145,27 @@ struct mongroup {
>  	u32			rmid;
>  };
>  
> +/**
> + * struct pseudo_lock_portion - portion of a pseudo-lock region on one resource
> + * @r:		RDT resource to which this pseudo-locked portion
> + *		belongs
> + * @d_id:	ID of cache instance to which this pseudo-locked portion
> + *		belongs
> + * @cbm:	bitmask of the pseudo-locked portion
> + * @list:	Entry in the list of pseudo-locked portion
> + *		belonging to the pseudo-locked region
> + */
> +struct pseudo_lock_portion {
> +	struct rdt_resource	*r;
> +	int			d_id;
> +	u32			cbm;
> +	struct list_head	list;
> +};
> +
>  /**
>   * struct pseudo_lock_region - pseudo-lock region information
> - * @r:			RDT resource to which this pseudo-locked region
> - *			belongs
> - * @d_id:		ID of cache instance to which this pseudo-locked region
> - *			belongs
> - * @cbm:		bitmask of the pseudo-locked region
> + * @portions:		list of portions across different resources that
> + *			are associated with this pseudo-locked region
>   * @lock_thread_wq:	waitqueue used to wait on the pseudo-locking thread
>   *			completion
>   * @thread_done:	variable used by waitqueue to test if pseudo-locking
> @@ -168,9 +182,7 @@ struct mongroup {
>   * @pm_reqs:		Power management QoS requests related to this region
>   */
>  struct pseudo_lock_region {
> -	struct rdt_resource	*r;
> -	int			d_id;
> -	u32			cbm;
> +	struct list_head	portions;
>  	wait_queue_head_t	lock_thread_wq;
>  	int			thread_done;
>  	int			cpu;
> @@ -569,11 +581,13 @@ bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_resource *r,
>  					 struct rdt_domain *d,
>  					 unsigned long cbm);
>  u32 rdtgroup_pseudo_locked_bits(struct rdt_resource *r, struct rdt_domain *d);
> -bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d);
> +bool rdtgroup_pseudo_locked_in_hierarchy(struct rdtgroup *selfgrp,
> +					 struct rdt_domain *d);
>  int rdt_pseudo_lock_init(void);
>  void rdt_pseudo_lock_release(void);
>  int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp);
>  void rdtgroup_pseudo_lock_remove(struct rdtgroup *rdtgrp);
> +void pseudo_lock_region_clear(struct pseudo_lock_region *plr);
>  struct rdt_domain *get_domain_from_cpu(int cpu, struct rdt_resource *r);
>  int update_domains(struct rdt_resource *r, int closid);
>  int closids_supported(void);
> diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> index 733cb7f34948..717ea26e325b 100644
> --- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> +++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> @@ -270,28 +270,85 @@ static int pseudo_lock_cstates_constrain(struct pseudo_lock_region *plr,
>   *
>   * Return: void
>   */
> -static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
> +void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
>  {
> +	struct pseudo_lock_portion *p, *tmp;
> +
>  	plr->size = 0;
>  	plr->line_size = 0;
>  	kfree(plr->kmem);
>  	plr->kmem = NULL;
> -	plr->r = NULL;
> -	plr->d_id = -1;
> -	plr->cbm = 0;
>  	pseudo_lock_cstates_relax(plr);
> +	if (!list_empty(&plr->portions)) {
> +		list_for_each_entry_safe(p, tmp, &plr->portions, list) {
> +			list_del(&p->list);
> +			kfree(p);
> +		}
> +	}
>  	plr->debugfs_dir = NULL;
>  }
>  
> +/**
> + * pseudo_lock_single_portion_valid - Verify properties of pseudo-lock region
> + * @plr: the main pseudo-lock region
> + * @p: the single portion that makes up the pseudo-locked region
> + *
> + * Verify and initialize properties of the pseudo-locked region.
> + *
> + * Return: -1 if portion of cache unable to be used for pseudo-locking
> + *         0 if portion of cache can be used for pseudo-locking, in
> + *         addition the CPU on which pseudo-locking will be performed will
> + *         be initialized as well as the size and cache line size of the region
> + */
> +static int pseudo_lock_single_portion_valid(struct pseudo_lock_region *plr,
> +					    struct pseudo_lock_portion *p)
> +{
> +	struct rdt_domain *d;
> +
> +	d = rdt_find_domain(p->r, p->d_id, NULL);
> +	if (IS_ERR_OR_NULL(d)) {
> +		rdt_last_cmd_puts("Cannot find cache domain\n");
> +		return -1;
> +	}
> +
> +	plr->cpu = cpumask_first(&d->cpu_mask);
> +	if (!cpu_online(plr->cpu)) {
> +		rdt_last_cmd_printf("CPU %u not online\n", plr->cpu);
> +		goto err_cpu;
> +	}
> +
> +	plr->line_size = get_cache_line_size(plr->cpu, p->r->cache_level);
> +	if (plr->line_size == 0) {
> +		rdt_last_cmd_puts("Unable to compute cache line length\n");
> +		goto err_cpu;
> +	}
> +
> +	if (pseudo_lock_cstates_constrain(plr, &d->cpu_mask)) {
> +		rdt_last_cmd_puts("Cannot limit C-states\n");
> +		goto err_line;
> +	}
> +
> +	plr->size = rdtgroup_cbm_to_size(p->r, d, p->cbm);
> +
> +	return 0;
> +
> +err_line:
> +	plr->line_size = 0;
> +err_cpu:
> +	plr->cpu = 0;
> +	return -1;
> +}
> +
>  /**
>   * pseudo_lock_region_init - Initialize pseudo-lock region information
>   * @plr: pseudo-lock region
>   *
>   * Called after user provided a schemata to be pseudo-locked. From the
>   * schemata the &struct pseudo_lock_region is on entry already initialized
> - * with the resource, domain, and capacity bitmask. Here the information
> - * required for pseudo-locking is deduced from this data and &struct
> - * pseudo_lock_region initialized further. This information includes:
> + * with the resource, domain, and capacity bitmask. Here the
> + * provided data is validated and information required for pseudo-locking
> + * deduced, and &struct pseudo_lock_region initialized further. This
> + * information includes:
>   * - size in bytes of the region to be pseudo-locked
>   * - cache line size to know the stride with which data needs to be accessed
>   *   to be pseudo-locked
> @@ -303,44 +360,50 @@ static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
>   */
>  static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
>  {
> -	struct rdt_domain *d;
> +	struct rdt_resource *l3_resource = &rdt_resources_all[RDT_RESOURCE_L3];
> +	struct pseudo_lock_portion *p;
>  	int ret;
>  
> -	/* Pick the first cpu we find that is associated with the cache. */
> -	d = rdt_find_domain(plr->r, plr->d_id, NULL);
> -	if (IS_ERR_OR_NULL(d)) {
> -		rdt_last_cmd_puts("Cache domain offline\n");
> -		ret = -ENODEV;
> +	if (list_empty(&plr->portions)) {
> +		rdt_last_cmd_puts("No pseudo-lock portions provided\n");
>  		goto out_region;

Not return?

Do you need to clear anything in an not even initialized plr?

>  	}
>  
> -	plr->cpu = cpumask_first(&d->cpu_mask);
> -
> -	if (!cpu_online(plr->cpu)) {
> -		rdt_last_cmd_printf("CPU %u associated with cache not online\n",
> -				    plr->cpu);
> -		ret = -ENODEV;
> -		goto out_region;
> +	/* Cache Pseudo-Locking only supported on L2 and L3 resources */
> +	list_for_each_entry(p, &plr->portions, list) {
> +		if (p->r->rid != RDT_RESOURCE_L2 &&
> +		    p->r->rid != RDT_RESOURCE_L3) {
> +			rdt_last_cmd_puts("Unsupported resource\n");
> +			goto out_region;
> +		}
>  	}
>  
> -	plr->line_size = get_cache_line_size(plr->cpu, plr->r->cache_level);
> -	if (plr->line_size == 0) {
> -		rdt_last_cmd_puts("Unable to determine cache line size\n");
> -		ret = -1;
> -		goto out_region;
> +	/*
> +	 * If only one resource requested to be pseudo-locked then:
> +	 * - Just a L3 cache portion is valid
> +	 * - Just a L2 cache portion on system without L3 cache is valid
> +	 */
> +	if (list_is_singular(&plr->portions)) {
> +		p = list_first_entry(&plr->portions, struct pseudo_lock_portion,
> +				     list);

Let that line stick out.

> +		if (p->r->rid == RDT_RESOURCE_L3 ||
> +		    (p->r->rid == RDT_RESOURCE_L2 &&
> +		     !l3_resource->alloc_capable)) {
> +			ret = pseudo_lock_single_portion_valid(plr, p);
> +			if (ret < 0)
> +				goto out_region;
> +			return 0;
> +		} else {
> +			rdt_last_cmd_puts("Invalid resource or just L2 provided when L3 is required\n");
> +			goto out_region;
> +		}
> +	} else {
> +		rdt_last_cmd_puts("Multiple pseudo-lock portions unsupported\n");
>  	}
>  
> -	plr->size = rdtgroup_cbm_to_size(plr->r, d, plr->cbm);
> -
> -	ret = pseudo_lock_cstates_constrain(plr, &d->cpu_mask);
> -	if (ret < 0)
> -		goto out_region;
> -
> -	return 0;
> -
>  out_region:
>  	pseudo_lock_region_clear(plr);
> -	return ret;
> +	return -1;
>  }
>

Yap, this patch is doing too many things at once and splitting it would help.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH V2 09/10] x86/resctrl: Pseudo-lock portions of multiple resources

[Borislav Petkov]

On Tue, Jul 30, 2019 at 10:29:43AM -0700, Reinette Chatre wrote:
> A cache pseudo-locked region may span more than one level of cache. A
> part of the pseudo-locked region that falls on one cache level is
> referred to as a pseudo-lock portion that was introduced previously.
> 
> Now a pseudo-locked region is allowed to have two portions instead of
> the previous limit of one. When a pseudo-locked region consists out of
> two portions it can only span a L2 and L3 resctrl resource.
> When a pseudo-locked region consists out of a L2 and L3 portion then
> there are some requirements:
> - the L2 and L3 cache has to be in same cache hierarchy
> - the L3 portion must be same size or larger than L2 portion
> 
> As documented in previous changes the list of portions are
> maintained so that the L2 portion would always appear first in the list
> to simplify any information retrieval.
> 
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
>  arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 142 +++++++++++++++++++++-
>  1 file changed, 139 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> index 717ea26e325b..7ab4e85a33a7 100644
> --- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> +++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> @@ -339,13 +339,104 @@ static int pseudo_lock_single_portion_valid(struct pseudo_lock_region *plr,
>  	return -1;
>  }
>  
> +/**
> + * pseudo_lock_l2_l3_portions_valid - Verify region across L2 and L3
> + * @plr: Pseudo-Locked region
> + * @l2_portion: L2 Cache portion of pseudo-locked region
> + * @l3_portion: L3 Cache portion of pseudo-locked region
> + *
> + * User requested a pseudo-locked region consisting of a L2 as well as L3
> + * cache portion. The portions are tested as follows:
> + *   - L2 and L3 cache instances have to be in the same cache hierarchy.
> + *     This is tested by ensuring that the L2 portion's cpumask is a
> + *     subset of the L3 portion's cpumask.
> + *   - L3 portion must be same size or larger than L2 portion.
> + *
> + * Return: -1 if the portions are unable to be used for a pseudo-locked
> + *         region, 0 if the portions could be used for a pseudo-locked
> + *         region. When returning 0:
> + *         - the pseudo-locked region's size, line_size (cache line length)
> + *           and CPU on which locking thread will be run are set.
> + *         - CPUs associated with L2 cache portion are constrained from
> + *           entering C-state that will affect the pseudo-locked region.
> + */
> +static int pseudo_lock_l2_l3_portions_valid(struct pseudo_lock_region *plr,
> +					    struct pseudo_lock_portion *l2_p,
> +					    struct pseudo_lock_portion *l3_p)
> +{
> +	struct rdt_domain *l2_d, *l3_d;
> +	unsigned int l2_size, l3_size;
> +
> +	l2_d = rdt_find_domain(l2_p->r, l2_p->d_id, NULL);
> +	if (IS_ERR_OR_NULL(l2_d)) {
> +		rdt_last_cmd_puts("Cannot locate L2 cache domain\n");
> +		return -1;
> +	}
> +
> +	l3_d = rdt_find_domain(l3_p->r, l3_p->d_id, NULL);
> +	if (IS_ERR_OR_NULL(l3_d)) {
> +		rdt_last_cmd_puts("Cannot locate L3 cache domain\n");
> +		return -1;
> +	}
> +
> +	if (!cpumask_subset(&l2_d->cpu_mask, &l3_d->cpu_mask)) {
> +		rdt_last_cmd_puts("L2 and L3 caches need to be in same hierarchy\n");
> +		return -1;
> +	}
> +

Put that sentence above about L2 CPUs being constrained here - it is
easier when following the code.

> +	if (pseudo_lock_cstates_constrain(plr, &l2_d->cpu_mask)) {
> +		rdt_last_cmd_puts("Cannot limit C-states\n");
> +		return -1;
> +	}

Also, can that function call be last in this function so that you can
save yourself all the goto labels?

> +
> +	l2_size = rdtgroup_cbm_to_size(l2_p->r, l2_d, l2_p->cbm);
> +	l3_size = rdtgroup_cbm_to_size(l3_p->r, l3_d, l3_p->cbm);
> +
> +	if (l2_size > l3_size) {
> +		rdt_last_cmd_puts("L3 cache portion has to be same size or larger than L2 cache portion\n");
> +		goto err_size;
> +	}
> +
> +	plr->size = l2_size;
> +
> +	l2_size = get_cache_line_size(cpumask_first(&l2_d->cpu_mask),
> +				      l2_p->r->cache_level);
> +	l3_size = get_cache_line_size(cpumask_first(&l3_d->cpu_mask),
> +				      l3_p->r->cache_level);
> +	if (l2_size != l3_size) {
> +		rdt_last_cmd_puts("L2 and L3 caches have different coherency cache line sizes\n");
> +		goto err_line;
> +	}
> +
> +	plr->line_size = l2_size;
> +
> +	plr->cpu = cpumask_first(&l2_d->cpu_mask);
> +
> +	if (!cpu_online(plr->cpu)) {
> +		rdt_last_cmd_printf("CPU %u associated with cache not online\n",
> +				    plr->cpu);
> +		goto err_cpu;
> +	}
> +
> +	return 0;
> +
> +err_cpu:
> +	plr->line_size = 0;
> +	plr->cpu = 0;
> +err_line:
> +	plr->size = 0;
> +err_size:
> +	pseudo_lock_cstates_relax(plr);
> +	return -1;
> +}
> +
>  /**
>   * pseudo_lock_region_init - Initialize pseudo-lock region information
>   * @plr: pseudo-lock region
>   *
>   * Called after user provided a schemata to be pseudo-locked. From the
>   * schemata the &struct pseudo_lock_region is on entry already initialized
> - * with the resource, domain, and capacity bitmask. Here the
> + * with the resource(s), domain(s), and capacity bitmask(s). Here the
>   * provided data is validated and information required for pseudo-locking
>   * deduced, and &struct pseudo_lock_region initialized further. This
>   * information includes:
> @@ -355,13 +446,24 @@ static int pseudo_lock_single_portion_valid(struct pseudo_lock_region *plr,
>   * - a cpu associated with the cache instance on which the pseudo-locking
>   *   flow can be executed
>   *
> + * A user provides a schemata for a pseudo-locked region. This schemata may
> + * contain portions that span different resources, for example, a cache
> + * pseudo-locked region that spans L2 and L3 cache. After the schemata is
> + * parsed into portions it needs to be verified that the provided portions
> + * are valid with the following tests:
> + *
> + * - L2 only portion on system that has only L2 resource - OK
> + * - L3 only portion on any system that supports it - OK
> + * - L2 portion on system that has L3 resource - require L3 portion
> + **
> + *
>   * Return: 0 on success, <0 on failure. Descriptive error will be written
>   * to last_cmd_status buffer.
>   */
>  static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
>  {
>  	struct rdt_resource *l3_resource = &rdt_resources_all[RDT_RESOURCE_L3];
> -	struct pseudo_lock_portion *p;
> +	struct pseudo_lock_portion *p, *n_p, *tmp;
>  	int ret;
>  
>  	if (list_empty(&plr->portions)) {
> @@ -397,8 +499,42 @@ static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
>  			rdt_last_cmd_puts("Invalid resource or just L2 provided when L3 is required\n");
>  			goto out_region;
>  		}
> +	}
> +
> +	/*
> +	 * List is neither empty nor singular, process first and second portions
> +	 */
> +	p = list_first_entry(&plr->portions, struct pseudo_lock_portion, list);
> +	n_p = list_next_entry(p, list);
> +
> +	/*
> +	 * If the second portion is not also the last portion user provided
> +	 * more portions than can be supported.
> +	 */
> +	tmp = list_last_entry(&plr->portions, struct pseudo_lock_portion, list);
> +	if (n_p != tmp) {
> +		rdt_last_cmd_puts("Only two pseudo-lock portions supported\n");
> +		goto out_region;
> +	}
> +
> +	if (p->r->rid == RDT_RESOURCE_L2 && n_p->r->rid == RDT_RESOURCE_L3) {
> +		ret = pseudo_lock_l2_l3_portions_valid(plr, p, n_p);
> +		if (ret < 0)
> +			goto out_region;
> +		return 0;
> +	} else if (p->r->rid == RDT_RESOURCE_L3 &&
> +		   n_p->r->rid == RDT_RESOURCE_L2) {
> +		if (pseudo_lock_l2_l3_portions_valid(plr, n_p, p) == 0) {

		if (!pseudo_...

> +			/*
> +			 * Let L2 and L3 portions appear in order in the
> +			 * portions list in support of consistent output to
> +			 * user space.
> +			 */
> +			list_rotate_left(&plr->portions);
> +			return 0;
> +		}
>  	} else {
> -		rdt_last_cmd_puts("Multiple pseudo-lock portions unsupported\n");
> +		rdt_last_cmd_puts("Invalid combination of resources\n");
>  	}
>  
>  out_region:
> -- 
> 2.17.2
> 

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH V2 08/10] x86/resctrl: Support pseudo-lock regions spanning resources

[Reinette Chatre]

Hi Borislav,

On 8/7/2019 2:18 AM, Borislav Petkov wrote:
> On Tue, Jul 30, 2019 at 10:29:42AM -0700, Reinette Chatre wrote:
>> Currently cache pseudo-locked regions only consider one cache level but
>> cache pseudo-locked regions may span multiple cache levels.
>>
>> In preparation for support of pseudo-locked regions spanning multiple
>> cache levels pseudo-lock 'portions' are introduced. A 'portion' of a
>> pseudo-locked region is the portion of a pseudo-locked region that
>> belongs to a specific resource. Each pseudo-locked portion is identified
>> with the resource (for example, L2 or L3 cache), the domain (the
>> specific cache instance), and the capacity bitmask that specifies which
>> region of the cache is used by the pseudo-locked region.
>>
>> In support of pseudo-locked regions spanning multiple cache levels a
>> pseudo-locked region could have multiple 'portions' but in this
>> introduction only single portions are allowed.
>>

(Will fix to active voice)

>> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
>> ---
>>  arch/x86/kernel/cpu/resctrl/ctrlmondata.c |  26 +++-
>>  arch/x86/kernel/cpu/resctrl/internal.h    |  32 ++--
>>  arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 180 ++++++++++++++++------
>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c    |  44 ++++--
>>  4 files changed, 211 insertions(+), 71 deletions(-)
> 
> This patch kinda got pretty big and is hard to review. Can
> you split it pls? The addition of pseudo_lock_portion and
> pseudo_lock_single_portion_valid() look like a separate patch to me.

Sorry about the hard review. I'll split it up.

> 
>> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> index a0383ff80afe..a60fb38a4d20 100644
>> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> @@ -207,7 +207,7 @@ int parse_cbm(struct rdt_parse_data *data, struct rdt_resource *r,
>>  	 * hierarchy.
>>  	 */
>>  	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP &&
>> -	    rdtgroup_pseudo_locked_in_hierarchy(d)) {
>> +	    rdtgroup_pseudo_locked_in_hierarchy(rdtgrp, d)) {
>>  		rdt_last_cmd_puts("Pseudo-locked region in hierarchy\n");
>>  		return -EINVAL;
>>  	}
>> @@ -282,6 +282,7 @@ static int parse_line(char *line, struct rdt_resource *r,
>>  			if (r->parse_ctrlval(&data, r, d))
>>  				return -EINVAL;
>>  			if (rdtgrp->mode ==  RDT_MODE_PSEUDO_LOCKSETUP) {
>> +				struct pseudo_lock_portion *p;
>>  				/*
>>  				 * In pseudo-locking setup mode and just
>>  				 * parsed a valid CBM that should be
>> @@ -290,9 +291,15 @@ static int parse_line(char *line, struct rdt_resource *r,
>>  				 * the required initialization for single
>>  				 * region and return.
>>  				 */
>> -				rdtgrp->plr->r = r;
>> -				rdtgrp->plr->d_id = d->id;
>> -				rdtgrp->plr->cbm = d->new_ctrl;
>> +				p = kzalloc(sizeof(*p), GFP_KERNEL);
>> +				if (!p) {
>> +					rdt_last_cmd_puts("Unable to allocate memory for pseudo-lock portion\n");
>> +					return -ENOMEM;
>> +				}
>> +				p->r = r;
>> +				p->d_id = d->id;
>> +				p->cbm = d->new_ctrl;
>> +				list_add(&p->list, &rdtgrp->plr->portions);
>>  				return 0;
>>  			}
> 
> Looking at the indentation level of this, it is basically begging to
> become a separate, helper function...

Will do.

> 
>>  			goto next;
>> @@ -410,8 +417,11 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
>>  			goto out;
>>  		}
>>  		ret = rdtgroup_parse_resource(resname, tok, rdtgrp);
>> -		if (ret)
>> +		if (ret) {
>> +			if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP)
>> +				pseudo_lock_region_clear(rdtgrp->plr);
>>  			goto out;
>> +		}
>>  	}
>>  
>>  	for_each_alloc_enabled_rdt_resource(r) {
>> @@ -459,6 +469,7 @@ static void show_doms(struct seq_file *s, struct rdt_resource *r, int closid)
>>  int rdtgroup_schemata_show(struct kernfs_open_file *of,
>>  			   struct seq_file *s, void *v)
>>  {
>> +	struct pseudo_lock_portion *p;
>>  	struct rdtgroup *rdtgrp;
>>  	struct rdt_resource *r;
>>  	int ret = 0;
>> @@ -470,8 +481,9 @@ int rdtgroup_schemata_show(struct kernfs_open_file *of,
>>  			for_each_alloc_enabled_rdt_resource(r)
>>  				seq_printf(s, "%s:uninitialized\n", r->name);
>>  		} else if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED) {
>> -			seq_printf(s, "%s:%d=%x\n", rdtgrp->plr->r->name,
>> -				   rdtgrp->plr->d_id, rdtgrp->plr->cbm);
>> +			list_for_each_entry(p, &rdtgrp->plr->portions, list)
>> +				seq_printf(s, "%s:%d=%x\n", p->r->name, p->d_id,
>> +					   p->cbm);
> 
> Shouldn't this say that those are portions now?

Are you suggesting new user output text? This is not needed. With this
change pseudo-locked resource groups can span more than one resource
(previously limited either L2 or L3). This change thus brings
pseudo-locked resource groups closer to regular resource groups that can
have allocations from multiple resources (and is supported by the
schemata file). A display of a non pseudo-lock resource group includes
all the resources supported, now the display of a pseudo-lock resource
group can include more than one resource similar to non pseudo-lock
resource groups.

Perhaps the naming ("portions") is causing confusing? Each "portion" is
the allocation of a resource that forms part of the pseudo-locked region.

> 
>>  		} else {
>>  			closid = rdtgrp->closid;
>>  			for_each_alloc_enabled_rdt_resource(r) {
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 892f38899dda..b041029d4de1 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -145,13 +145,27 @@ struct mongroup {
>>  	u32			rmid;
>>  };
>>  
>> +/**
>> + * struct pseudo_lock_portion - portion of a pseudo-lock region on one resource
>> + * @r:		RDT resource to which this pseudo-locked portion
>> + *		belongs
>> + * @d_id:	ID of cache instance to which this pseudo-locked portion
>> + *		belongs
>> + * @cbm:	bitmask of the pseudo-locked portion
>> + * @list:	Entry in the list of pseudo-locked portion
>> + *		belonging to the pseudo-locked region
>> + */
>> +struct pseudo_lock_portion {
>> +	struct rdt_resource	*r;
>> +	int			d_id;
>> +	u32			cbm;
>> +	struct list_head	list;
>> +};
>> +
>>  /**
>>   * struct pseudo_lock_region - pseudo-lock region information
>> - * @r:			RDT resource to which this pseudo-locked region
>> - *			belongs
>> - * @d_id:		ID of cache instance to which this pseudo-locked region
>> - *			belongs
>> - * @cbm:		bitmask of the pseudo-locked region
>> + * @portions:		list of portions across different resources that
>> + *			are associated with this pseudo-locked region
>>   * @lock_thread_wq:	waitqueue used to wait on the pseudo-locking thread
>>   *			completion
>>   * @thread_done:	variable used by waitqueue to test if pseudo-locking
>> @@ -168,9 +182,7 @@ struct mongroup {
>>   * @pm_reqs:		Power management QoS requests related to this region
>>   */
>>  struct pseudo_lock_region {
>> -	struct rdt_resource	*r;
>> -	int			d_id;
>> -	u32			cbm;
>> +	struct list_head	portions;
>>  	wait_queue_head_t	lock_thread_wq;
>>  	int			thread_done;
>>  	int			cpu;
>> @@ -569,11 +581,13 @@ bool rdtgroup_cbm_overlaps_pseudo_locked(struct rdt_resource *r,
>>  					 struct rdt_domain *d,
>>  					 unsigned long cbm);
>>  u32 rdtgroup_pseudo_locked_bits(struct rdt_resource *r, struct rdt_domain *d);
>> -bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d);
>> +bool rdtgroup_pseudo_locked_in_hierarchy(struct rdtgroup *selfgrp,
>> +					 struct rdt_domain *d);
>>  int rdt_pseudo_lock_init(void);
>>  void rdt_pseudo_lock_release(void);
>>  int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp);
>>  void rdtgroup_pseudo_lock_remove(struct rdtgroup *rdtgrp);
>> +void pseudo_lock_region_clear(struct pseudo_lock_region *plr);
>>  struct rdt_domain *get_domain_from_cpu(int cpu, struct rdt_resource *r);
>>  int update_domains(struct rdt_resource *r, int closid);
>>  int closids_supported(void);
>> diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
>> index 733cb7f34948..717ea26e325b 100644
>> --- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
>> +++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
>> @@ -270,28 +270,85 @@ static int pseudo_lock_cstates_constrain(struct pseudo_lock_region *plr,
>>   *
>>   * Return: void
>>   */
>> -static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
>> +void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
>>  {
>> +	struct pseudo_lock_portion *p, *tmp;
>> +
>>  	plr->size = 0;
>>  	plr->line_size = 0;
>>  	kfree(plr->kmem);
>>  	plr->kmem = NULL;
>> -	plr->r = NULL;
>> -	plr->d_id = -1;
>> -	plr->cbm = 0;
>>  	pseudo_lock_cstates_relax(plr);
>> +	if (!list_empty(&plr->portions)) {
>> +		list_for_each_entry_safe(p, tmp, &plr->portions, list) {
>> +			list_del(&p->list);
>> +			kfree(p);
>> +		}
>> +	}
>>  	plr->debugfs_dir = NULL;
>>  }
>>  
>> +/**
>> + * pseudo_lock_single_portion_valid - Verify properties of pseudo-lock region
>> + * @plr: the main pseudo-lock region
>> + * @p: the single portion that makes up the pseudo-locked region
>> + *
>> + * Verify and initialize properties of the pseudo-locked region.
>> + *
>> + * Return: -1 if portion of cache unable to be used for pseudo-locking
>> + *         0 if portion of cache can be used for pseudo-locking, in
>> + *         addition the CPU on which pseudo-locking will be performed will
>> + *         be initialized as well as the size and cache line size of the region
>> + */
>> +static int pseudo_lock_single_portion_valid(struct pseudo_lock_region *plr,
>> +					    struct pseudo_lock_portion *p)
>> +{
>> +	struct rdt_domain *d;
>> +
>> +	d = rdt_find_domain(p->r, p->d_id, NULL);
>> +	if (IS_ERR_OR_NULL(d)) {
>> +		rdt_last_cmd_puts("Cannot find cache domain\n");
>> +		return -1;
>> +	}
>> +
>> +	plr->cpu = cpumask_first(&d->cpu_mask);
>> +	if (!cpu_online(plr->cpu)) {
>> +		rdt_last_cmd_printf("CPU %u not online\n", plr->cpu);
>> +		goto err_cpu;
>> +	}
>> +
>> +	plr->line_size = get_cache_line_size(plr->cpu, p->r->cache_level);
>> +	if (plr->line_size == 0) {
>> +		rdt_last_cmd_puts("Unable to compute cache line length\n");
>> +		goto err_cpu;
>> +	}
>> +
>> +	if (pseudo_lock_cstates_constrain(plr, &d->cpu_mask)) {
>> +		rdt_last_cmd_puts("Cannot limit C-states\n");
>> +		goto err_line;
>> +	}
>> +
>> +	plr->size = rdtgroup_cbm_to_size(p->r, d, p->cbm);
>> +
>> +	return 0;
>> +
>> +err_line:
>> +	plr->line_size = 0;
>> +err_cpu:
>> +	plr->cpu = 0;
>> +	return -1;
>> +}
>> +
>>  /**
>>   * pseudo_lock_region_init - Initialize pseudo-lock region information
>>   * @plr: pseudo-lock region
>>   *
>>   * Called after user provided a schemata to be pseudo-locked. From the
>>   * schemata the &struct pseudo_lock_region is on entry already initialized
>> - * with the resource, domain, and capacity bitmask. Here the information
>> - * required for pseudo-locking is deduced from this data and &struct
>> - * pseudo_lock_region initialized further. This information includes:
>> + * with the resource, domain, and capacity bitmask. Here the
>> + * provided data is validated and information required for pseudo-locking
>> + * deduced, and &struct pseudo_lock_region initialized further. This
>> + * information includes:
>>   * - size in bytes of the region to be pseudo-locked
>>   * - cache line size to know the stride with which data needs to be accessed
>>   *   to be pseudo-locked
>> @@ -303,44 +360,50 @@ static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
>>   */
>>  static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
>>  {
>> -	struct rdt_domain *d;
>> +	struct rdt_resource *l3_resource = &rdt_resources_all[RDT_RESOURCE_L3];
>> +	struct pseudo_lock_portion *p;
>>  	int ret;
>>  
>> -	/* Pick the first cpu we find that is associated with the cache. */
>> -	d = rdt_find_domain(plr->r, plr->d_id, NULL);
>> -	if (IS_ERR_OR_NULL(d)) {
>> -		rdt_last_cmd_puts("Cache domain offline\n");
>> -		ret = -ENODEV;
>> +	if (list_empty(&plr->portions)) {
>> +		rdt_last_cmd_puts("No pseudo-lock portions provided\n");
>>  		goto out_region;
> 
> Not return?
> 
> Do you need to clear anything in an not even initialized plr?

At this point the plr has been initialized with the parameters of the
region requested by the user within rdtgroup_schemata_write() - at this
time the call that triggered this function from here
(rdtgroup_schemata_write()->rdtgroup_pseudo_lock_create()) does not do
cleanup of this data on failure and relies on the cleanup done earlier.

I agree that this is confusing by not being symmetrical. I'll rework this.

> 
>>  	}
>>  
>> -	plr->cpu = cpumask_first(&d->cpu_mask);
>> -
>> -	if (!cpu_online(plr->cpu)) {
>> -		rdt_last_cmd_printf("CPU %u associated with cache not online\n",
>> -				    plr->cpu);
>> -		ret = -ENODEV;
>> -		goto out_region;
>> +	/* Cache Pseudo-Locking only supported on L2 and L3 resources */
>> +	list_for_each_entry(p, &plr->portions, list) {
>> +		if (p->r->rid != RDT_RESOURCE_L2 &&
>> +		    p->r->rid != RDT_RESOURCE_L3) {
>> +			rdt_last_cmd_puts("Unsupported resource\n");
>> +			goto out_region;
>> +		}
>>  	}
>>  
>> -	plr->line_size = get_cache_line_size(plr->cpu, plr->r->cache_level);
>> -	if (plr->line_size == 0) {
>> -		rdt_last_cmd_puts("Unable to determine cache line size\n");
>> -		ret = -1;
>> -		goto out_region;
>> +	/*
>> +	 * If only one resource requested to be pseudo-locked then:
>> +	 * - Just a L3 cache portion is valid
>> +	 * - Just a L2 cache portion on system without L3 cache is valid
>> +	 */
>> +	if (list_is_singular(&plr->portions)) {
>> +		p = list_first_entry(&plr->portions, struct pseudo_lock_portion,
>> +				     list);
> 
> Let that line stick out.
> 
>> +		if (p->r->rid == RDT_RESOURCE_L3 ||
>> +		    (p->r->rid == RDT_RESOURCE_L2 &&
>> +		     !l3_resource->alloc_capable)) {
>> +			ret = pseudo_lock_single_portion_valid(plr, p);
>> +			if (ret < 0)
>> +				goto out_region;
>> +			return 0;
>> +		} else {
>> +			rdt_last_cmd_puts("Invalid resource or just L2 provided when L3 is required\n");
>> +			goto out_region;
>> +		}
>> +	} else {
>> +		rdt_last_cmd_puts("Multiple pseudo-lock portions unsupported\n");
>>  	}
>>  
>> -	plr->size = rdtgroup_cbm_to_size(plr->r, d, plr->cbm);
>> -
>> -	ret = pseudo_lock_cstates_constrain(plr, &d->cpu_mask);
>> -	if (ret < 0)
>> -		goto out_region;
>> -
>> -	return 0;
>> -
>>  out_region:
>>  	pseudo_lock_region_clear(plr);
>> -	return ret;
>> +	return -1;
>>  }
>>
> 
> Yap, this patch is doing too many things at once and splitting it would help.

Will do. Thank you very much.

Reinette

Re: [PATCH V2 09/10] x86/resctrl: Pseudo-lock portions of multiple resources

[Reinette Chatre]

Hi Borislav,

On 8/7/2019 8:25 AM, Borislav Petkov wrote:
> On Tue, Jul 30, 2019 at 10:29:43AM -0700, Reinette Chatre wrote:
>> A cache pseudo-locked region may span more than one level of cache. A
>> part of the pseudo-locked region that falls on one cache level is
>> referred to as a pseudo-lock portion that was introduced previously.
>>
>> Now a pseudo-locked region is allowed to have two portions instead of
>> the previous limit of one. When a pseudo-locked region consists out of
>> two portions it can only span a L2 and L3 resctrl resource.
>> When a pseudo-locked region consists out of a L2 and L3 portion then
>> there are some requirements:
>> - the L2 and L3 cache has to be in same cache hierarchy
>> - the L3 portion must be same size or larger than L2 portion
>>
>> As documented in previous changes the list of portions are
>> maintained so that the L2 portion would always appear first in the list
>> to simplify any information retrieval.
>>
>> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
>> ---
>>  arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 142 +++++++++++++++++++++-
>>  1 file changed, 139 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
>> index 717ea26e325b..7ab4e85a33a7 100644
>> --- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
>> +++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
>> @@ -339,13 +339,104 @@ static int pseudo_lock_single_portion_valid(struct pseudo_lock_region *plr,
>>  	return -1;
>>  }
>>  
>> +/**
>> + * pseudo_lock_l2_l3_portions_valid - Verify region across L2 and L3
>> + * @plr: Pseudo-Locked region
>> + * @l2_portion: L2 Cache portion of pseudo-locked region
>> + * @l3_portion: L3 Cache portion of pseudo-locked region
>> + *
>> + * User requested a pseudo-locked region consisting of a L2 as well as L3
>> + * cache portion. The portions are tested as follows:
>> + *   - L2 and L3 cache instances have to be in the same cache hierarchy.
>> + *     This is tested by ensuring that the L2 portion's cpumask is a
>> + *     subset of the L3 portion's cpumask.
>> + *   - L3 portion must be same size or larger than L2 portion.
>> + *
>> + * Return: -1 if the portions are unable to be used for a pseudo-locked
>> + *         region, 0 if the portions could be used for a pseudo-locked
>> + *         region. When returning 0:
>> + *         - the pseudo-locked region's size, line_size (cache line length)
>> + *           and CPU on which locking thread will be run are set.
>> + *         - CPUs associated with L2 cache portion are constrained from
>> + *           entering C-state that will affect the pseudo-locked region.
>> + */
>> +static int pseudo_lock_l2_l3_portions_valid(struct pseudo_lock_region *plr,
>> +					    struct pseudo_lock_portion *l2_p,
>> +					    struct pseudo_lock_portion *l3_p)
>> +{
>> +	struct rdt_domain *l2_d, *l3_d;
>> +	unsigned int l2_size, l3_size;
>> +
>> +	l2_d = rdt_find_domain(l2_p->r, l2_p->d_id, NULL);
>> +	if (IS_ERR_OR_NULL(l2_d)) {
>> +		rdt_last_cmd_puts("Cannot locate L2 cache domain\n");
>> +		return -1;
>> +	}
>> +
>> +	l3_d = rdt_find_domain(l3_p->r, l3_p->d_id, NULL);
>> +	if (IS_ERR_OR_NULL(l3_d)) {
>> +		rdt_last_cmd_puts("Cannot locate L3 cache domain\n");
>> +		return -1;
>> +	}
>> +
>> +	if (!cpumask_subset(&l2_d->cpu_mask, &l3_d->cpu_mask)) {
>> +		rdt_last_cmd_puts("L2 and L3 caches need to be in same hierarchy\n");
>> +		return -1;
>> +	}
>> +
> 
> Put that sentence above about L2 CPUs being constrained here - it is
> easier when following the code.

Will do.

> 
>> +	if (pseudo_lock_cstates_constrain(plr, &l2_d->cpu_mask)) {
>> +		rdt_last_cmd_puts("Cannot limit C-states\n");
>> +		return -1;
>> +	}
> 
> Also, can that function call be last in this function so that you can
> save yourself all the goto labels?

I do not fully understand this proposal. All those goto labels take care
of the the different failures that can be encountered during the
initialization of the pseudo-lock region. Each initialization failure is
associated with a goto where it jumps to the cleanup path. The
initialization starts with the constraining of the c-states
(initializing plr->pm_reqs), but if I move that I think it will not
reduce the goto labels, just change the order because of the other
initialization done (plr->size, plr->line_size, plr->cpu).

> 
>> +
>> +	l2_size = rdtgroup_cbm_to_size(l2_p->r, l2_d, l2_p->cbm);
>> +	l3_size = rdtgroup_cbm_to_size(l3_p->r, l3_d, l3_p->cbm);
>> +
>> +	if (l2_size > l3_size) {
>> +		rdt_last_cmd_puts("L3 cache portion has to be same size or larger than L2 cache portion\n");
>> +		goto err_size;
>> +	}
>> +
>> +	plr->size = l2_size;
>> +
>> +	l2_size = get_cache_line_size(cpumask_first(&l2_d->cpu_mask),
>> +				      l2_p->r->cache_level);
>> +	l3_size = get_cache_line_size(cpumask_first(&l3_d->cpu_mask),
>> +				      l3_p->r->cache_level);
>> +	if (l2_size != l3_size) {
>> +		rdt_last_cmd_puts("L2 and L3 caches have different coherency cache line sizes\n");
>> +		goto err_line;
>> +	}
>> +
>> +	plr->line_size = l2_size;
>> +
>> +	plr->cpu = cpumask_first(&l2_d->cpu_mask);
>> +
>> +	if (!cpu_online(plr->cpu)) {
>> +		rdt_last_cmd_printf("CPU %u associated with cache not online\n",
>> +				    plr->cpu);
>> +		goto err_cpu;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_cpu:
>> +	plr->line_size = 0;
>> +	plr->cpu = 0;
>> +err_line:
>> +	plr->size = 0;
>> +err_size:
>> +	pseudo_lock_cstates_relax(plr);
>> +	return -1;
>> +}
>> +
>>  /**
>>   * pseudo_lock_region_init - Initialize pseudo-lock region information
>>   * @plr: pseudo-lock region
>>   *
>>   * Called after user provided a schemata to be pseudo-locked. From the
>>   * schemata the &struct pseudo_lock_region is on entry already initialized
>> - * with the resource, domain, and capacity bitmask. Here the
>> + * with the resource(s), domain(s), and capacity bitmask(s). Here the
>>   * provided data is validated and information required for pseudo-locking
>>   * deduced, and &struct pseudo_lock_region initialized further. This
>>   * information includes:
>> @@ -355,13 +446,24 @@ static int pseudo_lock_single_portion_valid(struct pseudo_lock_region *plr,
>>   * - a cpu associated with the cache instance on which the pseudo-locking
>>   *   flow can be executed
>>   *
>> + * A user provides a schemata for a pseudo-locked region. This schemata may
>> + * contain portions that span different resources, for example, a cache
>> + * pseudo-locked region that spans L2 and L3 cache. After the schemata is
>> + * parsed into portions it needs to be verified that the provided portions
>> + * are valid with the following tests:
>> + *
>> + * - L2 only portion on system that has only L2 resource - OK
>> + * - L3 only portion on any system that supports it - OK
>> + * - L2 portion on system that has L3 resource - require L3 portion
>> + **
>> + *
>>   * Return: 0 on success, <0 on failure. Descriptive error will be written
>>   * to last_cmd_status buffer.
>>   */
>>  static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
>>  {
>>  	struct rdt_resource *l3_resource = &rdt_resources_all[RDT_RESOURCE_L3];
>> -	struct pseudo_lock_portion *p;
>> +	struct pseudo_lock_portion *p, *n_p, *tmp;
>>  	int ret;
>>  
>>  	if (list_empty(&plr->portions)) {
>> @@ -397,8 +499,42 @@ static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
>>  			rdt_last_cmd_puts("Invalid resource or just L2 provided when L3 is required\n");
>>  			goto out_region;
>>  		}
>> +	}
>> +
>> +	/*
>> +	 * List is neither empty nor singular, process first and second portions
>> +	 */
>> +	p = list_first_entry(&plr->portions, struct pseudo_lock_portion, list);
>> +	n_p = list_next_entry(p, list);
>> +
>> +	/*
>> +	 * If the second portion is not also the last portion user provided
>> +	 * more portions than can be supported.
>> +	 */
>> +	tmp = list_last_entry(&plr->portions, struct pseudo_lock_portion, list);
>> +	if (n_p != tmp) {
>> +		rdt_last_cmd_puts("Only two pseudo-lock portions supported\n");
>> +		goto out_region;
>> +	}
>> +
>> +	if (p->r->rid == RDT_RESOURCE_L2 && n_p->r->rid == RDT_RESOURCE_L3) {
>> +		ret = pseudo_lock_l2_l3_portions_valid(plr, p, n_p);
>> +		if (ret < 0)
>> +			goto out_region;
>> +		return 0;
>> +	} else if (p->r->rid == RDT_RESOURCE_L3 &&
>> +		   n_p->r->rid == RDT_RESOURCE_L2) {
>> +		if (pseudo_lock_l2_l3_portions_valid(plr, n_p, p) == 0) {
> 
> 		if (!pseudo_...
> 

Will do. Seems that I need to check all my code for this pattern.

>> +			/*
>> +			 * Let L2 and L3 portions appear in order in the
>> +			 * portions list in support of consistent output to
>> +			 * user space.
>> +			 */
>> +			list_rotate_left(&plr->portions);
>> +			return 0;
>> +		}
>>  	} else {
>> -		rdt_last_cmd_puts("Multiple pseudo-lock portions unsupported\n");
>> +		rdt_last_cmd_puts("Invalid combination of resources\n");
>>  	}
>>  
>>  out_region:
>> -- 
>> 2.17.2
>>
> 

Thank you very much

Reinette

Re: [PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower level caches

[Borislav Petkov]

On Tue, Aug 06, 2019 at 02:16:10PM -0700, Reinette Chatre wrote:
> > I'd leave it to tglx to say how we should mirror cache inclusivity in
> > cpuinfo_x86: whether a synthetic X86_FEATURE bit or cache the respective
> > CPUID words which state whether L2/L3 is inclusive...
> 
> Thank you very much. I appreciate your guidance here.

Ok, tglx and I talked it over a bit on IRC: so your 1/10 patch is pretty
close - just leave out the generic struct cacheinfo bits and put the
cache inclusivity property in a static variable there. It will be a
single bit of information only anyway, as this is system-wide and we can
always move it to generic code when some other arch wants it too.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower level caches

[Borislav Petkov]

On Thu, Aug 08, 2019 at 10:08:41AM +0200, Borislav Petkov wrote:
> Ok, tglx and I talked it over a bit on IRC: so your 1/10 patch is pretty
> close - just leave out the generic struct cacheinfo bits and put the
> cache inclusivity property in a static variable there.

... and by "there" I mean arch/x86/kernel/cpu/cacheinfo.c which contains
all cache properties etc on x86 and is the proper place to put stuff
like that.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH V2 09/10] x86/resctrl: Pseudo-lock portions of multiple resources

[Borislav Petkov]

On Wed, Aug 07, 2019 at 12:23:29PM -0700, Reinette Chatre wrote:
> I do not fully understand this proposal. All those goto labels take care
> of the the different failures that can be encountered during the
> initialization of the pseudo-lock region. Each initialization failure is
> associated with a goto where it jumps to the cleanup path. The
> initialization starts with the constraining of the c-states
> (initializing plr->pm_reqs), but if I move that I think it will not
> reduce the goto labels, just change the order because of the other
> initialization done (plr->size, plr->line_size, plr->cpu).

Here's one possible way to do it, pasting the whole function here as it
is easier to read it this way than an incremental diff ontop.

You basically cache all attributes in local variables and assign them to
the plr struct only on success, at the end. This way, no goto labels and
the C-states constraining, i.e., the most expensive operation, happens
last, only after all the other simpler checks have succeeded. And you
don't have to call pseudo_lock_cstates_relax() prematurely, when one of
those easier checks fail.

Makes sense?

Btw, I've marked the cpu_online() check with "CPU hotplug
lock?!?" question because I don't see you holding that lock with
get_online_cpus()/put_online_cpus().

static int pseudo_lock_l2_l3_portions_valid(struct pseudo_lock_region *plr,
					    struct pseudo_lock_portion *l2_p,
					    struct pseudo_lock_portion *l3_p)
{
	unsigned int l2_size, l3_size, size, line_size, cpu;
	struct rdt_domain *l2_d, *l3_d;

	l2_d = rdt_find_domain(l2_p->r, l2_p->d_id, NULL);
	if (IS_ERR_OR_NULL(l2_d)) {
		rdt_last_cmd_puts("Cannot locate L2 cache domain\n");
		return -1;
	}

	l3_d = rdt_find_domain(l3_p->r, l3_p->d_id, NULL);
	if (IS_ERR_OR_NULL(l3_d)) {
		rdt_last_cmd_puts("Cannot locate L3 cache domain\n");
		return -1;
	}

	if (!cpumask_subset(&l2_d->cpu_mask, &l3_d->cpu_mask)) {
		rdt_last_cmd_puts("L2 and L3 caches need to be in same hierarchy\n");
		return -1;
	}

	l2_size = rdtgroup_cbm_to_size(l2_p->r, l2_d, l2_p->cbm);
	l3_size = rdtgroup_cbm_to_size(l3_p->r, l3_d, l3_p->cbm);

	if (l2_size > l3_size) {
		rdt_last_cmd_puts("L3 cache portion has to be same size or larger than L2 cache portion\n");
		return -1;
	}

	size = l2_size;

	l2_size = get_cache_line_size(cpumask_first(&l2_d->cpu_mask), l2_p->r->cache_level);
	l3_size = get_cache_line_size(cpumask_first(&l3_d->cpu_mask), l3_p->r->cache_level);
	if (l2_size != l3_size) {
		rdt_last_cmd_puts("L2 and L3 caches have different coherency cache line sizes\n");
		return -1;
	}

	line_size = l2_size;

	cpu = cpumask_first(&l2_d->cpu_mask);

	/*
	 * CPU hotplug lock?!?
	 */
	if (!cpu_online(cpu)) {
		rdt_last_cmd_printf("CPU %u associated with cache not online\n", cpu);
		return -1;
	}

	if (!get_cache_inclusive(cpu, l3_p->r->cache_level)) {
		rdt_last_cmd_puts("L3 cache not inclusive\n");
		return -1;
	}

	/*
	 * All checks passed, constrain C-states:
	 */
	if (pseudo_lock_cstates_constrain(plr, &l2_d->cpu_mask)) {
		rdt_last_cmd_puts("Cannot limit C-states\n");
		pseudo_lock_cstates_relax(plr);
		return -1;
	}

	plr->line_size	= line_size;
	plr->size	= size;
	plr->cpu	= cpu;

	return 0;
}

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower level caches

[Reinette Chatre]

Hi Borislav,

On 8/8/2019 1:13 AM, Borislav Petkov wrote:
> On Thu, Aug 08, 2019 at 10:08:41AM +0200, Borislav Petkov wrote:
>> Ok, tglx and I talked it over a bit on IRC: so your 1/10 patch is pretty
>> close - just leave out the generic struct cacheinfo bits and put the
>> cache inclusivity property in a static variable there.
> 
> ... and by "there" I mean arch/x86/kernel/cpu/cacheinfo.c which contains
> all cache properties etc on x86 and is the proper place to put stuff
> like that.

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.

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?
* 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.
* Please note that while this is a single global static variable it will
be set over and over for each CPU on the system.

diff --git a/arch/x86/include/asm/cacheinfo.h
b/arch/x86/include/asm/cacheinfo.h
index 86b63c7feab7..97be5141bb4b 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);

+unsigned int cacheinfo_intel_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 733874f84f41..247b6a9b5c88 100644
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -187,6 +187,7 @@ struct _cpuid4_info_regs {
 };

 static unsigned short num_cache_leaves;
+static unsigned l3_inclusive;

 /* AMD doesn't have CPUID4. Emulate it here to report the same
    information to the user.  This makes some assumptions about the machine:
@@ -745,6 +746,11 @@ void init_hygon_cacheinfo(struct cpuinfo_x86 *c)
        num_cache_leaves = find_num_cache_leaves(c);
 }

+unsigned int cacheinfo_intel_l3_inclusive(void)
+{
+       return l3_inclusive;
+}
+
 void init_intel_cacheinfo(struct cpuinfo_x86 *c)
 {
        /* Cache sizes */
@@ -795,6 +801,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);
+                               l3_inclusive =
this_leaf.edx.split.inclusive;
                                break;
                        default:
                                break;
@@ -1010,13 +1017,6 @@ static void ci_leaf_init(struct cacheinfo *this_leaf,
        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;
 }

What do you think?

Reinette

Re: [PATCH V2 09/10] x86/resctrl: Pseudo-lock portions of multiple resources

[Reinette Chatre]

Hi Borislav,

On 8/8/2019 1:44 AM, Borislav Petkov wrote:
> On Wed, Aug 07, 2019 at 12:23:29PM -0700, Reinette Chatre wrote:
>> I do not fully understand this proposal. All those goto labels take care
>> of the the different failures that can be encountered during the
>> initialization of the pseudo-lock region. Each initialization failure is
>> associated with a goto where it jumps to the cleanup path. The
>> initialization starts with the constraining of the c-states
>> (initializing plr->pm_reqs), but if I move that I think it will not
>> reduce the goto labels, just change the order because of the other
>> initialization done (plr->size, plr->line_size, plr->cpu).
> 
> Here's one possible way to do it, pasting the whole function here as it
> is easier to read it this way than an incremental diff ontop.
> 
> You basically cache all attributes in local variables and assign them to
> the plr struct only on success, at the end. This way, no goto labels and
> the C-states constraining, i.e., the most expensive operation, happens
> last, only after all the other simpler checks have succeeded. And you
> don't have to call pseudo_lock_cstates_relax() prematurely, when one of
> those easier checks fail.
> 
> Makes sense?

It does. This looks much better. Thank you very much.

> 
> Btw, I've marked the cpu_online() check with "CPU hotplug
> lock?!?" question because I don't see you holding that lock with
> get_online_cpus()/put_online_cpus().

There is a locking order dependency between cpu_hotplug_lock and
rdtgroup_mutex (cpu_hotplug_lock before rdtgroup_mutex) that has to be
maintained. To do so in this flow you will find cpus_read_lock() in
rdtgroup_schemata_write(), so quite a distance from where it is needed.

Perhaps I should add a comment at the location where the lock is
required to document where the lock is obtained?

> static int pseudo_lock_l2_l3_portions_valid(struct pseudo_lock_region *plr,
> 					    struct pseudo_lock_portion *l2_p,
> 					    struct pseudo_lock_portion *l3_p)
> {
> 	unsigned int l2_size, l3_size, size, line_size, cpu;
> 	struct rdt_domain *l2_d, *l3_d;
> 
> 	l2_d = rdt_find_domain(l2_p->r, l2_p->d_id, NULL);
> 	if (IS_ERR_OR_NULL(l2_d)) {
> 		rdt_last_cmd_puts("Cannot locate L2 cache domain\n");
> 		return -1;
> 	}
> 
> 	l3_d = rdt_find_domain(l3_p->r, l3_p->d_id, NULL);
> 	if (IS_ERR_OR_NULL(l3_d)) {
> 		rdt_last_cmd_puts("Cannot locate L3 cache domain\n");
> 		return -1;
> 	}
> 
> 	if (!cpumask_subset(&l2_d->cpu_mask, &l3_d->cpu_mask)) {
> 		rdt_last_cmd_puts("L2 and L3 caches need to be in same hierarchy\n");
> 		return -1;
> 	}
> 
> 	l2_size = rdtgroup_cbm_to_size(l2_p->r, l2_d, l2_p->cbm);
> 	l3_size = rdtgroup_cbm_to_size(l3_p->r, l3_d, l3_p->cbm);
> 
> 	if (l2_size > l3_size) {
> 		rdt_last_cmd_puts("L3 cache portion has to be same size or larger than L2 cache portion\n");
> 		return -1;
> 	}
> 
> 	size = l2_size;
> 
> 	l2_size = get_cache_line_size(cpumask_first(&l2_d->cpu_mask), l2_p->r->cache_level);
> 	l3_size = get_cache_line_size(cpumask_first(&l3_d->cpu_mask), l3_p->r->cache_level);
> 	if (l2_size != l3_size) {
> 		rdt_last_cmd_puts("L2 and L3 caches have different coherency cache line sizes\n");
> 		return -1;
> 	}
> 
> 	line_size = l2_size;
> 
> 	cpu = cpumask_first(&l2_d->cpu_mask);
> 
> 	/*
> 	 * CPU hotplug lock?!?
> 	 */
> 	if (!cpu_online(cpu)) {
> 		rdt_last_cmd_printf("CPU %u associated with cache not online\n", cpu);
> 		return -1;
> 	}
> 
> 	if (!get_cache_inclusive(cpu, l3_p->r->cache_level)) {
> 		rdt_last_cmd_puts("L3 cache not inclusive\n");
> 		return -1;
> 	}
> 
> 	/*
> 	 * All checks passed, constrain C-states:
> 	 */
> 	if (pseudo_lock_cstates_constrain(plr, &l2_d->cpu_mask)) {
> 		rdt_last_cmd_puts("Cannot limit C-states\n");
> 		pseudo_lock_cstates_relax(plr);
> 		return -1;
> 	}
> 
> 	plr->line_size	= line_size;
> 	plr->size	= size;
> 	plr->cpu	= cpu;
> 
> 	return 0;
> }
> 

Thank you very much

Reinette

Re: [PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower level caches

[Borislav Petkov]

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.

Re: [PATCH V2 09/10] x86/resctrl: Pseudo-lock portions of multiple resources

[Borislav Petkov]

On Thu, Aug 08, 2019 at 01:13:46PM -0700, Reinette Chatre wrote:
> There is a locking order dependency between cpu_hotplug_lock and
> rdtgroup_mutex (cpu_hotplug_lock before rdtgroup_mutex) that has to be
> maintained. To do so in this flow you will find cpus_read_lock() in
> rdtgroup_schemata_write(), so quite a distance from where it is needed.
> 
> Perhaps I should add a comment at the location where the lock is
> required to document where the lock is obtained?

Even better - you can add:

	lockdep_assert_cpus_held();

above it which documents *and* checks too. :-)

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower level caches

[Reinette Chatre]

Hi Borislav,

On 8/9/2019 12:33 AM, Borislav Petkov wrote:
> 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.

I am sorry about that.

> 
>> 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;
> 
> 

I really appreciate that you took the time to put this together. I now
understand how all your suggestions come together.

Thank you for your patience in reviewing this change.

Reinette

Re: [PATCH V2 09/10] x86/resctrl: Pseudo-lock portions of multiple resources

[Reinette Chatre]

Hi Borislav,

On 8/9/2019 12:38 AM, Borislav Petkov wrote:
> On Thu, Aug 08, 2019 at 01:13:46PM -0700, Reinette Chatre wrote:
>> There is a locking order dependency between cpu_hotplug_lock and
>> rdtgroup_mutex (cpu_hotplug_lock before rdtgroup_mutex) that has to be
>> maintained. To do so in this flow you will find cpus_read_lock() in
>> rdtgroup_schemata_write(), so quite a distance from where it is needed.
>>
>> Perhaps I should add a comment at the location where the lock is
>> required to document where the lock is obtained?
> 
> Even better - you can add:
> 
> 	lockdep_assert_cpus_held();
> 
> above it which documents *and* checks too. :-)
> 

Will do.

Thank you

Reinette