x86/intel_rdt: Intel Memory bandwidth allocation

PATCH Superseded Vikas Shivappa 3 participants 18 messages Feb 17, 2017 – Mar 30, 2017

Subsystems (2)

Documentation x86

lore

Share & tools

lore

Vikas Shivappa

author

2017-02-17 19:58 UTC #0 lore raw

v2 cover x86/intel_rdt: Intel Memory bandwidth allocation

Memory b/w allocation(MBA) is part of the Intel Resource Director Technology (RDT). RDT helps monitor and share processor shared resources. MBA helps enforce a limit on the memory bandwidth(b/w) threads can use when they are scheduled. OS does the enforcement using MSR(model specific register) interface and mapping the threads to architecture specific CLOSids(class of service IDs).
The user interface is via the common resctrl fs for RDT features.

This can be used along with MBM (memory b/w monitoring) and cache allocation to control and monitor the applications cache and memory resources as per the QoS or other performance requirements. Use cases could be large serverclusters, VM, clould and container based services where the admin or orchestration tools can use this framework to manage/allocate these processor shared resources like other memory/cpu resources to provide QoS guarentees. Detailed use case examples are documented in the 01/08 Documentation patch.

Patches are dependent on :
https://lkml.org/lkml/2017/2/17/595

V2 changes
----------
Thanks to Thomas for all the feed back and below are the changes as per the feedback:
- Sent the RDT generic improvements patch series seperately
- Split the info file/validate apis as prep patches to MBA changes
- Fixed a lot of coding convention issues, variable initializations, and removing unnecessary allocations etc.
- Changed the hotcpu handling for CLOS/RMID to zero the CLOSid/RMID when cpu is offlined.
- Changed some naming conventions.
- Changed the interface to show the bandwidth percentage that can be requested rather than showing h/w specific delay values which do not map to a particular b/w percentage in non-linear scale

[PATCH 1/8] Documentation, x86: Documentation for Intel Mem b/w [PATCH 2/8] x86/intel_rdt/mba: Generalize the naming to get ready for [PATCH 3/8] x86/intel_rdt/mba: Memory b/w allocation feature detect [PATCH 4/8] x86/intel_rct/mba: Add MBA structures and initialize MBA [PATCH 5/8] x86/intel_rdt: info file support for MBA prepare [PATCH 6/8] x86/intel_rdt/mba: Add info directory files for MBA [PATCH 7/8] x86/intel_rdt: schemata file support for MBA prepare [PATCH 8/8] x86/intel_rdt/mba: Add schemata file support for MBA

Vikas Shivappa

author

2017-02-17 20:00 UTC #1 lore raw

PATCH v2 1/8 Documentation, x86: Documentation for Intel Mem b/w allocation

Update the intel_rdt_ui documentation to have Memory bandwidth(b/w) allocation interface usage.

Signed-off-by Vikas Shivappa <[email protected]>

---
 Documentation/x86/intel_rdt_ui.txt | 74 ++++++++++++++++++++++++++++++++++----
 1 file changed, 67 insertions(+), 7 deletions(-)

Documentation/x86/intel_rdt_ui.txt +67 -7

index d918d26..2f679e2 100644
@@ -4,6 +4,7 @@ Copyright (C) 2016 Intel Corporation
 
 Fenghua Yu <[email protected]>
 Tony Luck <[email protected]>
+Vikas Shivappa <[email protected]>
 
 This feature is enabled by the CONFIG_INTEL_RDT_A Kconfig and the
 X86 /proc/cpuinfo flag bits "rdt", "cat_l3" and "cdp_l3".

@@ -22,8 +23,8 @@ Info directory
 
 The 'info' directory contains information about the enabled
 resources. Each resource has its own subdirectory. The subdirectory
-names reflect the resource names. Each subdirectory contains the
-following files:
+names reflect the resource names.
+Cache resource(L3/L2)  subdirectory contains the following files:
 
 "num_closids":  The number of CLOSIDs which are valid for this
 	        resource. The kernel uses the smallest number of

@@ -35,6 +36,16 @@ following files:
 "min_cbm_bits": The minimum number of consecutive bits which must be
 		set when writing a mask.
 
+Memory bandwitdh(MB) subdirectory contains the following files:
+
+"min_bw":	The minimum memory bandwidth percentage which user can
+		request.
+
+"bw_gran":	The granularity in which the user can request the memory
+		bandwidth percentage.
+
+"scale_linear":Indicates if the bandwidth scale is linear or
+		non-linear.
 
 Resource groups
 ---------------

@@ -107,6 +118,28 @@ and 0xA are not.  On a system with a 20-bit mask each bit represents 5%
 of the capacity of the cache. You could partition the cache into four
 equal parts with masks: 0x1f, 0x3e0, 0x7c00, 0xf8000.
 
+Memory bandwidth(b/w) throttle
+------------------------------
+For Memory b/w resource, user controls the resource by indicating the
+percentage of total memory b/w.
+
+The minimum bandwidth percentage value for each cpu model is predefined
+and can be looked up through "info/MB/min_bw". The bandwidth granularity
+that can be requested is also dependent on the cpu model and can be
+looked up at "info/MB/bw_gran".
+
+The bandwidth percentage values are mapped to hardware delay values and
+programmed in the QOS_MSRs. The delay values may be in linear scale and
+non-linear scale. In a linear scale the programmed values directly
+correspond to a delay value(b/w percentage = 100 - delay). However in a
+non-linear scale, the percentage values correspond to a pre-caliberated
+delay values. The delay values in non-linear scale have the granularity
+of power of 2.
+
+The bandwidth throttling is a a core specific mechanism on some of Intel
+SKUs. Using a high bandwidth and a low bandwidth setting on two threads
+sharing a core will result in both threads being throttled to use the
+low bandwidth.
 
 L3 details (code and data prioritization disabled)
 --------------------------------------------------

@@ -129,16 +162,24 @@ schemata format is always:
 
 	L2:<cache_id0>=<cbm>;<cache_id1>=<cbm>;...
 
+Memory b/w Allocation details
+-----------------------------
+
+Memory b/w domain is L3 cache.
+
+	MB:<cache_id0>=bandwidth0;<cache_id1>=bandwidth1;...
+
 Example 1
 ---------
 On a two socket machine (one L3 cache per socket) with just four bits
-for cache bit masks
+for cache bit masks, minimum b/w of 10% with a memory bandwidth
+granularity of 10%
 
 # mount -t resctrl resctrl /sys/fs/resctrl
 # cd /sys/fs/resctrl
 # mkdir p0 p1
-# echo "L3:0=3;1=c" > /sys/fs/resctrl/p0/schemata
-# echo "L3:0=3;1=3" > /sys/fs/resctrl/p1/schemata
+# echo "L3:0=3;1=c\nMB:0=50;1=50" > /sys/fs/resctrl/p0/schemata
+# echo "L3:0=3;1=3\nMB:0=50;1=50" > /sys/fs/resctrl/p1/schemata
 
 The default resource group is unmodified, so we have access to all parts
 of all caches (its schemata file reads "L3:0=f;1=f").

@@ -147,6 +188,14 @@ Tasks that are under the control of group "p0" may only allocate from the
 "lower" 50% on cache ID 0, and the "upper" 50% of cache ID 1.
 Tasks in group "p1" use the "lower" 50% of cache on both sockets.
 
+Similarly, tasks that are under the control of group "p0" may use a
+maximum memory b/w of 50% on socket0, and the 50% on socket 1.
+Tasks in group "p1" may use the rest of 50% memory b/w on both sockets.
+Note that unlike cache masks, memory b/w cannot specify whether these
+allocations can overlap or not. The allocations specifies the maximum
+b/w that the group may be able to use and the system admin can configure
+the b/w accordingly.
+
 Example 2
 ---------
 Again two sockets, but this time with a more realistic 20-bit mask.

@@ -185,6 +234,16 @@ Ditto for the second real time task (with the remaining 25% of cache):
 # echo 5678 > p1/tasks
 # taskset -cp 2 5678
 
+For the same 2 socket system with memory b/w resource and CAT L3 the
+schemata would look like:
+
+Assume min_bw 10 and bw_gran is 10.
+
+# echo -e "L3:0=f8000;1=fffff\nMB:0=10;1=30" > p0/schemata
+
+This would request 10% memory b/w on socket 0 and 30% memory b/w on
+socket1.
+
 Example 3
 ---------

@@ -203,10 +262,11 @@ First we reset the schemata for the default group so that the "upper"
 # echo "L3:0=3ff" > schemata
 
 Next we make a resource group for our real time cores and give
-it access to the "top" 50% of the cache on socket 0.
+it access to the "top" 50% of the cache on socket 0 and 50% of memory
+bandwidth on socket 0.
 
 # mkdir p0
-# echo "L3:0=ffc00;" > p0/schemata
+# echo "L3:0=ffc00;\nMB:0=50" > p0/schemata
 
 Finally we move core 4-7 over to the new group and make sure that the
 kernel and the tasks running there get 50% of the cache.

Signature · 1 line

1.9.1

Vikas Shivappa

author

2017-02-17 20:00 UTC #2 lore raw

PATCH v2 2/8 x86/intel_rdt/mba: Generalize the naming to get ready for MBA

In reply to Vikas Shivappa

Preparatory patch to generalize the naming for RDT so that we can get ready to apply other resource controls like MBA.

RDT resource cbm values are named to ctrl_val representing generic control values which will hold both cbm(cache bit mask) and memory b/w throttle values. max_cbm is updated to default_ctrl which represents default values which provide no control which is all bits set in case of CAT and 100% bandwidth in case of MBA. The tmp_cbm is updated to tmp_ctrls. Similarly domain structures are updated to ctrl_val instead of cbm.

APIs are also generalized:
- get_cache_config is added to separate from memory specific apis.
- MSR update api names are changed from having cbm to ctrl.
- info file API names are set to reflect generic default_ctrl or control values rather than cbm.

Signed-off-by Vikas Shivappa <[email protected]>

---
 arch/x86/include/asm/intel_rdt.h         | 22 +++++++++++-----------
 arch/x86/kernel/cpu/intel_rdt.c          | 28 ++++++++++++++--------------
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 12 ++++++------
 arch/x86/kernel/cpu/intel_rdt_schemata.c | 26 +++++++++++++-------------
 4 files changed, 44 insertions(+), 44 deletions(-)

arch/x86/include/asm/intel_rdt.h +11 ~~-11~~

index 95ce5c8..326df9e 100644
@@ -71,14 +71,14 @@ struct rftype {
  * @capable:			Is this feature available on this machine
  * @name:			Name to use in "schemata" file
  * @num_closid:			Number of CLOSIDs available
- * @max_cbm:			Largest Cache Bit Mask allowed
+ * @default_ctrl:		Specifies default cache cbm or mem b/w percent.
  * @min_cbm_bits:		Minimum number of consecutive bits to be set
  *				in a cache bit mask
  * @domains:			All domains for this resource
  * @num_domains:		Number of domains active
  * @msr_base:			Base MSR address for CBMs
- * @tmp_cbms:			Scratch space when updating schemata
- * @num_tmp_cbms:		Number of CBMs in tmp_cbms
+ * @tmp_ctrl:			Scratch space when updating schemata
+ * @num_tmp_ctrl:		Number of control values in tmp_ctrl
  * @cache_level:		Which cache level defines scope of this domain
  * @cbm_idx_multi:		Multiplier of CBM index
  * @cbm_idx_offset:		Offset of CBM index. CBM index is computed by:

@@ -91,12 +91,12 @@ struct rdt_resource {
 	int			num_closid;
 	int			cbm_len;
 	int			min_cbm_bits;
-	u32			max_cbm;
+	u32			default_ctrl;
 	struct list_head	domains;
 	int			num_domains;
 	int			msr_base;
-	u32			*tmp_cbms;
-	int			num_tmp_cbms;
+	u32			*tmp_ctrl;
+	int			num_tmp_ctrl;
 	int			cache_level;
 	int			cbm_idx_multi;
 	int			cbm_idx_offset;

@@ -107,13 +107,13 @@ struct rdt_resource {
  * @list:	all instances of this resource
  * @id:		unique id for this instance
  * @cpu_mask:	which cpus share this resource
- * @cbm:	array of cache bit masks (indexed by CLOSID)
+ * @ctrl_val:	array of cache or mem ctrl values (indexed by CLOSID)
  */
 struct rdt_domain {
 	struct list_head	list;
 	int			id;
 	struct cpumask		cpu_mask;
-	u32			*cbm;
+	u32			*ctrl_val;
 };
 
 /**

@@ -164,8 +164,8 @@ enum {
 	unsigned int full;
 };
 
-/* CPUID.(EAX=10H, ECX=ResID=1).EDX */
-union cpuid_0x10_1_edx {
+/* CPUID.(EAX=10H, ECX=ResID).EDX */
+union cpuid_0x10_x_edx {
 	struct {
 		unsigned int cos_max:16;
 	} split;

@@ -174,7 +174,7 @@ enum {
 
 DECLARE_PER_CPU_READ_MOSTLY(int, cpu_closid);
 
-void rdt_cbm_update(void *arg);
+void rdt_ctrl_update(void *arg);
 struct rdtgroup *rdtgroup_kn_lock_live(struct kernfs_node *kn);
 void rdtgroup_kn_unlock(struct kernfs_node *kn);
 ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,

arch/x86/kernel/cpu/intel_rdt.c +14 ~~-14~~

index c8af5d9..5d8c1be 100644
@@ -119,7 +119,7 @@ static inline bool cache_alloc_hsw_probe(void)
 
 		r->num_closid = 4;
 		r->cbm_len = 20;
-		r->max_cbm = max_cbm;
+		r->default_ctrl = max_cbm;
 		r->min_cbm_bits = 2;
 		r->capable = true;
 		r->enabled = true;

@@ -130,16 +130,16 @@ static inline bool cache_alloc_hsw_probe(void)
 	return false;
 }
 
-static void rdt_get_config(int idx, struct rdt_resource *r)
+static void rdt_get_cache_config(int idx, struct rdt_resource *r)
 {
 	union cpuid_0x10_1_eax eax;
-	union cpuid_0x10_1_edx edx;
+	union cpuid_0x10_x_edx edx;
 	u32 ebx, ecx;
 
 	cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx, &edx.full);
 	r->num_closid = edx.split.cos_max + 1;
 	r->cbm_len = eax.split.cbm_len + 1;
-	r->max_cbm = BIT_MASK(eax.split.cbm_len + 1) - 1;
+	r->default_ctrl = BIT_MASK(eax.split.cbm_len + 1) - 1;
 	r->capable = true;
 	r->enabled = true;
 }

@@ -151,7 +151,7 @@ static void rdt_get_cdp_l3_config(int type)
 
 	r->num_closid = r_l3->num_closid / 2;
 	r->cbm_len = r_l3->cbm_len;
-	r->max_cbm = r_l3->max_cbm;
+	r->default_ctrl = r_l3->default_ctrl;
 	r->capable = true;
 	/*
 	 * By default, CDP is disabled. CDP can be enabled by mount parameter

@@ -171,7 +171,7 @@ static inline bool get_rdt_resources(void)
 		return false;
 
 	if (boot_cpu_has(X86_FEATURE_CAT_L3)) {
-		rdt_get_config(1, &rdt_resources_all[RDT_RESOURCE_L3]);
+		rdt_get_cache_config(1, &rdt_resources_all[RDT_RESOURCE_L3]);
 		if (boot_cpu_has(X86_FEATURE_CDP_L3)) {
 			rdt_get_cdp_l3_config(RDT_RESOURCE_L3DATA);
 			rdt_get_cdp_l3_config(RDT_RESOURCE_L3CODE);

@@ -180,7 +180,7 @@ static inline bool get_rdt_resources(void)
 	}
 	if (boot_cpu_has(X86_FEATURE_CAT_L2)) {
 		/* CPUID 0x10.2 fields are same format at 0x10.1 */
-		rdt_get_config(2, &rdt_resources_all[RDT_RESOURCE_L2]);
+		rdt_get_cache_config(2, &rdt_resources_all[RDT_RESOURCE_L2]);
 		ret = true;
 	}

@@ -200,7 +200,7 @@ static int get_cache_id(int cpu, int level)
 	return -1;
 }
 
-void rdt_cbm_update(void *arg)
+void rdt_ctrl_update(void *arg)
 {
 	struct msr_param *m = (struct msr_param *)arg;
 	struct rdt_resource *r = m->res;

@@ -221,7 +221,7 @@ void rdt_cbm_update(void *arg)
 	for (i = m->low; i < m->high; i++) {
 		int idx = cbm_idx(r, i);
 
-		wrmsrl(r->msr_base + idx, d->cbm[i]);
+		wrmsrl(r->msr_base + idx, d->ctrl_val[i]);
 	}
 }

@@ -294,8 +294,8 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
 
 	d->id = id;
 
-	d->cbm = kmalloc_array(r->num_closid, sizeof(*d->cbm), GFP_KERNEL);
-	if (!d->cbm) {
+	d->ctrl_val = kmalloc_array(r->num_closid, sizeof(*d->ctrl_val), GFP_KERNEL);
+	if (!d->ctrl_val) {
 		kfree(d);
 		return;
 	}

@@ -303,8 +303,8 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
 	for (i = 0; i < r->num_closid; i++) {
 		int idx = cbm_idx(r, i);
 
-		d->cbm[i] = r->max_cbm;
-		wrmsrl(r->msr_base + idx, d->cbm[i]);
+		d->ctrl_val[i] = r->default_ctrl;
+		wrmsrl(r->msr_base + idx, d->ctrl_val[i]);
 	}
 
 	cpumask_set_cpu(cpu, &d->cpu_mask);

@@ -326,7 +326,7 @@ static void domain_remove_cpu(int cpu, struct rdt_resource *r)
 	cpumask_clear_cpu(cpu, &d->cpu_mask);
 	if (cpumask_empty(&d->cpu_mask)) {
 		r->num_domains--;
-		kfree(d->cbm);
+		kfree(d->ctrl_val);
 		list_del(&d->list);
 		kfree(d);
 	}

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +6 -6

index 9b9565f..0d8fa61 100644
@@ -498,12 +498,12 @@ static int rdt_num_closids_show(struct kernfs_open_file *of,
 	return 0;
 }
 
-static int rdt_cbm_mask_show(struct kernfs_open_file *of,
+static int rdt_default_ctrl_show(struct kernfs_open_file *of,
 			     struct seq_file *seq, void *v)
 {
 	struct rdt_resource *r = of->kn->parent->priv;
 
-	seq_printf(seq, "%x\n", r->max_cbm);
+	seq_printf(seq, "%x\n", r->default_ctrl);
 
 	return 0;
 }

@@ -530,7 +530,7 @@ static int rdt_min_cbm_bits_show(struct kernfs_open_file *of,
 		.name		= "cbm_mask",
 		.mode		= 0444,
 		.kf_ops		= &rdtgroup_kf_single_ops,
-		.seq_show	= rdt_cbm_mask_show,
+		.seq_show	= rdt_default_ctrl_show,
 	},
 	{
 		.name		= "min_cbm_bits",

@@ -803,14 +803,14 @@ static int reset_all_ctrls(struct rdt_resource *r, u32 sclosid, u32 eclosid)
 		cpumask_set_cpu(cpumask_any(&d->cpu_mask), cpu_mask);
 
 		for (i = sclosid; i < eclosid; i++)
-			d->cbm[i] = r->max_cbm;
+			d->ctrl_val[i] = r->default_ctrl;
 	}
 	cpu = get_cpu();
 	/* Update CBM on this cpu if it's in cpu_mask. */
 	if (cpumask_test_cpu(cpu, cpu_mask))
-		rdt_cbm_update(&msr_param);
+		rdt_ctrl_update(&msr_param);
 	/* Update CBM on all other cpus in cpu_mask. */
-	smp_call_function_many(cpu_mask, rdt_cbm_update, &msr_param, 1);
+	smp_call_function_many(cpu_mask, rdt_ctrl_update, &msr_param, 1);
 	put_cpu();
 
 	free_cpumask_var(cpu_mask);

arch/x86/kernel/cpu/intel_rdt_schemata.c +13 ~~-13~~

index 527d042..3cde1e8 100644
@@ -38,7 +38,7 @@ static bool cbm_validate(unsigned long var, struct rdt_resource *r)
 {
 	unsigned long first_bit, zero_bit;
 
-	if (var == 0 || var > r->max_cbm)
+	if (var == 0 || var > r->default_ctrl)
 		return false;
 
 	first_bit = find_first_bit(&var, r->cbm_len);

@@ -66,7 +66,7 @@ static int parse_cbm(char *buf, struct rdt_resource *r)
 		return ret;
 	if (!cbm_validate(data, r))
 		return -EINVAL;
-	r->tmp_cbms[r->num_tmp_cbms++] = data;
+	r->tmp_ctrl[r->num_tmp_ctrl++] = data;
 
 	return 0;
 }

@@ -95,7 +95,7 @@ static int parse_line(char *line, struct rdt_resource *r)
 	}
 
 	/* Incorrect number of domains in the line */
-	if (r->num_tmp_cbms != r->num_domains)
+	if (r->num_tmp_ctrl != r->num_domains)
 		return -EINVAL;
 
 	/* Any garbage at the end of the line? */

@@ -123,18 +123,18 @@ static int update_domains(struct rdt_resource *r, int closid)
 	 * There by avoiding unnecessary IPIs.
 	 */
 	list_for_each_entry(d, &r->domains, list) {
-		if (d->cbm[msr_param.low] != r->tmp_cbms[idx]) {
+		if (d->ctrl_val[msr_param.low] != r->tmp_ctrl[idx]) {
 			cpumask_set_cpu(cpumask_any(&d->cpu_mask), cpu_mask);
-			d->cbm[msr_param.low] = r->tmp_cbms[idx];
+			d->ctrl_val[msr_param.low] = r->tmp_ctrl[idx];
 		}
 		idx++;
 	}
 	cpu = get_cpu();
 	/* Update CBM on this cpu if it's in cpu_mask. */
 	if (cpumask_test_cpu(cpu, cpu_mask))
-		rdt_cbm_update(&msr_param);
+		rdt_ctrl_update(&msr_param);
 	/* Update CBM on other cpus. */
-	smp_call_function_many(cpu_mask, rdt_cbm_update, &msr_param, 1);
+	smp_call_function_many(cpu_mask, rdt_ctrl_update, &msr_param, 1);
 	put_cpu();
 
 	free_cpumask_var(cpu_mask);

@@ -187,13 +187,13 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
 
 	/* get scratch space to save all the masks while we validate input */
 	for_each_enabled_rdt_resource(r) {
-		r->tmp_cbms = kcalloc(r->num_domains, sizeof(*l3_cbms),
+		r->tmp_ctrl = kcalloc(r->num_domains, sizeof(*l3_cbms),
 				      GFP_KERNEL);
-		if (!r->tmp_cbms) {
+		if (!r->tmp_ctrl) {
 			ret = -ENOMEM;
 			goto out;
 		}
-		r->num_tmp_cbms = 0;
+		r->num_tmp_ctrl = 0;
 	}
 
 	r = NULL;

@@ -225,8 +225,8 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
 out:
 	rdtgroup_kn_unlock(of->kn);
 	for_each_enabled_rdt_resource(r) {
-		kfree(r->tmp_cbms);
-		r->tmp_cbms = NULL;
+		kfree(r->tmp_ctrl);
+		r->tmp_ctrl = NULL;
 	}
 	return ret ?: nbytes;
 }

@@ -240,7 +240,7 @@ static void show_doms(struct seq_file *s, struct rdt_resource *r, int closid)
 	list_for_each_entry(dom, &r->domains, list) {
 		if (sep)
 			seq_puts(s, ";");
-		seq_printf(s, "%d=%x", dom->id, dom->cbm[closid]);
+		seq_printf(s, "%d=%x", dom->id, dom->ctrl_val[closid]);
 		sep = true;
 	}
 	seq_puts(s, "\n");

Signature · 1 line

1.9.1

Vikas Shivappa

author

2017-02-17 19:59 UTC #3 lore raw

PATCH v2 3/8 x86/intel_rdt/mba: Memory b/w allocation feature detect

In reply to Vikas Shivappa

Detect MBA feature if CPUID.(EAX=10H, ECX=0):EBX.L2[bit 3] = 1. Add supporting data structures to detect feature details which is done in later patch using CPUID with EAX=10H, ECX= 3.

Signed-off-by Vikas Shivappa <[email protected]>

---
 arch/x86/include/asm/cpufeatures.h | 2 ++
 arch/x86/include/asm/intel_rdt.h   | 8 ++++++++
 arch/x86/kernel/cpu/intel_rdt.c    | 4 ++++
 arch/x86/kernel/cpu/scattered.c    | 1 +
 4 files changed, 15 insertions(+)

arch/x86/include/asm/cpufeatures.h +2 -0

index eafee31..50cbdd0 100644
@@ -201,6 +201,8 @@
 #define X86_FEATURE_AVX512_4VNNIW (7*32+16) /* AVX-512 Neural Network Instructions */
 #define X86_FEATURE_AVX512_4FMAPS (7*32+17) /* AVX-512 Multiply Accumulation Single precision */
 
+#define X86_FEATURE_MBA         ( 7*32+18) /* Memory Bandwidth Allocation */
+
 /* Virtualization flags: Linux defined, word 8 */
 #define X86_FEATURE_TPR_SHADOW  ( 8*32+ 0) /* Intel TPR Shadow */
 #define X86_FEATURE_VNMI        ( 8*32+ 1) /* Intel Virtual NMI */

arch/x86/include/asm/intel_rdt.h +8 -0

index 326df9e..d2eee45 100644
@@ -164,6 +164,14 @@ enum {
 	unsigned int full;
 };
 
+/* CPUID.(EAX=10H, ECX=ResID=3).EAX */
+union cpuid_0x10_3_eax {
+	struct {
+		unsigned int max_delay:12;
+	} split;
+	unsigned int full;
+};
+
 /* CPUID.(EAX=10H, ECX=ResID).EDX */
 union cpuid_0x10_x_edx {
 	struct {

arch/x86/kernel/cpu/intel_rdt.c +4 -0

index 5d8c1be..b76a518 100644
@@ -184,6 +184,10 @@ static inline bool get_rdt_resources(void)
 		ret = true;
 	}
 
+	if (boot_cpu_has(X86_FEATURE_MBA)) {
+		ret = true;
+	}
+
 	return ret;
 }

arch/x86/kernel/cpu/scattered.c +1 -0

index d979406..23c2350 100644
@@ -27,6 +27,7 @@ struct cpuid_bit {
 	{ X86_FEATURE_CAT_L3,		CPUID_EBX,  1, 0x00000010, 0 },
 	{ X86_FEATURE_CAT_L2,		CPUID_EBX,  2, 0x00000010, 0 },
 	{ X86_FEATURE_CDP_L3,		CPUID_ECX,  2, 0x00000010, 1 },
+	{ X86_FEATURE_MBA,		CPUID_EBX,  3, 0x00000010, 0 },
 	{ X86_FEATURE_HW_PSTATE,	CPUID_EDX,  7, 0x80000007, 0 },
 	{ X86_FEATURE_CPB,		CPUID_EDX,  9, 0x80000007, 0 },
 	{ X86_FEATURE_PROC_FEEDBACK,    CPUID_EDX, 11, 0x80000007, 0 },

Signature · 1 line

1.9.1

Vikas Shivappa

author

2017-02-17 19:59 UTC #4 lore raw

PATCH v2 4/8 x86/intel_rct/mba: Add MBA structures and initialize MBA

In reply to Vikas Shivappa

The MBA feature details like minimum bandwidth supported, b/w granularity etc are obtained via executing CPUID with EAX=10H ,ECX=3.

Setup and initialize the MBA specific extensions to data structures like global list of RDT resources, RDT resource structure and RDT domain structure.

Signed-off-by Vikas Shivappa <[email protected]>

---
 arch/x86/include/asm/intel_rdt.h | 17 +++++++
 arch/x86/kernel/cpu/intel_rdt.c  | 95 ++++++++++++++++++++++++++++++++++------
 2 files changed, 99 insertions(+), 13 deletions(-)

arch/x86/include/asm/intel_rdt.h +17 -0

index d2eee45..af65b2a 100644
@@ -11,6 +11,9 @@
 #define IA32_L3_QOS_CFG		0xc81
 #define IA32_L3_CBM_BASE	0xc90
 #define IA32_L2_CBM_BASE	0xd10
+#define IA32_MBA_THRTL_BASE	0xd50
+#define MAX_MBA_THRTL		100u
+#define MBA_IS_LINEAR		0x4
 
 #define L3_QOS_CDP_ENABLE	0x01ULL

@@ -74,6 +77,14 @@ struct rftype {
  * @default_ctrl:		Specifies default cache cbm or mem b/w percent.
  * @min_cbm_bits:		Minimum number of consecutive bits to be set
  *				in a cache bit mask
+ * @max_delay:			Max throttle delay. Delay is the hardware
+ *				understandable value for memory b/w.
+ * @min_bw:			Minimum memory bandwidth in percentage
+ *				user can request
+ * @bw_gran:			Bandwidth granularity
+ * @delay_linear:		True if Mem b/w delay is in linear scale
+ * @mb_map:			Mapping of mem b/w delay to
+ *				b/w throttle percentage
  * @domains:			All domains for this resource
  * @num_domains:		Number of domains active
  * @msr_base:			Base MSR address for CBMs

@@ -92,6 +103,11 @@ struct rdt_resource {
 	int			cbm_len;
 	int			min_cbm_bits;
 	u32			default_ctrl;
+	u32			max_delay;
+	u32			min_bw;
+	u32			bw_gran;
+	u32			delay_linear;
+	u32			*mb_map;
 	struct list_head	domains;
 	int			num_domains;
 	int			msr_base;

@@ -141,6 +157,7 @@ enum {
 	RDT_RESOURCE_L3DATA,
 	RDT_RESOURCE_L3CODE,
 	RDT_RESOURCE_L2,
+	RDT_RESOURCE_MBA,
 
 	/* Must be the last */
 	RDT_NUM_RESOURCES,

arch/x86/kernel/cpu/intel_rdt.c +82 ~~-13~~

135 lines not on this page · raw message

Signature · 1 line

1.9.1

Thomas Gleixner

2017-03-01 16:25 UTC #5 lore raw

Re: [PATCH 4/8] x86/intel_rct/mba: Add MBA structures and initialize MBA

arch/x86/include/asm/intel_rdt.h line 16

Quoted 9 lines · Vikas Shivappa

arch/x86/include/asm/intel_rdt.h +3 -0

@@ -11,6 +11,9 @@
 #define IA32_L3_QOS_CFG		0xc81
 #define IA32_L3_CBM_BASE	0xc90
 #define IA32_L2_CBM_BASE	0xd10
+#define IA32_MBA_THRTL_BASE	0xd50
+#define MAX_MBA_THRTL		100u
+#define MBA_IS_LINEAR		0x4

I have a hard time to figure out how the latter two constants are related to this list of registers. MBA_IS_LINEAR is used to check the CPUID bit and MAX_MBA_THRTL is obviously a pure software constant because with a non-linear scale the maximum value is not 100.

Just slapping defines to random places is equally bad as using hard coded constants.

arch/x86/include/asm/intel_rdt.h

Quoted 9 lines

arch/x86/include/asm/intel_rdt.h +9 -0

+/*
+ * rdt_get_mb_table() - get a mapping of b/w percentage values
+ * exposed to user interface and the h/w understandable delay values.
+ *
+ * The non-linear delay values have the granularity of power of two
+ * and also the h/w does not guarantee a curve for configured delay
+ * values vs. actual b/w throttled.
+ * Hence we need a mapping that is pre caliberated for user to express
+ * the b/w in terms of any sensible number.

... calibrated so the user can express the bandwidth as a percentage value.

arch/x86/include/asm/intel_rdt.h

Quoted 6 lines

arch/x86/include/asm/intel_rdt.h +6 -0

+static inline int rdt_get_mb_table(struct rdt_resource *r)
+{
+	/*
+	 * There are no Intel SKUs as of now to support non-linear delay.
+	 */
+	r->mb_map = NULL;

What's the point of setting this to NULL?

Also it would be helpful to emit log info here so people don't have to start digging around.

	pr_info("Bandwidth map not implemented for ....", ... model);

arch/x86/include/asm/intel_rdt.h

Quoted 2 lines

arch/x86/include/asm/intel_rdt.h +2 -0
+
+	return -ENODEV;

Returning -ENODEV to a function which just returns a boolean value is pointless.

arch/x86/include/asm/intel_rdt.h static inline bool get_rdt_resources(void) · line 241

Quoted 12 lines

static void rdt_get_cache_config(int idx, struct rdt_resource *r) {

arch/x86/include/asm/intel_rdt.h +2 -3

 	union cpuid_0x10_1_eax eax;

@@ -184,9 +237,8 @@ static inline bool get_rdt_resources(void)
 		ret = true;
 	}
 
-	if (boot_cpu_has(X86_FEATURE_MBA)) {
-		ret = true;
-	}
+	if (boot_cpu_has(X86_FEATURE_MBA))
+		ret = rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]);

Groan. When rdt_get_mem_config() returns false (because the map is not implemented), then the whole function returns false and CAT is disabled.

Quoted 10 lines

arch/x86/include/asm/intel_rdt.h +10 -0

+static int domain_setup_ctrlval(struct rdt_resource *r, struct rdt_domain *d)
+{
+	int i;
+
+	d->ctrl_val = kmalloc_array(r->num_closid,
+				     sizeof(*d->ctrl_val), GFP_KERNEL);
+	if (!d->ctrl_val)
+		return -ENOMEM;
+
+	/*

arch/x86/include/asm/intel_rdt.h static inline bool get_rdt_resources(void)

Quoted 10 lines

arch/x86/include/asm/intel_rdt.h +10 -0

+	 * Initialize the Control MSRs to having no control.
+	 * For Cache Allocation: Set all bits in cbm
+	 * For Memory Allocation: Set b/w requested to 100
+	 */
+	for (i = 0; i < r->num_closid; i++) {
+		int idx = cbm_idx(r, i);
+
+		d->ctrl_val[i] = r->default_ctrl;
+		wrmsrl(r->msr_base + idx, d->ctrl_val[i]);
+	}

So if you use a local pointer for that, this whole mess becomes readable.

static int domain_setup_ctrlval(struct rdt_resource *r, struct rdt_domain *d)

{
	u32 *p;
	int i;

	p = kmalloc_array(r->num_closid, sizeof(*d->ctrl_val), GFP_KERNEL);
	if (!p)
		return -ENOMEM;

	d->ctrl_val = p;

	/* Initialize the Control MSRs to the default value */
	for (i = 0; i < r->num_closid; i++, p++) {
		int idx = cbm_idx(r, i);

		*p = r->default_ctrl;
		wrmsrl(r->msr_base + idx, *p);
	}

arch/x86/include/asm/intel_rdt.h

Quoted 8 lines

arch/x86/include/asm/intel_rdt.h +4 -1

+
+	return 0;
+}
 
 static void domain_add_cpu(int cpu, struct rdt_resource *r)
 {
-	int i, id = get_cache_id(cpu, r->cache_level);
+	int id = get_cache_id(cpu, r->cache_level), ret;

Bah. If you have the same type in one line, then please move the uninitialized variables to the front.

	int ret, id = get_cache_id(cpu, r->cache_level);

But a s/i/ret/ would have been to simple and kept the code readable.

arch/x86/include/asm/intel_rdt.h static void domain_add_cpu(int cpu, struct rdt_resource *r) · line 381

Quoted 11 lines

arch/x86/include/asm/intel_rdt.h +2 -2

@@ -298,19 +374,12 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
 
 	d->id = id;
 
-	d->ctrl_val = kmalloc_array(r->num_closid, sizeof(*d->ctrl_val), GFP_KERNEL);
-	if (!d->ctrl_val) {
+	ret = domain_setup_ctrlval(r, d);
+	if (ret) {
 		kfree(d);
 		return;
 	}

What's the point of this 'ret' variable if the function is void?

	if (domain_setup_ctrlval(r, d)) {
		kfree(d);
		return;
	}

would have been to easy to read, right?

Thanks,

tglx

Shivappa Vikas

author

2017-03-10 21:50 UTC #6 lore raw

Re: [PATCH 4/8] x86/intel_rct/mba: Add MBA structures and initialize MBA

Quoted 138 lines · Thomas Gleixner

arch/x86/include/asm/intel_rdt.h line 16
Quoted 9 lines · Vikas Shivappa
arch/x86/include/asm/intel_rdt.h +3 -0
@@ -11,6 +11,9 @@
 #define IA32_L3_QOS_CFG		0xc81
 #define IA32_L3_CBM_BASE	0xc90
 #define IA32_L2_CBM_BASE	0xd10
+#define IA32_MBA_THRTL_BASE	0xd50
+#define MAX_MBA_THRTL		100u
+#define MBA_IS_LINEAR		0x4
I have a hard time to figure out how the latter two constants are related to this list of registers. MBA_IS_LINEAR is used to check the CPUID bit and MAX_MBA_THRTL is obviously a pure software constant because with a non-linear scale the maximum value is not 100.
Just slapping defines to random places is equally bad as using hard coded constants.
arch/x86/include/asm/intel_rdt.h
Quoted 9 lines
arch/x86/include/asm/intel_rdt.h +9 -0
+/*
+ * rdt_get_mb_table() - get a mapping of b/w percentage values
+ * exposed to user interface and the h/w understandable delay values.
+ *
+ * The non-linear delay values have the granularity of power of two
+ * and also the h/w does not guarantee a curve for configured delay
+ * values vs. actual b/w throttled.
+ * Hence we need a mapping that is pre caliberated for user to express
+ * the b/w in terms of any sensible number.
... calibrated so the user can express the bandwidth as a percentage value.
arch/x86/include/asm/intel_rdt.h
Quoted 6 lines
arch/x86/include/asm/intel_rdt.h +6 -0
+static inline int rdt_get_mb_table(struct rdt_resource *r)
+{
+	/*
+	 * There are no Intel SKUs as of now to support non-linear delay.
+	 */
+	r->mb_map = NULL;
What's the point of setting this to NULL?
Also it would be helpful to emit log info here so people don't have to start digging around.
	pr_info("Bandwidth map not implemented for ....", ... model);
arch/x86/include/asm/intel_rdt.h
Quoted 2 lines
arch/x86/include/asm/intel_rdt.h +2 -0
+
+	return -ENODEV;
Returning -ENODEV to a function which just returns a boolean value is pointless.
arch/x86/include/asm/intel_rdt.h static inline bool get_rdt_resources(void) · line 241
Quoted 12 lines
static void rdt_get_cache_config(int idx, struct rdt_resource *r) {
arch/x86/include/asm/intel_rdt.h +2 -3
 	union cpuid_0x10_1_eax eax;
@@ -184,9 +237,8 @@ static inline bool get_rdt_resources(void)
 		ret = true;
 	}
 
-	if (boot_cpu_has(X86_FEATURE_MBA)) {
-		ret = true;
-	}
+	if (boot_cpu_has(X86_FEATURE_MBA))
+		ret = rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]);
Groan. When rdt_get_mem_config() returns false (because the map is not implemented), then the whole function returns false and CAT is disabled.
Quoted 10 lines
arch/x86/include/asm/intel_rdt.h +10 -0
+static int domain_setup_ctrlval(struct rdt_resource *r, struct rdt_domain *d)
+{
+	int i;
+
+	d->ctrl_val = kmalloc_array(r->num_closid,
+				     sizeof(*d->ctrl_val), GFP_KERNEL);
+	if (!d->ctrl_val)
+		return -ENOMEM;
+
+	/*
arch/x86/include/asm/intel_rdt.h static inline bool get_rdt_resources(void)
Quoted 10 lines
arch/x86/include/asm/intel_rdt.h +10 -0
+	 * Initialize the Control MSRs to having no control.
+	 * For Cache Allocation: Set all bits in cbm
+	 * For Memory Allocation: Set b/w requested to 100
+	 */
+	for (i = 0; i < r->num_closid; i++) {
+		int idx = cbm_idx(r, i);
+
+		d->ctrl_val[i] = r->default_ctrl;
+		wrmsrl(r->msr_base + idx, d->ctrl_val[i]);
+	}
So if you use a local pointer for that, this whole mess becomes readable.
static int domain_setup_ctrlval(struct rdt_resource *r, struct rdt_domain *d)
{
	u32 *p;
	int i;

	p = kmalloc_array(r->num_closid, sizeof(*d->ctrl_val), GFP_KERNEL);
	if (!p)
		return -ENOMEM;

	d->ctrl_val = p;

	/* Initialize the Control MSRs to the default value */
	for (i = 0; i < r->num_closid; i++, p++) {
		int idx = cbm_idx(r, i);

		*p = r->default_ctrl;
		wrmsrl(r->msr_base + idx, *p);
	}
arch/x86/include/asm/intel_rdt.h
Quoted 8 lines
arch/x86/include/asm/intel_rdt.h +4 -1
+
+	return 0;
+}
 
 static void domain_add_cpu(int cpu, struct rdt_resource *r)
 {
-	int i, id = get_cache_id(cpu, r->cache_level);
+	int id = get_cache_id(cpu, r->cache_level), ret;
Bah. If you have the same type in one line, then please move the uninitialized variables to the front.
	int ret, id = get_cache_id(cpu, r->cache_level);
But a s/i/ret/ would have been to simple and kept the code readable.
arch/x86/include/asm/intel_rdt.h static void domain_add_cpu(int cpu, struct rdt_resource *r) · line 381
Quoted 11 lines
arch/x86/include/asm/intel_rdt.h +2 -2
@@ -298,19 +374,12 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
 
 	d->id = id;
 
-	d->ctrl_val = kmalloc_array(r->num_closid, sizeof(*d->ctrl_val), GFP_KERNEL);
-	if (!d->ctrl_val) {
+	ret = domain_setup_ctrlval(r, d);
+	if (ret) {
 		kfree(d);
 		return;
 	}
What's the point of this 'ret' variable if the function is void?

Quoted 6 lines

	if (domain_setup_ctrlval(r, d)) {
		kfree(d);
		return;
	}
would have been to easy to read, right?

Will fix all the issues pointed. Thanks for pointing out.

Quoted 5 lines

Thanks,
tglx

Vikas Shivappa

author

2017-02-17 19:58 UTC #7 lore raw

PATCH v2 5/8 x86/intel_rdt: info file support for MBA prepare

In reply to Vikas Shivappa

As a preparatory patch to MBA info file setup, generalize the info file setup to have the option to choose between different set of files. Although multiple cache resources have same info files, Memory resources have different set of info files. That way we have the option to choose between memory resource and cache resource info files.

Signed-off-by Vikas Shivappa <[email protected]>

---
 arch/x86/include/asm/intel_rdt.h         |  5 +++++
 arch/x86/kernel/cpu/intel_rdt.c          |  1 +
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 18 ++++++++++++++----
 3 files changed, 20 insertions(+), 4 deletions(-)

arch/x86/include/asm/intel_rdt.h +5 -0

index af65b2a..36abed2 100644
@@ -77,6 +77,8 @@ struct rftype {
  * @default_ctrl:		Specifies default cache cbm or mem b/w percent.
  * @min_cbm_bits:		Minimum number of consecutive bits to be set
  *				in a cache bit mask
+ * @info_files:		resctrl info files for the resource
+ * @infofiles_len:		Number of info files
  * @max_delay:			Max throttle delay. Delay is the hardware
  *				understandable value for memory b/w.
  * @min_bw:			Minimum memory bandwidth in percentage

@@ -103,6 +105,8 @@ struct rdt_resource {
 	int			cbm_len;
 	int			min_cbm_bits;
 	u32			default_ctrl;
+	struct rftype		*info_files;
+	int			infofiles_len;
 	u32			max_delay;
 	u32			min_bw;
 	u32			bw_gran;

@@ -144,6 +148,7 @@ struct msr_param {
 	int			high;
 };
 
+void rdt_get_cache_infofile(struct rdt_resource *r);
 extern struct mutex rdtgroup_mutex;
 
 extern struct rdt_resource rdt_resources_all[];

arch/x86/kernel/cpu/intel_rdt.c +1 -0

index 130ce98..481ff32 100644
@@ -193,6 +193,7 @@ static void rdt_get_cache_config(int idx, struct rdt_resource *r)
 	r->num_closid = edx.split.cos_max + 1;
 	r->cbm_len = eax.split.cbm_len + 1;
 	r->default_ctrl = BIT_MASK(eax.split.cbm_len + 1) - 1;
+	rdt_get_cache_infofile(r);
 	r->capable = true;
 	r->enabled = true;
 }

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +14 -4

index 0d8fa61..0a70e87 100644
@@ -519,7 +519,7 @@ static int rdt_min_cbm_bits_show(struct kernfs_open_file *of,
 }
 
 /* rdtgroup information files for one cache resource. */
-static struct rftype res_info_files[] = {
+static struct rftype res_cache_info_files[] = {
 	{
 		.name		= "num_closids",
 		.mode		= 0444,

@@ -540,11 +540,18 @@ static int rdt_min_cbm_bits_show(struct kernfs_open_file *of,
 	},
 };
 
+void rdt_get_cache_infofile(struct rdt_resource *r)
+{
+	r->info_files = &res_cache_info_files[0];
+	r->infofiles_len = ARRAY_SIZE(res_cache_info_files);
+}
+
 static int rdtgroup_create_info_dir(struct kernfs_node *parent_kn)
 {
 	struct kernfs_node *kn_subdir;
+	struct rftype *res_info_files;
 	struct rdt_resource *r;
-	int ret;
+	int ret, len;
 
 	/* create the directory */
 	kn_info = kernfs_create_dir(parent_kn, "info", parent_kn->mode, NULL);

@@ -563,8 +570,11 @@ static int rdtgroup_create_info_dir(struct kernfs_node *parent_kn)
 		ret = rdtgroup_kn_set_ugid(kn_subdir);
 		if (ret)
 			goto out_destroy;
-		ret = rdtgroup_add_files(kn_subdir, res_info_files,
-					 ARRAY_SIZE(res_info_files));
+
+		res_info_files = r->info_files;
+		len = r->infofiles_len;
+
+		ret = rdtgroup_add_files(kn_subdir, res_info_files, len);
 		if (ret)
 			goto out_destroy;
 		kernfs_activate(kn_subdir);

Signature · 1 line

1.9.1

Thomas Gleixner

2017-03-01 15:35 UTC #8 lore raw

Re: [PATCH 5/8] x86/intel_rdt: info file support for MBA prepare

Quoted 5 lines · Vikas Shivappa

As a preparatory patch to MBA info file setup, generalize the info file setup to have the option to choose between different set of files. Although multiple cache resources have same info files, Memory resources have different set of info files. That way we have the option to choose between memory resource and cache resource info files.

Sigh.

arch/x86/include/asm/intel_rdt.h struct rftype { · line 81

Quoted 6 lines

arch/x86/include/asm/intel_rdt.h +2 -0

@@ -77,6 +77,8 @@ struct rftype {
  * @default_ctrl:		Specifies default cache cbm or mem b/w percent.
  * @min_cbm_bits:		Minimum number of consecutive bits to be set
  *				in a cache bit mask
+ * @info_files:		resctrl info files for the resource
+ * @infofiles_len:		Number of info files

len == length, nr == number. No? Too intuitive, right?

And no, not infofiles_nr. It wants to be nr_infofiles. And while at it please either use infofiles or info_files, but not a mixture of both. Random underscores are not enhancing readability at all.

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static int rdt_min_cbm_bits_show(struct kernfs_open_file *of, · line 545

Quoted 3 lines

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +3 -0

+void rdt_get_cache_infofile(struct rdt_resource *r)
+{
+	r->info_files = &res_cache_info_files[0];

What's wrong with

        r->info_files = res_cache_info_files;

Nothing, but it would be too easy to read. This is not the obfuscated c-code contest.

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static int rdt_min_cbm_bits_show(struct kernfs_open_file *of, · line 546

Quoted 1 line

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +1 -0
+	r->infofiles_len = ARRAY_SIZE(res_cache_info_files);

Thanks,

tglx

Shivappa Vikas

author

2017-03-10 22:03 UTC #9 lore raw

Re: [PATCH 5/8] x86/intel_rdt: info file support for MBA prepare

Quoted 10 lines · Thomas Gleixner

Quoted 5 lines · Vikas Shivappa
As a preparatory patch to MBA info file setup, generalize the info file setup to have the option to choose between different set of files. Although multiple cache resources have same info files, Memory resources have different set of info files. That way we have the option to choose between memory resource and cache resource info files.
Sigh.

Quoted 8 lines

arch/x86/include/asm/intel_rdt.h struct rftype { · line 81
Quoted 6 lines
arch/x86/include/asm/intel_rdt.h +2 -0
@@ -77,6 +77,8 @@ struct rftype {
  * @default_ctrl:		Specifies default cache cbm or mem b/w percent.
  * @min_cbm_bits:		Minimum number of consecutive bits to be set
  *				in a cache bit mask
+ * @info_files:		resctrl info files for the resource
+ * @infofiles_len:		Number of info files
len == length, nr == number. No? Too intuitive, right?

Took it as len as the rdtgroup_add_files calls its parameter as len.. Will fix to nr_infofiles as that seems better.

Quoted 11 lines

And no, not infofiles_nr. It wants to be nr_infofiles. And while at it please either use infofiles or info_files, but not a mixture of both. Random underscores are not enhancing readability at all.
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static int rdt_min_cbm_bits_show(struct kernfs_open_file *of, · line 545
Quoted 3 lines
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +3 -0
+void rdt_get_cache_infofile(struct rdt_resource *r)
+{
+	r->info_files = &res_cache_info_files[0];
What's wrong with

Quoted 4 lines

       r->info_files = res_cache_info_files;
Nothing, but it would be too easy to read. This is not the obfuscated c-code contest.

Will fix..

Thanks,
Vikas

Quoted 7 lines

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static int rdt_min_cbm_bits_show(struct kernfs_open_file *of, · line 546
Quoted 2 lines
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +1 -0
+	r->infofiles_len = ARRAY_SIZE(res_cache_info_files);
Thanks,
tglx

Vikas Shivappa

author

2017-02-17 19:59 UTC #10 lore raw

PATCH v2 6/8 x86/intel_rdt/mba: Add info directory files for MBA

In reply to Vikas Shivappa

Add files in info directory for MBA.
The files in the info directory are as follows :
- num_closids: max number of closids for MBA which represents the max class of service user can configure.
- min_bw: the minimum memory bandwidth(b/w) values in percentage.

OS maps the b/w percentage values to memory b/w throttle delay values and configures them via MSR interface by writing to the QOS_MSRs. These delay values can have a linear or nonlinear scale.

- bw_gran: The memory b/w granularity that can be configured. For ex: If the granularity is 10% and min_bw is 10, valid bandwidth values are 10,20,30...

Signed-off-by Vikas Shivappa <[email protected]>

---
 arch/x86/include/asm/intel_rdt.h         |  2 +
 arch/x86/kernel/cpu/intel_rdt.c          |  1 +
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 64 ++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+)

arch/x86/include/asm/intel_rdt.h +2 -0

index 36abed2..24de64c 100644
@@ -149,6 +149,8 @@ struct msr_param {
 };
 
 void rdt_get_cache_infofile(struct rdt_resource *r);
+void rdt_get_mba_infofile(struct rdt_resource *r);
+
 extern struct mutex rdtgroup_mutex;
 
 extern struct rdt_resource rdt_resources_all[];

arch/x86/kernel/cpu/intel_rdt.c +1 -0

index 481ff32..353c476b4 100644
@@ -176,6 +176,7 @@ static bool rdt_get_mem_config(struct rdt_resource *r)
 		if (rdt_get_mb_table(r))
 			return false;
 	}
+	rdt_get_mba_infofile(r);
 
 	r->capable = true;
 	r->enabled = true;

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +64 -0

76 lines not on this page · raw message

Signature · 1 line

1.9.1

Thomas Gleixner

2017-03-01 16:25 UTC #11 lore raw

Re: [PATCH 6/8] x86/intel_rdt/mba: Add info directory files for MBA

Quoted 6 lines · Vikas Shivappa

Add files in info directory for MBA.
The files in the info directory are as follows :
- num_closids: max number of closids for MBA which represents the max class of service user can configure.
- min_bw: the minimum memory bandwidth(b/w) values in percentage.

Quoted 7 lines

OS maps the b/w percentage values to memory b/w throttle delay values and configures them via MSR interface by writing to the QOS_MSRs. These delay values can have a linear or nonlinear scale.
- bw_gran: The memory b/w granularity that can be configured. For ex: If the granularity is 10% and min_bw is 10, valid bandwidth values are 10,20,30...

This is unreadable. It's possible to structure ASCII text cleanly.

x86/rdt: Add info directory files for MBA

The info directory for MBA contains the following files:

num_closids

The maximum number of class of service slots available for MBA

min_bandwidth

The minimum memory bandwidth percentage value

bandwidth_gran

The granularity of the bandwidth control for the particular hardware in percent. The available bandwidth control steps are:
min_bw + N * bw_gran Intermediate values are rounded to the next control step available on the hardware.

delay_linear

If set, the control registers take a linear percentage based value between min_bandwidth and 100 percent.

If not set, the control registers take a power of 2 based value which is mapped by the kernel to percentage based values.

This file is of pure informational nature and has no influence on the values which are written to the schemata files. These are always percentage based.

Note, that this uses the actual file names and not some random abbreviations thereof. It also documents delay_linear and gets rid of the implementation details of QOS_MSRs. They are irrelevant here.

And exactly this information wants to go into Documentation/... preferably in exactly this patch and not in a disconnected one which describes stuff differently for whatever reasons.

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static int rdt_min_cbm_bits_show(struct kernfs_open_file *of, · line 527

Quoted 7 lines

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +7 -0

+static int rdt_min_bw_show(struct kernfs_open_file *of,
+			     struct seq_file *seq, void *v)
+{
+	struct rdt_resource *r = of->kn->parent->priv;
+
+	seq_printf(seq, "%d\n", r->min_bw);
+

Can you please get rid of these pointless extra new lines before the 'return 0;' ? They are just eating screen estate and do not make the code more readable.

Quoted 1 line

+/* rdtgroup information files for MBE. */

What is MBE?

Quoted 1 line

+static struct rftype res_mbe_info_files[] = {

Randomizing names make the code more secure or what are you trying to achieve?

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static int rdt_min_cbm_bits_show(struct kernfs_open_file *of, · line 603

Quoted 3 lines

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +3 -0

+void rdt_get_mba_infofile(struct rdt_resource *r)
+{
+	r->info_files = &res_mbe_info_files[0];

See other mail.

Thanks,

tglx

Shivappa Vikas

author

2017-03-10 23:25 UTC #12 lore raw

Re: [PATCH 6/8] x86/intel_rdt/mba: Add info directory files for MBA

Quoted 28 lines · Thomas Gleixner

Quoted 6 lines · Vikas Shivappa
Add files in info directory for MBA.
The files in the info directory are as follows :
- num_closids: max number of closids for MBA which represents the max class of service user can configure.
- min_bw: the minimum memory bandwidth(b/w) values in percentage.
Quoted 7 lines
OS maps the b/w percentage values to memory b/w throttle delay values and configures them via MSR interface by writing to the QOS_MSRs. These delay values can have a linear or nonlinear scale.
- bw_gran: The memory b/w granularity that can be configured. For ex: If the granularity is 10% and min_bw is 10, valid bandwidth values are 10,20,30...
This is unreadable. It's possible to structure ASCII text cleanly.
x86/rdt: Add info directory files for MBA
The info directory for MBA contains the following files:
num_closids
The maximum number of class of service slots available for MBA
min_bandwidth

Quoted 7 lines

The minimum memory bandwidth percentage value
bandwidth_gran
The granularity of the bandwidth control for the particular hardware in percent. The available bandwidth control steps are:
min_bw + N * bw_gran Intermediate values are rounded to the next

Is this next or pervious? Meaning when 12 is requested on a 10 granularity , we give 10 ?

Quoted 7 lines

control step available on the hardware.
delay_linear
If set, the control registers take a linear percentage based value between min_bandwidth and 100 percent.

Quoted 6 lines

If not set, the control registers take a power of 2 based value which is mapped by the kernel to percentage based values.
This file is of pure informational nature and has no influence on the values which are written to the schemata files. These are always percentage based.

Will update the changelogs

Quoted 9 lines

Note, that this uses the actual file names and not some random abbreviations thereof. It also documents delay_linear and gets rid of the implementation details of QOS_MSRs. They are irrelevant here.
And exactly this information wants to go into Documentation/... preferably in exactly this patch and not in a disconnected one which describes stuff differently for whatever reasons.

Quoted 11 lines

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static int rdt_min_cbm_bits_show(struct kernfs_open_file *of, · line 527
Quoted 7 lines
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +7 -0
+static int rdt_min_bw_show(struct kernfs_open_file *of,
+			     struct seq_file *seq, void *v)
+{
+	struct rdt_resource *r = of->kn->parent->priv;
+
+	seq_printf(seq, "%d\n", r->min_bw);
+
Can you please get rid of these pointless extra new lines before the 'return 0;' ? They are just eating screen estate and do not make the code more readable.

All the rest of the show functions have that line before return like the rdt_min_cbm_bits_show etc. I have tried to always keep a line before return like the old cqm/rapl - if thats ok

Quoted 7 lines

Quoted 2 lines
+/* rdtgroup information files for MBE. */
What is MBE?
Quoted 1 line
+static struct rftype res_mbe_info_files[] = {

Quoted 9 lines

Randomizing names make the code more secure or what are you trying to achieve?
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c static int rdt_min_cbm_bits_show(struct kernfs_open_file *of, · line 603
Quoted 3 lines
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +3 -0
+void rdt_get_mba_infofile(struct rdt_resource *r)
+{
+	r->info_files = &res_mbe_info_files[0];
See other mail.

Will fix the MBE to MBA and the pointer init.

Thanks,
Vikas

Quoted 4 lines

Thanks,
tglx

Vikas Shivappa

author

2017-02-17 19:58 UTC #13 lore raw

PATCH v2 7/8 x86/intel_rdt: schemata file support for MBA prepare

In reply to Vikas Shivappa

Add support to introduce generic APIs for control validation and writing QOS_MSRs for RDT resources. The control validation api is meant to validate the control values like cache bit mask for CAT and memory b/w percentage for MBA. A resource generic display format is also added and used for the resources depending on whether its displayed in hex/decimal.

Signed-off-by Vikas Shivappa <[email protected]>

---
 arch/x86/include/asm/intel_rdt.h         |  8 +++++++
 arch/x86/kernel/cpu/intel_rdt.c          | 33 +++++++++++++++++++++-----
 arch/x86/kernel/cpu/intel_rdt_schemata.c | 40 ++++++++++++++++++--------------
 3 files changed, 58 insertions(+), 23 deletions(-)

arch/x86/include/asm/intel_rdt.h +8 -0

index 24de64c..8748b0d 100644
@@ -77,6 +77,9 @@ struct rftype {
  * @default_ctrl:		Specifies default cache cbm or mem b/w percent.
  * @min_cbm_bits:		Minimum number of consecutive bits to be set
  *				in a cache bit mask
+ * @format_str:		Per resource format string to show domain val
+ * @parse_ctrlval:		Per resource API to parse the ctrl values
+ * @msr_update:		API to update QOS MSRs
  * @info_files:		resctrl info files for the resource
  * @infofiles_len:		Number of info files
  * @max_delay:			Max throttle delay. Delay is the hardware

@@ -105,6 +108,9 @@ struct rdt_resource {
 	int			cbm_len;
 	int			min_cbm_bits;
 	u32			default_ctrl;
+	const char		*format_str;
+	int (*parse_ctrlval)	(char *buf, struct rdt_resource *r);
+	void (*msr_update)	(void *a1, void *a2, struct rdt_resource *r);
 	struct rftype		*info_files;
 	int			infofiles_len;
 	u32			max_delay;

@@ -150,6 +156,8 @@ struct msr_param {
 
 void rdt_get_cache_infofile(struct rdt_resource *r);
 void rdt_get_mba_infofile(struct rdt_resource *r);
+int parse_cbm(char *buf, struct rdt_resource *r);
+void cqm_wrmsr(void *a1, void *a2, struct rdt_resource *r);
 
 extern struct mutex rdtgroup_mutex;

arch/x86/kernel/cpu/intel_rdt.c +27 -6

index 353c476b4..7ce4453 100644
@@ -44,6 +44,9 @@ struct rdt_resource rdt_resources_all[] = {
 		.name		= "L3",
 		.domains	= domain_init(RDT_RESOURCE_L3),
 		.msr_base	= IA32_L3_CBM_BASE,
+		.parse_ctrlval	= parse_cbm,
+		.msr_update	= cqm_wrmsr,
+		.format_str	= "%d=%x",
 		.min_cbm_bits	= 1,
 		.cache_level	= 3,
 		.cbm_idx_multi	= 1,

@@ -53,6 +56,9 @@ struct rdt_resource rdt_resources_all[] = {
 		.name		= "L3DATA",
 		.domains	= domain_init(RDT_RESOURCE_L3DATA),
 		.msr_base	= IA32_L3_CBM_BASE,
+		.parse_ctrlval	= parse_cbm,
+		.msr_update	= cqm_wrmsr,
+		.format_str	= "%d=%x",
 		.min_cbm_bits	= 1,
 		.cache_level	= 3,
 		.cbm_idx_multi	= 2,

@@ -62,6 +68,9 @@ struct rdt_resource rdt_resources_all[] = {
 		.name		= "L3CODE",
 		.domains	= domain_init(RDT_RESOURCE_L3CODE),
 		.msr_base	= IA32_L3_CBM_BASE,
+		.parse_ctrlval	= parse_cbm,
+		.msr_update	= cqm_wrmsr,
+		.format_str	= "%d=%x",
 		.min_cbm_bits	= 1,
 		.cache_level	= 3,
 		.cbm_idx_multi	= 2,

@@ -71,6 +80,9 @@ struct rdt_resource rdt_resources_all[] = {
 		.name		= "L2",
 		.domains	= domain_init(RDT_RESOURCE_L2),
 		.msr_base	= IA32_L2_CBM_BASE,
+		.parse_ctrlval	= parse_cbm,
+		.msr_update	= cqm_wrmsr,
+		.format_str	= "%d=%x",
 		.min_cbm_bits	= 1,
 		.cache_level	= 2,
 		.cbm_idx_multi	= 1,

@@ -258,11 +270,24 @@ static int get_cache_id(int cpu, int level)
 	return -1;
 }
 
+void cqm_wrmsr(void *a1, void *a2, struct rdt_resource *r)
+{
+	struct rdt_domain *d = (struct rdt_domain *)a2;
+	struct msr_param *m = (struct msr_param *)a1;
+	int i;
+
+	for (i = m->low; i < m->high; i++) {
+		int idx = cbm_idx(r, i);
+
+		wrmsrl(r->msr_base + idx, d->ctrl_val[i]);
+	}
+}
+
 void rdt_ctrl_update(void *arg)
 {
 	struct msr_param *m = (struct msr_param *)arg;
 	struct rdt_resource *r = m->res;
-	int i, cpu = smp_processor_id();
+	int cpu = smp_processor_id();
 	struct rdt_domain *d;
 
 	list_for_each_entry(d, &r->domains, list) {

@@ -276,11 +301,7 @@ void rdt_ctrl_update(void *arg)
 	return;
 
 found:
-	for (i = m->low; i < m->high; i++) {
-		int idx = cbm_idx(r, i);
-
-		wrmsrl(r->msr_base + idx, d->ctrl_val[i]);
-	}
+	r->msr_update(m, d, r);
 }
 
 /*

arch/x86/kernel/cpu/intel_rdt_schemata.c +23 ~~-17~~

index 3cde1e8..4141662 100644
@@ -34,41 +34,46 @@
  *	are allowed (e.g. FFFFH, 0FF0H, 003CH, etc.).
  * Additionally Haswell requires at least two bits set.
  */
-static bool cbm_validate(unsigned long var, struct rdt_resource *r)
+static int cbm_validate(char *buf, unsigned long *data, struct rdt_resource *r)
 {
-	unsigned long first_bit, zero_bit;
+	unsigned long first_bit, zero_bit, var;
+	int ret;
+
+	ret = kstrtoul(buf, 16, &var);
+	if (ret)
+		return ret;
 
 	if (var == 0 || var > r->default_ctrl)
-		return false;
+		return -EINVAL;
 
 	first_bit = find_first_bit(&var, r->cbm_len);
 	zero_bit = find_next_zero_bit(&var, r->cbm_len, first_bit);
 
 	if (find_next_bit(&var, r->cbm_len, zero_bit) < r->cbm_len)
-		return false;
+		return -EINVAL;
 
 	if ((zero_bit - first_bit) < r->min_cbm_bits)
-		return false;
-	return true;
+		return -EINVAL;
+
+	*data = var;
+
+	return 0;
 }
 
 /*
- * Read one cache bit mask (hex). Check that it is valid for the current
- * resource type.
+ * Read the user RDT control value into tempory buffer:
+ * Cache bit mask (hex) or Memory b/w throttle (decimal).
+ * Check that it is valid for the current resource type.
  */
-static int parse_cbm(char *buf, struct rdt_resource *r)
+int parse_cbm(char *buf, struct rdt_resource *r)
 {
 	unsigned long data;
 	int ret;
 
-	ret = kstrtoul(buf, 16, &data);
-	if (ret)
-		return ret;
-	if (!cbm_validate(data, r))
-		return -EINVAL;
+	ret = cbm_validate(buf, &data, r);
 	r->tmp_ctrl[r->num_tmp_ctrl++] = data;
 
-	return 0;
+	return ret;
 }
 
 /*

@@ -90,7 +95,7 @@ static int parse_line(char *line, struct rdt_resource *r)
 		id = strsep(&dom, "=");
 		if (kstrtoul(id, 10, &dom_id) || dom_id != d->id)
 			return -EINVAL;
-		if (parse_cbm(dom, r))
+		if (r->parse_ctrlval(dom, r))
 			return -EINVAL;
 	}

@@ -240,7 +245,8 @@ static void show_doms(struct seq_file *s, struct rdt_resource *r, int closid)
 	list_for_each_entry(dom, &r->domains, list) {
 		if (sep)
 			seq_puts(s, ";");
-		seq_printf(s, "%d=%x", dom->id, dom->ctrl_val[closid]);
+
+		seq_printf(s, r->format_str, dom->id, dom->ctrl_val[closid]);
 		sep = true;
 	}
 	seq_puts(s, "\n");

Signature · 1 line

1.9.1

Thomas Gleixner

2017-03-01 16:25 UTC #14 lore raw

Re: [PATCH 7/8] x86/intel_rdt: schemata file support for MBA prepare\

On Fri, 17 Feb 2017, Vikas Shivappa wrote:

Subject: x86/intel_rdt: schemata file support for MBA prepare

I have no idea what MBA prepare is. Is that yet another variant aside of MBE?

Quoted 6 lines

Add support to introduce generic APIs for control validation and writing QOS_MSRs for RDT resources. The control validation api is meant to validate the control values like cache bit mask for CAT and memory b/w percentage for MBA. A resource generic display format is also added and used for the resources depending on whether its displayed in hex/decimal.

The usual unpenetratable mess of random sentences lumped together.

arch/x86/include/asm/intel_rdt.h struct rftype { · line 80

Quoted 9 lines

arch/x86/include/asm/intel_rdt.h +1 -0

index 24de64c..8748b0d 100644
@@ -77,6 +77,9 @@ struct rftype {
  * @default_ctrl:		Specifies default cache cbm or mem b/w percent.
  * @min_cbm_bits:		Minimum number of consecutive bits to be set
  *				in a cache bit mask
+ * @format_str:		Per resource format string to show domain val

Can you please spell out words in comments instead of using random abbreviations just as you see fit?

arch/x86/include/asm/intel_rdt.h struct rftype { · line 81

Quoted 1 line

arch/x86/include/asm/intel_rdt.h +1 -0
+ * @parse_ctrlval:		Per resource API to parse the ctrl values

That's not an API. That's a function pointer.

arch/x86/include/asm/intel_rdt.h struct rftype { · line 82

Quoted 1 line

arch/x86/include/asm/intel_rdt.h +1 -0
+ * @msr_update:		API to update QOS MSRs

Ditto.

arch/x86/include/asm/intel_rdt.h struct rdt_resource { · line 113

Quoted 10 lines

arch/x86/include/asm/intel_rdt.h +3 -0

  * @info_files:		resctrl info files for the resource
  * @infofiles_len:		Number of info files
  * @max_delay:			Max throttle delay. Delay is the hardware

@@ -105,6 +108,9 @@ struct rdt_resource {
 	int			cbm_len;
 	int			min_cbm_bits;
 	u32			default_ctrl;
+	const char		*format_str;
+	int (*parse_ctrlval)	(char *buf, struct rdt_resource *r);
+	void (*msr_update)	(void *a1, void *a2, struct rdt_resource *r);

void *a1, *a2? Dammit, both implementations (CAT and MBA) use the same types. This just avoids type safety which does not magically come back by your completely nonsensical typecasts inside the callback implementations.

arch/x86/include/asm/intel_rdt.h struct rdt_resource { · line 114

Quoted 1 line

arch/x86/include/asm/intel_rdt.h +1 -0
+void cqm_wrmsr(void *a1, void *a2, struct rdt_resource *r)

And this is global because it's only used in this file, right?

arch/x86/include/asm/intel_rdt.h struct rdt_resource {

Quoted 3 lines

arch/x86/include/asm/intel_rdt.h +3 -0

+{
+	struct rdt_domain *d = (struct rdt_domain *)a2;
+	struct msr_param *m = (struct msr_param *)a1;

Oh well......

Quoted 13 lines

arch/x86/include/asm/intel_rdt.h +9 -2

+	int i;
+
+	for (i = m->low; i < m->high; i++) {
+		int idx = cbm_idx(r, i);
+
+		wrmsrl(r->msr_base + idx, d->ctrl_val[i]);
+	}
+}
 
-static bool cbm_validate(unsigned long var, struct rdt_resource *r)
+static int cbm_validate(char *buf, unsigned long *data, struct rdt_resource *r)
 {
-	unsigned long first_bit, zero_bit;

arch/x86/include/asm/intel_rdt.h

Quoted 10 lines

arch/x86/include/asm/intel_rdt.h +7 -1

+	unsigned long first_bit, zero_bit, var;
+	int ret;
+
+	ret = kstrtoul(buf, 16, &var);
+	if (ret)
+		return ret;
 
 	if (var == 0 || var > r->default_ctrl)
-		return false;
+		return -EINVAL;

So you change this function and the whole call chain to return either -EINVAL or 0 instead of false/true.

And then you treat the integer return value as boolean on the call site again:

arch/x86/include/asm/intel_rdt.h static int parse_line(char *line, struct rdt_resource *r) · line 99

Quoted 7 lines

arch/x86/include/asm/intel_rdt.h +1 -1

@@ -90,7 +95,7 @@ static int parse_line(char *line, struct rdt_resource *r)
 		id = strsep(&dom, "=");
 		if (kstrtoul(id, 10, &dom_id) || dom_id != d->id)
 			return -EINVAL;
-		if (parse_cbm(dom, r))
+		if (r->parse_ctrlval(dom, r))
 			return -EINVAL;

What's the purpose of this exercise? Annoying reviewers or what?

Thanks,

tglx

Shivappa Vikas

2017-03-30 19:05 UTC #15 lore raw

Re: [PATCH 7/8] x86/intel_rdt: schemata file support for MBA prepare\

Quoted 89 lines · Thomas Gleixner

On Fri, 17 Feb 2017, Vikas Shivappa wrote:
Subject: x86/intel_rdt: schemata file support for MBA prepare
I have no idea what MBA prepare is. Is that yet another variant aside of MBE?
Quoted 6 lines
Add support to introduce generic APIs for control validation and writing QOS_MSRs for RDT resources. The control validation api is meant to validate the control values like cache bit mask for CAT and memory b/w percentage for MBA. A resource generic display format is also added and used for the resources depending on whether its displayed in hex/decimal.
The usual unpenetratable mess of random sentences lumped together.
arch/x86/include/asm/intel_rdt.h struct rftype { · line 80
Quoted 9 lines
arch/x86/include/asm/intel_rdt.h +1 -0
index 24de64c..8748b0d 100644
@@ -77,6 +77,9 @@ struct rftype {
  * @default_ctrl:		Specifies default cache cbm or mem b/w percent.
  * @min_cbm_bits:		Minimum number of consecutive bits to be set
  *				in a cache bit mask
+ * @format_str:		Per resource format string to show domain val
Can you please spell out words in comments instead of using random abbreviations just as you see fit?
arch/x86/include/asm/intel_rdt.h struct rftype { · line 81
Quoted 1 line
arch/x86/include/asm/intel_rdt.h +1 -0
+ * @parse_ctrlval:		Per resource API to parse the ctrl values
That's not an API. That's a function pointer.
arch/x86/include/asm/intel_rdt.h struct rftype { · line 82
Quoted 1 line
arch/x86/include/asm/intel_rdt.h +1 -0
+ * @msr_update:		API to update QOS MSRs
Ditto.
arch/x86/include/asm/intel_rdt.h struct rdt_resource { · line 113
Quoted 10 lines
arch/x86/include/asm/intel_rdt.h +3 -0
  * @info_files:		resctrl info files for the resource
  * @infofiles_len:		Number of info files
  * @max_delay:			Max throttle delay. Delay is the hardware
@@ -105,6 +108,9 @@ struct rdt_resource {
 	int			cbm_len;
 	int			min_cbm_bits;
 	u32			default_ctrl;
+	const char		*format_str;
+	int (*parse_ctrlval)	(char *buf, struct rdt_resource *r);
+	void (*msr_update)	(void *a1, void *a2, struct rdt_resource *r);
void *a1, *a2? Dammit, both implementations (CAT and MBA) use the same types. This just avoids type safety which does not magically come back by your completely nonsensical typecasts inside the callback implementations.
arch/x86/include/asm/intel_rdt.h struct rdt_resource { · line 114
Quoted 1 line
arch/x86/include/asm/intel_rdt.h +1 -0
+void cqm_wrmsr(void *a1, void *a2, struct rdt_resource *r)
And this is global because it's only used in this file, right?
arch/x86/include/asm/intel_rdt.h struct rdt_resource {
Quoted 3 lines
arch/x86/include/asm/intel_rdt.h +3 -0
+{
+	struct rdt_domain *d = (struct rdt_domain *)a2;
+	struct msr_param *m = (struct msr_param *)a1;
Oh well......
Quoted 13 lines
arch/x86/include/asm/intel_rdt.h +9 -2
+	int i;
+
+	for (i = m->low; i < m->high; i++) {
+		int idx = cbm_idx(r, i);
+
+		wrmsrl(r->msr_base + idx, d->ctrl_val[i]);
+	}
+}
 
-static bool cbm_validate(unsigned long var, struct rdt_resource *r)
+static int cbm_validate(char *buf, unsigned long *data, struct rdt_resource *r)
 {
-	unsigned long first_bit, zero_bit;
arch/x86/include/asm/intel_rdt.h
Quoted 10 lines
arch/x86/include/asm/intel_rdt.h +7 -1
+	unsigned long first_bit, zero_bit, var;
+	int ret;
+
+	ret = kstrtoul(buf, 16, &var);
+	if (ret)
+		return ret;
 
 	if (var == 0 || var > r->default_ctrl)
-		return false;
+		return -EINVAL;
So you change this function and the whole call chain to return either -EINVAL or 0 instead of false/true.

Quoted 2 lines

And then you treat the integer return value as boolean on the call site again:

Will fix wrt all the comments.

Thanks,
Vikas

Vikas Shivappa

author

2017-02-17 19:58 UTC #16 lore raw

PATCH v2 8/8 x86/intel_rdt/mba: Add schemata file support for MBA

In reply to Vikas Shivappa

Add support to update the MBA bandwidth values for the domains. The MBA bandwidth values are specified for each domain by updating the schemata. The schemata string is parsed and validated for the correct bandwidth values for meeting the minimum b/w and b/w granularity requirements. OS updates the corresponding domain PQOS_MSRs which are indexed from 0xD50 for MBA.

For linear scale the granularity is just the 100-max_delay. For non-linear values, it is obtained by the mapping between the delay values and percentage b/w values.

Signed-off-by Vikas Shivappa <[email protected]>

---
 arch/x86/include/asm/intel_rdt.h         |  3 +++
 arch/x86/kernel/cpu/intel_rdt.c          | 35 ++++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/intel_rdt_schemata.c | 37 ++++++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+)

arch/x86/include/asm/intel_rdt.h +3 -0

index 8748b0d..869fee1 100644
@@ -13,6 +13,7 @@
 #define IA32_L2_CBM_BASE	0xd10
 #define IA32_MBA_THRTL_BASE	0xd50
 #define MAX_MBA_THRTL		100u
+#define INVALID_DELAY		100u
 #define MBA_IS_LINEAR		0x4
 
 #define L3_QOS_CDP_ENABLE	0x01ULL

@@ -157,7 +158,9 @@ struct msr_param {
 void rdt_get_cache_infofile(struct rdt_resource *r);
 void rdt_get_mba_infofile(struct rdt_resource *r);
 int parse_cbm(char *buf, struct rdt_resource *r);
+int parse_thrtl(char *buf, struct rdt_resource *r);
 void cqm_wrmsr(void *a1, void *a2, struct rdt_resource *r);
+void mba_wrmsr(void *a1, void *a2, struct rdt_resource *r);
 
 extern struct mutex rdtgroup_mutex;

arch/x86/kernel/cpu/intel_rdt.c +35 -0

index 7ce4453..c0e56a7 100644
@@ -92,6 +92,9 @@ struct rdt_resource rdt_resources_all[] = {
 		.name		= "MB",
 		.domains	= domain_init(RDT_RESOURCE_MBA),
 		.msr_base	= IA32_MBA_THRTL_BASE,
+		.parse_ctrlval	= parse_thrtl,
+		.msr_update	= mba_wrmsr,
+		.format_str	= "%d=%d",
 		.cache_level	= 3,
 		.cbm_idx_multi	= 1,
 		.cbm_idx_offset = 0

@@ -151,6 +154,19 @@ static inline bool cache_alloc_hsw_probe(void)
 }
 
 /*
+ * Map the memory b/w percentage value to delay values
+ * that can be written to QOS_MSRs.
+ * There are currently no SKUs which support non linear delay values.
+ */
+static u32 delay_bw_map(unsigned long bw, struct rdt_resource *r)
+{
+	if (r->delay_linear)
+		return MAX_MBA_THRTL - bw;
+
+	return INVALID_DELAY;
+}
+
+/*
  * rdt_get_mb_table() - get a mapping of b/w percentage values
  * exposed to user interface and the h/w understandable delay values.
  *

@@ -270,6 +286,25 @@ static int get_cache_id(int cpu, int level)
 	return -1;
 }
 
+void mba_wrmsr(void *a1, void *a2, struct rdt_resource *r)
+{
+	struct rdt_domain *d = (struct rdt_domain *)a2;
+	struct msr_param *m = (struct msr_param *)a1;
+	int i;
+
+	for (i = m->low; i < m->high; i++) {
+		int idx = cbm_idx(r, i);
+
+		/*
+		 * Write the delay value for mba.
+		 * delay_bw_map will return a correct value
+		 * for this call as a nonexistant map throws error
+		 * on init.
+		 */
+		wrmsrl(r->msr_base + idx, delay_bw_map(d->ctrl_val[i], r));
+	}
+}
+
 void cqm_wrmsr(void *a1, void *a2, struct rdt_resource *r)
 {
 	struct rdt_domain *d = (struct rdt_domain *)a2;

arch/x86/kernel/cpu/intel_rdt_schemata.c +37 -0

index 4141662..1c016c2 100644
@@ -29,6 +29,43 @@
 #include <asm/intel_rdt.h>
 
 /*
+ * Check whether MBA bandwidth percentage value is correct.
+ * The value is checked against the minimum bandwidth
+ * values and the b/w granularity specified by the h/w.
+ */
+static int thrtl_validate(char *buf, unsigned long *data, struct rdt_resource *r)
+{
+	unsigned long bw;
+	int ret;
+
+	ret = kstrtoul(buf, 10, &bw);
+	if (ret)
+		return ret;
+
+	if (bw < r->min_bw || bw > MAX_MBA_THRTL || (bw % r->bw_gran))
+		return -EINVAL;
+	*data = bw;
+
+	return 0;
+}
+
+/*
+ * Read the user RDT control value into tempory buffer:
+ * Cache bit mask (hex) or Memory b/w throttle (decimal).
+ * Check that it is valid for the current resource type.
+ */
+int parse_thrtl(char *buf, struct rdt_resource *r)
+{
+	unsigned long data;
+	int ret;
+
+	ret = thrtl_validate(buf, &data, r);
+	r->tmp_ctrl[r->num_tmp_ctrl++] = data;
+
+	return ret;
+}
+
+/*
  * Check whether a cache bit mask is valid. The SDM says:
  *	Please note that all (and only) contiguous '1' combinations
  *	are allowed (e.g. FFFFH, 0FF0H, 003CH, etc.).

Signature · 1 line

1.9.1

Thomas Gleixner

2017-03-01 16:59 UTC #17 lore raw

Re: [PATCH 8/8] x86/intel_rdt/mba: Add schemata file support for MBA

arch/x86/include/asm/intel_rdt.h line 16

Quoted 5 lines · Vikas Shivappa

arch/x86/include/asm/intel_rdt.h +1 -0

@@ -13,6 +13,7 @@
 #define IA32_L2_CBM_BASE	0xd10
 #define IA32_MBA_THRTL_BASE	0xd50
 #define MAX_MBA_THRTL		100u
+#define INVALID_DELAY		100u

100% is an interesting definition of invalid.

arch/x86/include/asm/intel_rdt.h struct msr_param { · line 161

Quoted 8 lines

arch/x86/include/asm/intel_rdt.h +1 -0

 #define MBA_IS_LINEAR		0x4
 
 #define L3_QOS_CDP_ENABLE	0x01ULL

@@ -157,7 +158,9 @@ struct msr_param {
 void rdt_get_cache_infofile(struct rdt_resource *r);
 void rdt_get_mba_infofile(struct rdt_resource *r);
 int parse_cbm(char *buf, struct rdt_resource *r);
+int parse_thrtl(char *buf, struct rdt_resource *r);

Bah. The resource is MBA not 'thrtl'. Consistent naming is NOT optional.

arch/x86/include/asm/intel_rdt.h struct msr_param { · line 163

Quoted 2 lines

arch/x86/include/asm/intel_rdt.h +1 -0

 void cqm_wrmsr(void *a1, void *a2, struct rdt_resource *r);
+void mba_wrmsr(void *a1, void *a2, struct rdt_resource *r);

Again this is global, because it's only used in intel_rdt.c, right?

arch/x86/kernel/cpu/intel_rdt.c static inline bool cache_alloc_hsw_probe(void) · line 164

Quoted 9 lines

arch/x86/kernel/cpu/intel_rdt.c +8 -0

 /*
+ * Map the memory b/w percentage value to delay values
+ * that can be written to QOS_MSRs.
+ * There are currently no SKUs which support non linear delay values.
+ */
+static u32 delay_bw_map(unsigned long bw, struct rdt_resource *r)
+{
+	if (r->delay_linear)
+		return MAX_MBA_THRTL - bw;

Bah. Why do you have r->default_ctrl? Just to hard code stuff in places?

arch/x86/kernel/cpu/intel_rdt.c static inline bool cache_alloc_hsw_probe(void) · line 166

Quoted 2 lines

arch/x86/kernel/cpu/intel_rdt.c +2 -0
+
+	return INVALID_DELAY;

This is crap. If the code reaches this point, then the map has been initialized. If the map has not been initialized, but delay_linear is 0 then that's a BUG and we should handle it gracefully.

It's fine to prepare for the eventual arrival of this non linear nonsense, but then we do it proper.

	if (r->delay_linear)
		return r->default_ctrl - bw;

	if (!r->bw_map) {
		WARN_ONCE("Sensible error message");
		return r->default_ctrl;
	}
	WARN_ONCE("Sensible error message");
	return r->default_ctrl;

Whether this needs to be a WARN_ONCE() or a pr_err_once() is debatable as the call chain is known and the only value of the backtrace is that it is more prominent in dmesg.

arch/x86/kernel/cpu/intel_rdt.c static int get_cache_id(int cpu, int level) · line 300

Quoted 12 lines

arch/x86/kernel/cpu/intel_rdt.c +12 -0

+void mba_wrmsr(void *a1, void *a2, struct rdt_resource *r)
+{
+	struct rdt_domain *d = (struct rdt_domain *)a2;
+	struct msr_param *m = (struct msr_param *)a1;
+	int i;
+
+	for (i = m->low; i < m->high; i++) {
+		int idx = cbm_idx(r, i);
+
+		/*
+		 * Write the delay value for mba.
+		 * delay_bw_map will return a correct value

If you use function names in a comment then please do it proper:
delay_bw_map()

arch/x86/kernel/cpu/intel_rdt.c static int get_cache_id(int cpu, int level) · line 302

Quoted 2 lines

arch/x86/kernel/cpu/intel_rdt.c +2 -0

+		 * for this call as a nonexistant map throws error
+		 * on init.

Oh well. Adding nonsensical comments is way simpler than thinking about proper implementations, right?

The function as you wrote it does not return a correct value under all circumstances and the nonexistant map does not throw an error at all. We simply should not reach that code when the map does not exist.

Quoted 6 lines

/*
+ * Check whether MBA bandwidth percentage value is correct.
+ * The value is checked against the minimum bandwidth
+ * values and the b/w granularity specified by the h/w.
+ */
+static int thrtl_validate(char *buf, unsigned long *data, struct rdt_resource *r)

Darn. The input value is not a throttle value. Read the comment you wrote yourself. It's a bandwidth percentage. So why the heck can't you make the function name reflect what it does?

arch/x86/kernel/cpu/intel_rdt_schemata.c line 45

Quoted 9 lines

arch/x86/kernel/cpu/intel_rdt_schemata.c +9 -0

+{
+	unsigned long bw;
+	int ret;
+
+	ret = kstrtoul(buf, 10, &bw);
+	if (ret)
+		return ret;
+
+	if (bw < r->min_bw || bw > MAX_MBA_THRTL || (bw % r->bw_gran))

I told you last time, that requiring exact matches is bad because you can't use the same settings across different machines. It does not matter whether it's accurate simply because the actual outcome in the hardware is not accurate either. So the only interesting thing is that:

	 r->min_bw <= val <= MAX_MBA_BW

Yes, it's not MAX_MBA_THRTL, it's MAX_MBA_BW, which is always 100%. Please fix that all over the place.

In principle, we could even relax the requirement to:

        0 <= val <= MAX_MBA_BW

and set the value to r->min_bw if it's smaller.

And of course this granularity check is completely bogus when non-linear mappings come into play. So while you have plastered the rest of the code with bogus handling of the non-linear stuff, you ignore it here.

A simple

	if (!r->delay_linear)
		return -ENOTSUPP;

would be the proper thing to do.

Quoted 3 lines

+/*
+ * Read the user RDT control value into tempory buffer:
+ * Cache bit mask (hex) or Memory b/w throttle (decimal).

That's a really valuable comment for this function. NOT!

I really wonder how you manage to not confuse yourself with the inconsistent mess you create all over the place. Maybe you do....

Thanks,

tglx