x86/intel_rdt: Memory b/w Allocation support

Memory b/w allocation is part of Intel RDT(resource director technology) which lets user control the amount of memory b/w (L2 external b/w) per thread. This is done programming MSR interfaces like cache allocation technology and other RDT features.
This patch adds documentation for Memory b/w allocation interface usage.

+Memory b/w throttle

Quoted 5 lines · Vikas Shivappa

Memory b/w allocation is part of Intel RDT(resource director technology) which lets user control the amount of memory b/w (L2 external b/w) per thread. This is done programming MSR interfaces like cache allocation technology and other RDT features.
This patch adds documentation for Memory b/w allocation interface usage.

Sigh. I told you how often that 'This patch' is crap. We already know that this is a patch. Read and finally act according to Documentation/process/SubmittingPatches

Quoted 1 line

+Memory b/w throttle

Can we please spell out Bandwidth at least once? b/w can mean anything (black/white, both ways ...)

Documentation/x86/intel_rdt_ui.txt and 0xA are not. On a system with a 20-bit mask each bit represents 5% · line 123

Quoted 12 lines

+-------------------

Documentation/x86/intel_rdt_ui.txt +11 -0

+For Memory b/w resource, the portion of total memory b/w the user can
+restrict or 'throttle by' is indicated by the thrtl_by values.
+
+Throttle by values could be linear scale or non-linear scale.  In linear
+scale a thrtl_by value of say 20 would throttle the memory b/w by 20%
+allowing only 80% max b/w. In nonlinear scale currently SDM specifies
+throttle values in 2^n values. However the h/w does not guarantee a
+specific curve for the amount of memory b/w that is actually throttled.
+But for any thrtl_by value x > y, its guaranteed that x would throttle
+more b/w than y.  The info directory specifies the max thrtl_by value
+and thrtl_by granularity.

This interface is really crap. The natural way to express it is:

Requested Bandwidth = X %

i.e. 100% is unthrottled.

The info file should tell the minimum bandwidth,the granularity value and the scale mode.

The actual programming should just take a bandwidth percentage value between 0 and 100. The written value is adjusted by the write function to the granularity and minimum bandwidth, so a subsequent readout will tell the effective value.

That's important because that allows scripts to work independent of the actual hardware implementation with default bandwidth configurations and then allows the user/admin to readout the effective values on a particular machine. If someone wants to adjust them machine specific, that's possible as well.

Aside of that this documentation should contain some information about the limitations of that bandwidth control, i.e. the fact that this is a core specific mechanism and using a high bandwidth and a low bandwidth setting on two threads sharing a core will throttle the high bandwidth thread inadvertently.

because that's going to bring interesting surprises for users.

Thanks,

tglx

Quoted 3 lines · Thomas Gleixner

This interface is really crap. The natural way to express it is:

Requested Bandwidth = X %

I wanted to do it this way which did seem more intuitive but the issue is with the non-linear scale which the hardware does not guarantee a particular percentage for a particular value. Or we don't know the curve for delay value vs. actual b/w throttled.

ex: in non linear scale , the granularity is 2^n.
Max : 512

Say a value of 256 is not guaranteed to have 50% or even follow a curve where we can calculate the corresponding percentage.

Quoted 10 lines · Shivappa Vikas

Quoted 3 lines · Thomas Gleixner

This interface is really crap. The natural way to express it is:

Requested Bandwidth = X %

I wanted to do it this way which did seem more intuitive but the issue is with the non-linear scale which the hardware does not guarantee a particular percentage for a particular value. Or we don't know the curve for delay value vs. actual b/w throttled.

Quoted 5 lines

ex: in non linear scale , the granularity is 2^n.
Max : 512

Say a value of 256 is not guaranteed to have 50% or even follow a curve where we can calculate the corresponding percentage.

The question is whether this non linear scale thing is just a first implementation attempt and any sane hardware in the future will use the percentage value (which is an approximation as well).

If that non-linear scale is not going to be prevalent, then we really can live with the fallout of a particular CPU type.

If it's going to stay, then Intel should be able to provide simple tables which give us the required information for a particular CPU model.

Thanks,

tglx

Quoted 2 lines · Thomas Gleixner

If it's going to stay, then Intel should be able to provide simple tables which give us the required information for a particular CPU model.

By sample table - does this mean we can map a throttle value in non-linear scale to its percentage ?

Quoted 4 lines · Shivappa Vikas

Quoted 2 lines · Thomas Gleixner

If it's going to stay, then Intel should be able to provide simple tables which give us the required information for a particular CPU model.

Quoted 2 lines

By sample table - does this mean we can map a throttle value in non-linear scale to its percentage ?

Exactly. And that map should be provided by Intel simply because it's a royal pain in the neck if users have to map random power of 2 values to something useful. And of course every user has to do the same thing on their own. That'd be more than silly.

Thanks,

tglx

This patch does some changes to get ready to handle more resources like Memory b/w allocation(MBA).

-Update the control registers only when user changes the controls(cbm for Cache resources and Mem b/w for memory). Hence not sending IPIs on all domains when user updates the control vals.
-Introduce next_enabled_resource rather than looping through all resources while parsing each schemata line. The order of resources should be anyways the same as the root schemata.
-Return error as soon as we detect a resource not entering all domain values in schemata rather than waiting till we parse all resources.

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

+/*
+ * Parameter r must be NULL or pointing to
+ * a valid rdt_resource_all entry.
+ * Points r to the next enabled RDT resource at the end.
+ */

Quoted 11 lines · Vikas Shivappa

This patch does some changes to get ready to handle more resources like Memory b/w allocation(MBA).

-Update the control registers only when user changes the controls(cbm for Cache resources and Mem b/w for memory). Hence not sending IPIs on all domains when user updates the control vals.
-Introduce next_enabled_resource rather than looping through all resources while parsing each schemata line. The order of resources should be anyways the same as the root schemata.
-Return error as soon as we detect a resource not entering all domain values in schemata rather than waiting till we parse all resources.

That looks all like reasonable optimizations and I have a hard time to understand why this is a prerequisite for the bandwidth support.

And each of these changes is independent so they should be in seperate patches.

Quoted 6 lines

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

+/*
+ * Parameter r must be NULL or pointing to
+ * a valid rdt_resource_all entry.
+ * Points r to the next enabled RDT resource at the end.
+ */

arch/x86/include/asm/intel_rdt.h enum { · line 173

Quoted 10 lines

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

+#define next_enabled_rdt_resource(r)					\
+do {									\
+	if (!r)								\
+		r = rdt_resources_all;					\
+	else								\
+		r++;							\
+	for (; r < rdt_resources_all + RDT_NUM_RESOURCES; r++)		\
+		if (r->enabled)						\
+			break;						\
+} while (0)

This is crap, really. What the heck is wrong with a proper function?

static struct rdt_resource *get_next_enabled_resource(struct rdt_resource *r)

{
	....

	return r;
}

Thanks,

tglx

Quoted 16 lines · Thomas Gleixner

Quoted 11 lines · Vikas Shivappa

This patch does some changes to get ready to handle more resources like Memory b/w allocation(MBA).

-Update the control registers only when user changes the controls(cbm for Cache resources and Mem b/w for memory). Hence not sending IPIs on all domains when user updates the control vals.
-Introduce next_enabled_resource rather than looping through all resources while parsing each schemata line. The order of resources should be anyways the same as the root schemata.
-Return error as soon as we detect a resource not entering all domain values in schemata rather than waiting till we parse all resources.

That looks all like reasonable optimizations and I have a hard time to understand why this is a prerequisite for the bandwidth support.

Quoted 2 lines

And each of these changes is independent so they should be in seperate patches.

Ok will split them. Its not a pre requisite for MBA. Should i send as a seperate series ?

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.

Quoted 3 lines · 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.

So why is the $subject of this patch claiming that it provides the feature detection?

arch/x86/include/asm/intel_rdt.h enum { · line 187

Quoted 6 lines

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

-/* CPUID.(EAX=10H, ECX=ResID=1).EDX */
+/* CPUID.(EAX=10H, ECX=ResID=3).EAX */
+union cpuid_0x10_3_eax {
+	struct {
+		unsigned int max_delay:12;
+	} split;

And the point of this struct is?

Quoted 3 lines

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

+	unsigned int full;
+};

Quoted 9 lines · Thomas Gleixner

Quoted 3 lines · 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.

So why is the $subject of this patch claiming that it provides the feature detection?

Quoted 8 lines

arch/x86/include/asm/intel_rdt.h enum { · line 187

Quoted 6 lines

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

-/* CPUID.(EAX=10H, ECX=ResID=1).EDX */
+/* CPUID.(EAX=10H, ECX=ResID=3).EAX */
+union cpuid_0x10_3_eax {
+	struct {
+		unsigned int max_delay:12;
+	} split;

And the point of this struct is?

I suppose its there so we cannot forget adding it when we add more bitfields in that word and to keep naming (full vs split) consistent wrt other cpuid unions that do have multiple fields.

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

+static void rdt_get_mem_config(struct rdt_resource *r)
+{
+	union cpuid_0x10_3_eax eax;
+	union cpuid_0x10_x_edx edx;
+	u32 ebx, ecx;

Quoted 5 lines · Vikas Shivappa

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

+static void rdt_get_mem_config(struct rdt_resource *r)
+{
+	union cpuid_0x10_3_eax eax;
+	union cpuid_0x10_x_edx edx;
+	u32 ebx, ecx;

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

Quoted 10 lines

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

+
+	cpuid_count(0x00000010, 3, &eax.full, &ebx, &ecx, &edx.full);
+	r->num_closid = edx.split.cos_max + 1;
+	r->max_delay = eax.split.max_delay + 1;
+	r->no_ctrl = 0;
+	if (ecx & MBE_IS_LINEAR)
+		r->delay_linear = true;
+
+	if (r->delay_linear)
+		r->delay_gran = MAX_MBA_THRTL - r->max_delay;

What's the point of this extra conditional?

	if (ecx & MBE_IS_LINEAR) {
		r->delay_linear = true;
		r->delay_gran = MAX_MBA_THRTL - r->max_delay;
	}

would be too obvious and easy to understand, right?

arch/x86/kernel/cpu/intel_rdt.c static struct rdt_domain *rdt_find_domain(struct rdt_resource *r, int id, · line 301

Quoted 9 lines

arch/x86/kernel/cpu/intel_rdt.c +8 -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) {
+		kfree(d);

Freeing memory in the error path of some other random function is just wrong.

	if (!d->ctrl_val)
		return -ENOMEM;

and deal with the fallout at the calling function.

Thanks,

tglx

Add the 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.
- max_thrtl_by: the max throttle by values.

Throttle by can have a linear scale and non linear scale. In linear scale, if a throttle_by value is 40%, it means that the memory b/w is throttled 'by' 40% or in other words a max of 60% b/w is allowed to pass. In non-linear scale, the throttle_by values are in 2^n granularity. The h/w does not guarantee a curve of actual throttle w.r.t the configured values. But if a throttle_by value of x > y, then x is guaranteed to throttle more b/w than y.

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

  * @max_delay:		Max throttle delay
  * @delay_gran:		Throttle delay granularity
  * @delay_linear:		true if delay is in linear scale

@@ -98,6 +99,8 @@ struct rdt_resource {
 	int			cbm_len;
 	int			min_cbm_bits;
 	u32			no_ctrl;
+	struct rftype		*info_files;
+	int			infofiles_len;
 	u32			max_delay;
 	u32			delay_gran;
 	u32			delay_linear;

@@ -137,6 +140,9 @@ struct msr_param {
 	int			high;
 };
 
+void rdt_get_cache_infofile(struct rdt_resource *r);
+void rdt_get_mbe_infofile(struct rdt_resource *r);
+
 extern struct mutex rdtgroup_mutex;
 
 extern struct rdt_resource rdt_resources_all[];

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

index 6736e1d..bdfbd1d 100644
@@ -154,6 +154,7 @@ static void rdt_get_mem_config(struct rdt_resource *r)
 	if (r->delay_linear)
 		r->delay_gran = MAX_MBA_THRTL - r->max_delay;
 
+	rdt_get_mbe_infofile(r);
 	r->capable = true;
 	r->enabled = true;
 }

@@ -168,6 +169,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->no_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 +33 -1

index 53f1917..9d9b7f4 100644
@@ -517,9 +517,41 @@ static int rdt_min_cbm_bits_show(struct kernfs_open_file *of,
 
 	return 0;
 }
+static int rdt_max_delay_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->max_delay);
+
+	return 0;
+}
+
+static int rdt_delay_gran_show(struct kernfs_open_file *of,
+			     struct seq_file *seq, void *v)
+{
+	struct rdt_resource *r = of->kn->parent->priv;
+
+	if (r->delay_linear)
+		seq_printf(seq, "%d\n", r->delay_gran);
+	else
+		seq_printf(seq, "power of 2\n");
+
+	return 0;
+}
+
+static int rdt_delay_linear_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->delay_linear);
+
+	return 0;
+}
 
 /* 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 +572,52 @@ static int rdt_min_cbm_bits_show(struct kernfs_open_file *of,
 	},
 };
 

Quoted 6 lines · Vikas Shivappa

Add the 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.
- max_thrtl_by: the max throttle by values.

Quoted 7 lines

Throttle by can have a linear scale and non linear scale. In linear scale, if a throttle_by value is 40%, it means that the memory b/w is throttled 'by' 40% or in other words a max of 60% b/w is allowed to pass. In non-linear scale, the throttle_by values are in 2^n granularity. The h/w does not guarantee a curve of actual throttle w.r.t the configured values. But if a throttle_by value of x > y, then x is guaranteed to throttle more b/w than y.

This is ambigous because that is only correct when the effective values are different. x=11 and y=12 with a granularity of 10 are resulting in the same throttling.

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

Quoted 9 lines

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

@@ -5,7 +5,6 @@
 
 #include <linux/kernfs.h>
 #include <linux/jump_label.h>
-
 #include <asm/intel_rdt_common.h>

This white space is there on purpose to seperate linux and asm prefixed includes. Stop making random white space changes.

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

Quoted 6 lines

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

@@ -77,6 +76,8 @@ struct rftype {
  * @no_ctrl:			Specifies max cache cbm or min mem b/w delay.
  * @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

This wants to be a seperate patch as it changes the way how the info files are set up. The implementation for the MBA stuff is a follow up patch.

Quoted 94 lines

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

  * @max_delay:		Max throttle delay
  * @delay_gran:		Throttle delay granularity
  * @delay_linear:		true if delay is in linear scale

@@ -98,6 +99,8 @@ struct rdt_resource {
 	int			cbm_len;
 	int			min_cbm_bits;
 	u32			no_ctrl;
+	struct rftype		*info_files;
+	int			infofiles_len;
 	u32			max_delay;
 	u32			delay_gran;
 	u32			delay_linear;

@@ -137,6 +140,9 @@ struct msr_param {
 	int			high;
 };
 
+void rdt_get_cache_infofile(struct rdt_resource *r);
+void rdt_get_mbe_infofile(struct rdt_resource *r);
+
 extern struct mutex rdtgroup_mutex;
 
 extern struct rdt_resource rdt_resources_all[];

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

index 6736e1d..bdfbd1d 100644
@@ -154,6 +154,7 @@ static void rdt_get_mem_config(struct rdt_resource *r)
 	if (r->delay_linear)
 		r->delay_gran = MAX_MBA_THRTL - r->max_delay;
 
+	rdt_get_mbe_infofile(r);
 	r->capable = true;
 	r->enabled = true;
 }

@@ -168,6 +169,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->no_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 +33 -1

index 53f1917..9d9b7f4 100644
@@ -517,9 +517,41 @@ static int rdt_min_cbm_bits_show(struct kernfs_open_file *of,
 
 	return 0;
 }
+static int rdt_max_delay_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->max_delay);
+
+	return 0;
+}
+
+static int rdt_delay_gran_show(struct kernfs_open_file *of,
+			     struct seq_file *seq, void *v)
+{
+	struct rdt_resource *r = of->kn->parent->priv;
+
+	if (r->delay_linear)
+		seq_printf(seq, "%d\n", r->delay_gran);
+	else
+		seq_printf(seq, "power of 2\n");
+
+	return 0;
+}
+
+static int rdt_delay_linear_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->delay_linear);
+
+	return 0;
+}
 
 /* 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 +572,52 @@ static int rdt_min_cbm_bits_show(struct kernfs_open_file *of,
 	},
 };
 

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

Quoted 10 lines

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

+/* rdtgroup information files for MBE. */
+static struct rftype res_mbe_info_files[] = {
+	{
+		.name		= "num_closids",
+		.mode		= 0444,
+		.kf_ops		= &rdtgroup_kf_single_ops,
+		.seq_show	= rdt_num_closids_show,
+	},
+	{
+		.name		= "max_thrtl_by",

You surely could not come up with a more cryptic file name, right? What's so wrong with spelling out throttle? And the whole '_by' postfix here and on the other files is pointless as well.

Thanks,

tglx

Quoted 16 lines · Thomas Gleixner

Quoted 6 lines · Vikas Shivappa

Add the 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.
- max_thrtl_by: the max throttle by values.

Quoted 7 lines

Throttle by can have a linear scale and non linear scale. In linear scale, if a throttle_by value is 40%, it means that the memory b/w is throttled 'by' 40% or in other words a max of 60% b/w is allowed to pass. In non-linear scale, the throttle_by values are in 2^n granularity. The h/w does not guarantee a curve of actual throttle w.r.t the configured values. But if a throttle_by value of x > y, then x is guaranteed to throttle more b/w than y.

Quoted 3 lines

This is ambigous because that is only correct when the effective values are different. x=11 and y=12 with a granularity of 10 are resulting in the same throttling.

The x and y are actual values written which i will spell out. The assumption is that only correct values are input though because we filterout the values which dont follow granularity, meaning return -EINVAL when someone tries to write 11 when granularity is 10. This was with the idea thats its easier for the user to understand whats actually written. Woudl that be reasonable or does it need a change ?
(Although the h/w does like you say , we can do a msr write for 11 etc ..)

Quoted 6 lines

Quoted 6 lines

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

+/* rdtgroup information files for MBE. */
+static struct rftype res_mbe_info_files[] = {
+	{
+		.name		= "num_closids",
+		.mode		= 0444,
+		.kf_ops		= &rdtgroup_kf_single_ops,

Quoted 8 lines

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

Quoted 4 lines

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

+		.seq_show	= rdt_num_closids_show,
+	},
+	{
+		.name		= "max_thrtl_by",

You surely could not come up with a more cryptic file name, right? What's so wrong with spelling out throttle? And the whole '_by' postfix here and on the other files is pointless as well.

This is due to the issue i mention in reply to 1/8.. Can be changed to

Quoted 46 lines · Shivappa Vikas

Quoted 16 lines · Thomas Gleixner

Quoted 6 lines · Vikas Shivappa

Add the 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.
- max_thrtl_by: the max throttle by values.

Quoted 7 lines

Throttle by can have a linear scale and non linear scale. In linear scale, if a throttle_by value is 40%, it means that the memory b/w is throttled 'by' 40% or in other words a max of 60% b/w is allowed to pass. In non-linear scale, the throttle_by values are in 2^n granularity. The h/w does not guarantee a curve of actual throttle w.r.t the configured values. But if a throttle_by value of x > y, then x is guaranteed to throttle more b/w than y.

Quoted 3 lines

This is ambigous because that is only correct when the effective values are different. x=11 and y=12 with a granularity of 10 are resulting in the same throttling.

The x and y are actual values written which i will spell out. The assumption is that only correct values are input though because we filterout the values which dont follow granularity, meaning return -EINVAL when someone tries to write 11 when granularity is 10. This was with the idea thats its easier for the user to understand whats actually written. Woudl that be reasonable or does it need a change ?
(Although the h/w does like you say , we can do a msr write for 11 etc ..)

Quoted 14 lines

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

Quoted 10 lines

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

+/* rdtgroup information files for MBE. */
+static struct rftype res_mbe_info_files[] = {
+	{
+		.name		= "num_closids",
+		.mode		= 0444,
+		.kf_ops		= &rdtgroup_kf_single_ops,
+		.seq_show	= rdt_num_closids_show,
+	},
+	{
+		.name		= "max_thrtl_by",

You surely could not come up with a more cryptic file name, right? What's so wrong with spelling out throttle? And the whole '_by' postfix here and on the other files is pointless as well.

Quoted 1 line

This is due to the issue i mention in reply to 1/8.. Can be changed to

Err no. max_thrtl_by has nothing to do with 1/8. Of course you want to expose the hardware information.

What's wrong with spelling out throttle and remove the '_by':

max_throttle

That is readable, understandable and matches the documentation in the SDM.

It's not that hard.

Thanks,

tglx

/*
+ * Check whether MBE 'throttle by' value is correct.
+ * As per the SDM, when the scale is linear the
+ * throttle_by granularity is '100 - max_thrtl_by'
+ * and when its non-linear it is 'power of 2'.

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

Quoted 12 lines · Vikas Shivappa

+ * @display_str: Format string to show schemata
+ * @validate: API to validate the ctrl values.
* @info_files: resctrl info files for the resource
* @infofiles_len: Number of info files
* @max_delay: Max throttle delay

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

@@ -99,6 +101,9 @@ struct rdt_resource {
 	int			cbm_len;
 	int			min_cbm_bits;
 	u32			no_ctrl;
+	char			*display_str;
+	int (*validate)		(char *buf, unsigned long *data,
+				  struct rdt_resource *r);

Again this display and validation change wants to be seperate from the bandwidth stuff.

It's not rocket science to split patches into preparatory and implementation parts.

arch/x86/kernel/cpu/intel_rdt.c static void rdt_get_mem_config(struct rdt_resource *r) · line 161

Quoted 4 lines

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

+	r->display_str = kstrdup("%d=%d", GFP_KERNEL);
+	if (!r->display_str)
+		return -ENOMEM;

And the point of this allocation is? To consume extra memory for a constant string which is in const data anyway.

      r->display_str = "%d=%d";

does not need allcotion and consumes exactly the same amount of const data as the above. Oh well...

arch/x86/kernel/cpu/intel_rdt.c static void rdt_get_cdp_l3_config(int type) · line 209

Quoted 3 lines

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

-static inline bool get_rdt_resources(void)
+static inline int get_rdt_resources(void)
 {

And the point of this change is? Lots of churn to return the same -ENODEV value at the call site. So why are you trying to return other values instead of the simple boolean success/fail decision?

Quoted 5 lines

/*
+ * Check whether MBE 'throttle by' value is correct.
+ * As per the SDM, when the scale is linear the
+ * throttle_by granularity is '100 - max_thrtl_by'
+ * and when its non-linear it is 'power of 2'.

That's wrong. We really want to let the user set a bandwidth percentage value from 0 - 100 %. And then adjust it to the proper value which the hardware can provide. So the user value is independent from granularity, linear and the max throttling allowed.

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

Quoted 13 lines

/*
- * 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.
*/

arch/x86/kernel/cpu/intel_rdt_schemata.c +2 -2

-static int parse_cbm(char *buf, struct rdt_resource *r)
+static int parse_ctrls(char *buf, struct rdt_resource *r)
 {
 	unsigned long data;
-	int ret;
+	int ret = 0;

What's the purpose of initializing ret to 0 if the next action is assigning ret the return value of the validate function?

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

Quoted 12 lines

arch/x86/kernel/cpu/intel_rdt_schemata.c +2 -6

-	ret = kstrtoul(buf, 16, &data);
-	if (ret)
-		return ret;
-	if (!cbm_validate(data, r))
-		return -EINVAL;
+	ret = r->validate(buf, &data, r);
 	r->tmp_ctrl[r->num_tmp_ctrl++] = data;
 
-	return 0;
+	return ret;
 }

Thanks,

tglx

During rmdir only reset the ctrl values for the rdtgroup's closid. This is done so that that next time when the closid is reused they dont reflect old values.

Remove the closid update during cpuonline in cqm as its already in the CAT code. Since both cqm and CAT want the rmid and closid to be zero when cpu is online, remove the PQR MSR write to zero during cpuonline because the MSRs are at zero after cpu reset and also during

Quoted 3 lines · Vikas Shivappa

During rmdir only reset the ctrl values for the rdtgroup's closid. This is done so that that next time when the closid is reused they dont reflect old values.

How on earth is that related to MBA?

Quoted 5 lines

Remove the closid update during cpuonline in cqm as its already in the CAT code. Since both cqm and CAT want the rmid and closid to be zero when cpu is online, remove the PQR MSR write to zero during cpuonline because the MSRs are at zero after cpu reset and also during

Are you sure that the MSRs are zero when the cpu goes offline and then online again? That's not a reset.

Thanks,

tglx