Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1555855 > unrolled thread

[PATCH 3/8] x86/intel_rdt/mba: Improvements to handle more RDT resources like MBA

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/8] x86/intel_rdt/mba: Improvements to handle more RDT resources like MBA Vikas Shivappa <vikas.shivappa@linux.intel.com> - 2017-01-10 20:40 +0100
    Re: [PATCH 3/8] x86/intel_rdt/mba: Improvements to handle more RDT  resources like MBA Thomas Gleixner <tglx@linutronix.de> - 2017-01-16 15:00 +0100
      Re: [PATCH 3/8] x86/intel_rdt/mba: Improvements to handle more RDT  resources like MBA Shivappa Vikas <vikas.shivappa@intel.com> - 2017-01-18 02:00 +0100
        Re: [PATCH 3/8] x86/intel_rdt/mba: Improvements to handle more RDT  resources like MBA Thomas Gleixner <tglx@linutronix.de> - 2017-01-18 10:10 +0100

#1555855 — [PATCH 3/8] x86/intel_rdt/mba: Improvements to handle more RDT resources like MBA

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.

Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
---
 arch/x86/include/asm/intel_rdt.h         | 16 +++++++++++++
 arch/x86/kernel/cpu/intel_rdt_schemata.c | 40 +++++++++++++++++---------------
 2 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/arch/x86/include/asm/intel_rdt.h b/arch/x86/include/asm/intel_rdt.h
index 9f66bf5..e1f8acb 100644
--- a/arch/x86/include/asm/intel_rdt.h
+++ b/arch/x86/include/asm/intel_rdt.h
@@ -156,6 +156,22 @@ enum {
 	     r++)							      \
 		if (r->enabled)
 
+/*
+ * 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.
+ */
+#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)
+
 /* CPUID.(EAX=10H, ECX=ResID=1).EAX */
 union cpuid_0x10_1_eax {
 	struct {
diff --git a/arch/x86/kernel/cpu/intel_rdt_schemata.c b/arch/x86/kernel/cpu/intel_rdt_schemata.c
index c50f742..054d771 100644
--- a/arch/x86/kernel/cpu/intel_rdt_schemata.c
+++ b/arch/x86/kernel/cpu/intel_rdt_schemata.c
@@ -94,6 +94,10 @@ static int parse_line(char *line, struct rdt_resource *r)
 			return -EINVAL;
 	}
 
+	/* Incorrect number of domains in the line */
+	if (r->num_tmp_ctrl != r->num_domains)
+		return -EINVAL;
+
 	/* Any garbage at the end of the line? */
 	if (line && line[0])
 		return -EINVAL;
@@ -114,9 +118,16 @@ static int update_domains(struct rdt_resource *r, int closid)
 	msr_param.high = msr_param.low + 1;
 	msr_param.res = r;
 
+	/*
+	 * Only update the domains that user has changed.
+	 * There by avoiding unnecessary IPIs.
+	 */
 	list_for_each_entry(d, &r->domains, list) {
-		cpumask_set_cpu(cpumask_any(&d->cpu_mask), cpu_mask);
-		d->ctrl_val[msr_param.low] = r->tmp_ctrl[idx++];
+		if (d->ctrl_val[msr_param.low] != r->tmp_ctrl[idx]) {
+			cpumask_set_cpu(cpumask_any(&d->cpu_mask), cpu_mask);
+			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. */
@@ -164,30 +175,21 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
 		r->num_tmp_ctrl = 0;
 	}
 
+	r = NULL;
 	while ((tok = strsep(&buf, "\n")) != NULL) {
 		resname = strsep(&tok, ":");
 		if (!tok) {
 			ret = -EINVAL;
 			goto out;
 		}
-		for_each_enabled_rdt_resource(r) {
-			if (!strcmp(resname, r->name) &&
-			    closid < r->num_closid) {
-				ret = parse_line(tok, r);
-				if (ret)
-					goto out;
-				break;
-			}
-		}
-		if (!r->name) {
-			ret = -EINVAL;
-			goto out;
-		}
-	}
 
-	/* Did the parser find all the masks we need? */
-	for_each_enabled_rdt_resource(r) {
-		if (r->num_tmp_ctrl != r->num_domains) {
+		next_enabled_rdt_resource(r);
+
+		if (!strcmp(resname, r->name)) {
+			ret = parse_line(tok, r);
+			if (ret)
+				goto out;
+		} else {
 			ret = -EINVAL;
 			goto out;
 		}
-- 
1.9.1

[toc] | [next] | [standalone]

#1559755 — Re: [PATCH 3/8] x86/intel_rdt/mba: Improvements to handle more RDT resources like MBA

On Tue, 10 Jan 2017, Vikas Shivappa wrote:
> 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.

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

[toc] | [prev] | [next] | [standalone]

#1561190 — Re: [PATCH 3/8] x86/intel_rdt/mba: Improvements to handle more RDT resources like MBA

On Mon, 16 Jan 2017, Thomas Gleixner wrote:

> On Tue, 10 Jan 2017, Vikas Shivappa wrote:
>> 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.

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

>
>> +/*
>> + * 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.
>> + */
>> +#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;
> }
>

Will fix,

Thanks,
Vikas

> Thanks,
>
> 	tglx
>

[toc] | [prev] | [next] | [standalone]

#1561365 — Re: [PATCH 3/8] x86/intel_rdt/mba: Improvements to handle more RDT resources like MBA

On Tue, 17 Jan 2017, Shivappa Vikas wrote:
> On Mon, 16 Jan 2017, Thomas Gleixner wrote:
> 
> > On Tue, 10 Jan 2017, Vikas Shivappa wrote:
> > > 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.
> 
> Ok will split them. Its not a pre requisite for MBA. Should i send as a
> seperate series ?

Yes please. That way we can apply preparatory things first and then
concentrate on the new MBA functionality.

Thanks,

	tglx

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web