[PATCH 0/5] x86/intel_rdt: Improvements/Fixes to RDT framework

[PATCH 0/5] x86/intel_rdt: Improvements/Fixes to RDT framework

[Vikas Shivappa]

Add some improvements to the existing RDT(Resource director technology)
framework and fix some hotcpu RDT code.  Sending them as a seperate
series as per Thomas suggestion during the V1 of MBA review (Memory
bandwidth allocation) -
https://marc.info/?l=linux-kernel&m=148407699309286

Patches are based on 4.10-rc8

[PATCH 1/5] x86/intel_rdt: Update control registers only when user
[PATCH 2/5] x86/intel_rdt: Improvements to parsing schemata
[PATCH 3/5] x86/intel_rdt: Fail early on a resource with incorrect
[PATCH 4/5] x86/intel_rdt: Reset the cbm MSR during rmdir
[PATCH 5/5] x86/intel_rdt: hotcpu updates for RDT

[PATCH 1/5] x86/intel_rdt: Update control registers only when user really modifies it

[Vikas Shivappa]

When a schemata is updated, the values for all the domains and all
resources are entered.  However, the values for each of them may not
change in many use cases as the user is only updating values for a
subset of resources and domains. The resource control values are updated
via QOS_MSRs which are per package. Change the update to QOS_MSRs to
happen only when the control value on the particular domain is updated.
Hence not sending IPIs on all domains when user updates the control
vals.

Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
---
 arch/x86/kernel/cpu/intel_rdt_schemata.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel_rdt_schemata.c b/arch/x86/kernel/cpu/intel_rdt_schemata.c
index f369cb8..14ba504 100644
--- a/arch/x86/kernel/cpu/intel_rdt_schemata.c
+++ b/arch/x86/kernel/cpu/intel_rdt_schemata.c
@@ -114,9 +114,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->cbm[msr_param.low] = r->tmp_cbms[idx++];
+		if (d->cbm[msr_param.low] != r->tmp_cbms[idx]) {
+			cpumask_set_cpu(cpumask_any(&d->cpu_mask), cpu_mask);
+			d->cbm[msr_param.low] = r->tmp_cbms[idx];
+		}
+		idx++;
 	}
 	cpu = get_cpu();
 	/* Update CBM on this cpu if it's in cpu_mask. */
-- 
1.9.1

Re: [PATCH 1/5] x86/intel_rdt: Update control registers only when user really modifies it

[Thomas Gleixner]

On Fri, 17 Feb 2017, Vikas Shivappa wrote:

x86/intel_rdt: Update control registers only when user really modifies it

This hardly is a precise short summary.

> When a schemata is updated, the values for all the domains and all
> resources are entered.  However, the values for each of them may not
> change in many use cases as the user is only updating values for a
> subset of resources and domains. The resource control values are updated
> via QOS_MSRs which are per package. Change the update to QOS_MSRs to
> happen only when the control value on the particular domain is updated.
> Hence not sending IPIs on all domains when user updates the control
> vals.

Can you please structure your changelogs in a way which makes them
readable? The above is one big confusing lump. I asked you before to
provide changelogs which are properly structured:

 1) Context
 2) Problem
 3) Solution

and the sections to be precise and clear and not clobbered with completely
useless implementation details.

So a proper changelog for this would be:

 x86/intel_rdt: Avoid update of unchanged control registers

   Schemata files can only be updated as a whole, even if only a single
   value for a specific domain/resource changes.

   The current implementation updates all control registers unconditionally
   even if the values have not been changed by the schemata update. This
   results in pointless IPIs and MSR writes.

   Add a check whether the control register value actually changed and only
   update the affected CPUs.

Can you spot the difference?

> index f369cb8..14ba504 100644
> --- a/arch/x86/kernel/cpu/intel_rdt_schemata.c
> +++ b/arch/x86/kernel/cpu/intel_rdt_schemata.c
> @@ -114,9 +114,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.

s/There by/Thereby/

But the above is wrong anyway because you split it into two sentences and
obfuscate the reasoning.

	 * To avoid unnecessary IPIs update only domains, which have been
         * changed by the schemata write.

That makes it clear that we do it in order to avoid the IPIs. The above
could be misinterpreted as having the side effect of avoiding the IPIs.

Thanks,

	tglx

Re: [PATCH 1/5] x86/intel_rdt: Update control registers only when user really modifies it

[Shivappa Vikas]

On Wed, 1 Mar 2017, Thomas Gleixner wrote:

> On Fri, 17 Feb 2017, Vikas Shivappa wrote:
>
> x86/intel_rdt: Update control registers only when user really modifies it
>
> This hardly is a precise short summary.
>
>> When a schemata is updated, the values for all the domains and all
>> resources are entered.  However, the values for each of them may not
>> change in many use cases as the user is only updating values for a
>> subset of resources and domains. The resource control values are updated
>> via QOS_MSRs which are per package. Change the update to QOS_MSRs to
>> happen only when the control value on the particular domain is updated.
>> Hence not sending IPIs on all domains when user updates the control
>> vals.
>
> Can you please structure your changelogs in a way which makes them
> readable? The above is one big confusing lump. I asked you before to
> provide changelogs which are properly structured:
>
> 1) Context
> 2) Problem
> 3) Solution

Will fix all the change logs -

>
> and the sections to be precise and clear and not clobbered with completely
> useless implementation details.
>
> So a proper changelog for this would be:
>
> x86/intel_rdt: Avoid update of unchanged control registers
>
>   Schemata files can only be updated as a whole, even if only a single
>   value for a specific domain/resource changes.
>
>   The current implementation updates all control registers unconditionally
>   even if the values have not been changed by the schemata update. This
>   results in pointless IPIs and MSR writes.
>
>   Add a check whether the control register value actually changed and only
>   update the affected CPUs.
>
> Can you spot the difference?
>
>> index f369cb8..14ba504 100644
>> --- a/arch/x86/kernel/cpu/intel_rdt_schemata.c
>> +++ b/arch/x86/kernel/cpu/intel_rdt_schemata.c
>> @@ -114,9 +114,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.
>
> s/There by/Thereby/
>
> But the above is wrong anyway because you split it into two sentences and
> obfuscate the reasoning.
>
> 	 * To avoid unnecessary IPIs update only domains, which have been
>         * changed by the schemata write.
>
> That makes it clear that we do it in order to avoid the IPIs. The above
> could be misinterpreted as having the side effect of avoiding the IPIs.
>

Will fix the comment

Thanks,
Vikas

> Thanks,
>
> 	tglx
>

[PATCH 2/5] x86/intel_rdt: Improvements to parsing schemata

[Vikas Shivappa]

The schemata file requires all RDT (Resource director technology)
resources be entered in the same order they are shown in the root
schemata file.
Hence remove the looping through all resources while parsing each
schemata and get the next enabled resource after processing a resource.

Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
---
 arch/x86/kernel/cpu/intel_rdt_schemata.c | 40 ++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel_rdt_schemata.c b/arch/x86/kernel/cpu/intel_rdt_schemata.c
index 14ba504..64b43b1 100644
--- a/arch/x86/kernel/cpu/intel_rdt_schemata.c
+++ b/arch/x86/kernel/cpu/intel_rdt_schemata.c
@@ -138,6 +138,27 @@ static int update_domains(struct rdt_resource *r, int closid)
 	return 0;
 }
 
+/*
+ * Parameter r must be NULL or pointing to
+ * a valid rdt_resource_all entry.
+ * returns next enabled RDT resource.
+ */
+static inline struct rdt_resource*
+get_next_enabled_rdt_resource(struct rdt_resource *r)
+{
+	struct rdt_resource *it = r;
+
+	if (!it)
+		it = rdt_resources_all;
+	else
+		it++;
+	for (; it < rdt_resources_all + RDT_NUM_RESOURCES; it++)
+		if (it->enabled)
+			return it;
+
+	return NULL;
+}
+
 ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
 				char *buf, size_t nbytes, loff_t off)
 {
@@ -171,22 +192,21 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
 		r->num_tmp_cbms = 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) {
+
+		r = get_next_enabled_rdt_resource(r);
+
+		if (r && !strcmp(resname, r->name)) {
+			ret = parse_line(tok, r);
+			if (ret)
+				goto out;
+		} else {
 			ret = -EINVAL;
 			goto out;
 		}
-- 
1.9.1

Re: [PATCH 2/5] x86/intel_rdt: Improvements to parsing schemata

[Thomas Gleixner]

On Fri, 17 Feb 2017, Vikas Shivappa wrote:

> The schemata file requires all RDT (Resource director technology)
> resources be entered in the same order they are shown in the root
> schemata file.
> Hence remove the looping through all resources while parsing each
> schemata and get the next enabled resource after processing a resource.

Again, you desribe WHAT you are doing and not WHY.

 x86/intel_rdt: Improveme schemata parsing

  The schemata file requires all resources be written in the same order
  as they are shown in the root schemata file.

  The current parser searches all resources to find a matching resource for
  each resource line in the schemata file. This is suboptimal as the order
  of the resources is fixed.

  Avoid the repeating lookups by walking the resource descriptors linearly
  while processing the input lines.

So that would describe again the context, the problem and the solution in a
precise and understandable way. It's not that hard.

Though, I have to ask the question WHY is that required. It's neither
required by the current implementation nor by anything else. The current
implementation can nicely deal with any ordering of the resource lines due
to the lookup. So now the question is, what makes this change necessary and
what's the advantage of doing it this way?

> +/*
> + * Parameter r must be NULL or pointing to
> + * a valid rdt_resource_all entry.
> + * returns next enabled RDT resource.

New sentences start with an upper case letter. Aside of that, please do not
make arbitrary line breaks at randomly chosen locations. Use the 80 chars
estate unless there is a structural reason not to do so.

> +static inline struct rdt_resource*
> +get_next_enabled_rdt_resource(struct rdt_resource *r)
> +{
> +	struct rdt_resource *it = r;
> +
> +	if (!it)
> +		it = rdt_resources_all;
> +	else
> +		it++;
> +	for (; it < rdt_resources_all + RDT_NUM_RESOURCES; it++)
> +		if (it->enabled)
> +			return it;

Once more. This lacks curly braces around the for() construct. See

  http://lkml.kernel.org/r/alpine.DEB.2.20.1701171956290.3645@nanos

But that's the least of the problems with this code.

> +
> +	return NULL;
> +}
> +
>  ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
>  				char *buf, size_t nbytes, loff_t off)
>  {
> @@ -171,22 +192,21 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
>  		r->num_tmp_cbms = 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) {
> +
> +		r = get_next_enabled_rdt_resource(r);
> +
> +		if (r && !strcmp(resname, r->name)) {
> +			ret = parse_line(tok, r);
> +			if (ret)
> +				goto out;
> +		} else {
>  			ret = -EINVAL;
>  			goto out;
>  		}

I really have a hard time to figure out, why this convoluted
get_next_enabled_rdt_resource() is better than what we have now.

The write function is hardly a hot path and enforcing the resource write
ordering is questionable at best.

If there is a real reason to do so, then this can be written way less
convoluted.

static struct rdt_resource *get_enabled_resource(struct rdt_resource *r)
{
	for (; r < rdt_resources_all + RDT_NUM_RESOURCES; r++) {
		if (r->enabled)
			return r;
}

ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
 				char *buf, size_t nbytes, loff_t off)
{
.....

	r = get_enabled_resource(rdt_resources_all);

 	while (r && (tok = strsep(&buf, "\n")) != NULL) {
		ret = -EINVAL;
		
 		resname = strsep(&tok, ":");
 		if (!tok || strcmp(resname, r->name))
 			goto out;

		ret = parse_line(tok, r);
		if (ret)
			goto out;

		r = get_enabled_resource(++r);
	}

Can you spot the difference?

Anyway, first of all we want to know WHY this ordering is required. If it's
not required then why would we enforce it?

Thanks,

	tglx

Re: [PATCH 2/5] x86/intel_rdt: Improvements to parsing schemata

[Shivappa Vikas]

> Anyway, first of all we want to know WHY this ordering is required. If it's
> not required then why would we enforce it?

Do the users want to see the data in the same order they entered ? If we want to 
do that then its easy to implement when we enforce the order .. (because right 
now resources are always displayed in a predefined order).
Otherwise i can drop this patch.

Thanks,
Vikas

>
> Thanks,
>
> 	tglx
>

Re: [PATCH 2/5] x86/intel_rdt: Improvements to parsing schemata

[Thomas Gleixner]

On Thu, 9 Mar 2017, Shivappa Vikas wrote:

> 
> > Anyway, first of all we want to know WHY this ordering is required. If it's
> > not required then why would we enforce it?
> 
> Do the users want to see the data in the same order they entered ? If we want
> to do that then its easy to implement when we enforce the order .. (because
> right now resources are always displayed in a predefined order).
> Otherwise i can drop this patch.

It's fine to display them in a defined order, but there is no point to
enforce the ordering on write.

The real question here is whether we really have to write every line on
every update. IMO it's sufficient to write a single resource line and not
require to update all resources every time.

Thanks,

	tglx

Re: [PATCH 2/5] x86/intel_rdt: Improvements to parsing schemata

[Shivappa Vikas]

On Fri, 10 Mar 2017, Thomas Gleixner wrote:

> On Thu, 9 Mar 2017, Shivappa Vikas wrote:
>
>>
>>> Anyway, first of all we want to know WHY this ordering is required. If it's
>>> not required then why would we enforce it?
>>
>> Do the users want to see the data in the same order they entered ? If we want
>> to do that then its easy to implement when we enforce the order .. (because
>> right now resources are always displayed in a predefined order).
>> Otherwise i can drop this patch.
>
> It's fine to display them in a defined order, but there is no point to
> enforce the ordering on write.
>
> The real question here is whether we really have to write every line on
> every update. IMO it's sufficient to write a single resource line and not
> require to update all resources every time.

Ok in that case we can drop this. because my thought was that user wants to see 
the contents he wrote when he overwrites the whole file like below , IOW its 
wierd for user to do

# echo "L3:0=0xff;1=0xf0" > /sys/fs/resctrl/schemata
then

# cat /sys/fs/resctrl/schemata
L3:0=0xff;1=0xf0
L2:0=0xff;1=0xf0
MB:....

But he did not write the L2,MB when he did an overwrite of the whole file.


Thanks,
Vikas

>
> Thanks,
>
> 	tglx
>
>
>

Re: [PATCH 2/5] x86/intel_rdt: Improvements to parsing schemata

[Thomas Gleixner]

On Fri, 10 Mar 2017, Shivappa Vikas wrote:
> On Fri, 10 Mar 2017, Thomas Gleixner wrote:
> > It's fine to display them in a defined order, but there is no point to
> > enforce the ordering on write.
> > 
> > The real question here is whether we really have to write every line on
> > every update. IMO it's sufficient to write a single resource line and not
> > require to update all resources every time.
> 
> Ok in that case we can drop this. because my thought was that user wants to
> see the contents he wrote when he overwrites the whole file like below , IOW
> its wierd for user to do
> 
> # echo "L3:0=0xff;1=0xf0" > /sys/fs/resctrl/schemata
> then
> 
> # cat /sys/fs/resctrl/schemata
> L3:0=0xff;1=0xf0
> L2:0=0xff;1=0xf0
> MB:....
> 
> But he did not write the L2,MB when he did an overwrite of the whole file.

Well, we have several options to tackle this:

1) Have schemata files for each resource

   schemata_l2, _l3 _mb

2) Request a full overwrite every time (all entries required)

   That still does not require ordering

3) Allow full overwrite and 'append' mode

   echo "...." > schemata

   Overwrites the whole file. It does not require all entries to be
   supplied.  Non supplied entries are reset to default

   echo "...." >> schemata
		 
   "Appends" the supplied entries by overwriting the existing ones.

My favourite would be #1, but I have no strong opinions other than not
caring about resource write ordering for #2 and #3.

Thanks,

	tglx

Re: [PATCH 2/5] x86/intel_rdt: Improvements to parsing schemata

[Luck, Tony]

On Fri, Mar 10, 2017 at 07:58:51PM +0100, Thomas Gleixner wrote:
> Well, we have several options to tackle this:
> 
> 1) Have schemata files for each resource
> 
>    schemata_l2, _l3 _mb
> 
> 2) Request a full overwrite every time (all entries required)
> 
>    That still does not require ordering
> 
> 3) Allow full overwrite and 'append' mode
> 
>    echo "...." > schemata
> 
>    Overwrites the whole file. It does not require all entries to be
>    supplied.  Non supplied entries are reset to default
> 
>    echo "...." >> schemata
> 		 
>    "Appends" the supplied entries by overwriting the existing ones.
> 
> My favourite would be #1, but I have no strong opinions other than not
> caring about resource write ordering for #2 and #3.

If you are going to head in the direction of partial update, then
why not go for:

4) Drop the code that check that the user wrote all
   the fields as well as the check for all the lines. Just update
   the bits they list, and leave the rest unchanged.

I.e. the user could say:

# echo "L3:1=0x3f" > schemata

if they just wanted to update resource L3, instance 1.

I don't think there is much benefit to the overwrite vs. append
semantics for the user.

-Tony

Re: [PATCH 2/5] x86/intel_rdt: Improvements to parsing schemata

[Thomas Gleixner]

On Fri, 10 Mar 2017, Luck, Tony wrote:
> On Fri, Mar 10, 2017 at 07:58:51PM +0100, Thomas Gleixner wrote:
> > Well, we have several options to tackle this:
> > 
> > 1) Have schemata files for each resource
> > 
> >    schemata_l2, _l3 _mb
> > 
> > 2) Request a full overwrite every time (all entries required)
> > 
> >    That still does not require ordering
> > 
> > 3) Allow full overwrite and 'append' mode
> > 
> >    echo "...." > schemata
> > 
> >    Overwrites the whole file. It does not require all entries to be
> >    supplied.  Non supplied entries are reset to default
> > 
> >    echo "...." >> schemata
> > 		 
> >    "Appends" the supplied entries by overwriting the existing ones.
> > 
> > My favourite would be #1, but I have no strong opinions other than not
> > caring about resource write ordering for #2 and #3.
> 
> If you are going to head in the direction of partial update, then
> why not go for:
> 
> 4) Drop the code that check that the user wrote all
>    the fields as well as the check for all the lines. Just update
>    the bits they list, and leave the rest unchanged.
> 
> I.e. the user could say:
> 
> # echo "L3:1=0x3f" > schemata
> 
> if they just wanted to update resource L3, instance 1.

Even better

> I don't think there is much benefit to the overwrite vs. append
> semantics for the user.

Agreed.

Thanks,

	tglx

[PATCH] x86/intel_rdt: Implement "update" mode when writing schemata file

[Luck, Tony]

From: Tony Luck <tony.luck@intel.com>

The schemata file can have multiple lines and it is cumbersome to
update from shell scripts.

Remove code that requires that the user provide values for every
resource (in the right order).  If the user provides values for
just a few resources, update them and leave the rest unchanged.

Side benefit: we now check which values were updated and only send
IPIs to cpus that actually have updates.

Cc: Vikas Shivappa <vikas.shivappa@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
Tested-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 Documentation/x86/intel_rdt_ui.txt       | 14 ++++++
 arch/x86/include/asm/intel_rdt.h         | 10 ++---
 arch/x86/kernel/cpu/intel_rdt.c          |  2 -
 arch/x86/kernel/cpu/intel_rdt_schemata.c | 73 +++++++++++++-------------------
 4 files changed, 48 insertions(+), 51 deletions(-)

diff --git a/Documentation/x86/intel_rdt_ui.txt b/Documentation/x86/intel_rdt_ui.txt
index 51cf6fa5591f..3ea198460469 100644
--- a/Documentation/x86/intel_rdt_ui.txt
+++ b/Documentation/x86/intel_rdt_ui.txt
@@ -129,6 +129,20 @@ schemata format is always:
 
 	L2:<cache_id0>=<cbm>;<cache_id1>=<cbm>;...
 
+Reading/writing the schemata file
+---------------------------------
+Reading the schemata file will show the state of all resources
+on all domains. When writing you only need to specify those values
+which you wish to change.  E.g.
+
+# cat schemata
+L3DATA:0=fffff;1=fffff;2=fffff;3=fffff
+L3CODE:0=fffff;1=fffff;2=fffff;3=fffff
+# echo "L3DATA:2=3c0;" > schemata
+# cat schemata
+L3DATA:0=fffff;1=fffff;2=3c0;3=fffff
+L3CODE:0=fffff;1=fffff;2=fffff;3=fffff
+
 Example 1
 ---------
 On a two socket machine (one L3 cache per socket) with just four bits
diff --git a/arch/x86/include/asm/intel_rdt.h b/arch/x86/include/asm/intel_rdt.h
index 0d64397cee58..d7705270c401 100644
--- a/arch/x86/include/asm/intel_rdt.h
+++ b/arch/x86/include/asm/intel_rdt.h
@@ -76,10 +76,7 @@ struct rftype {
  * @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
  * @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:
@@ -94,10 +91,7 @@ struct rdt_resource {
 	int			min_cbm_bits;
 	u32			max_cbm;
 	struct list_head	domains;
-	int			num_domains;
 	int			msr_base;
-	u32			*tmp_cbms;
-	int			num_tmp_cbms;
 	int			cache_level;
 	int			cbm_idx_multi;
 	int			cbm_idx_offset;
@@ -109,12 +103,16 @@ struct rdt_resource {
  * @id:		unique id for this instance
  * @cpu_mask:	which cpus share this resource
  * @cbm:	array of cache bit masks (indexed by CLOSID)
+ * @new_cbm:	new cbm value to be loaded
+ * @have_new_cbm: did user provide new_cbm for this domain
  */
 struct rdt_domain {
 	struct list_head	list;
 	int			id;
 	struct cpumask		cpu_mask;
 	u32			*cbm;
+	u32			new_cbm;
+	bool			have_new_cbm;
 };
 
 /**
diff --git a/arch/x86/kernel/cpu/intel_rdt.c b/arch/x86/kernel/cpu/intel_rdt.c
index 5a533fefefa0..329b8876b984 100644
--- a/arch/x86/kernel/cpu/intel_rdt.c
+++ b/arch/x86/kernel/cpu/intel_rdt.c
@@ -309,7 +309,6 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
 
 	cpumask_set_cpu(cpu, &d->cpu_mask);
 	list_add_tail(&d->list, add_pos);
-	r->num_domains++;
 }
 
 static void domain_remove_cpu(int cpu, struct rdt_resource *r)
@@ -325,7 +324,6 @@ 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);
 		list_del(&d->list);
 		kfree(d);
diff --git a/arch/x86/kernel/cpu/intel_rdt_schemata.c b/arch/x86/kernel/cpu/intel_rdt_schemata.c
index f369cb8db0d5..c8e054f4f269 100644
--- a/arch/x86/kernel/cpu/intel_rdt_schemata.c
+++ b/arch/x86/kernel/cpu/intel_rdt_schemata.c
@@ -56,7 +56,7 @@ static bool cbm_validate(unsigned long var, struct rdt_resource *r)
  * Read one cache bit mask (hex). Check that it is valid for the current
  * resource type.
  */
-static int parse_cbm(char *buf, struct rdt_resource *r)
+static int parse_cbm(char *buf, struct rdt_resource *r, struct rdt_domain *d)
 {
 	unsigned long data;
 	int ret;
@@ -66,7 +66,8 @@ 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;
+	d->new_cbm = data;
+	d->have_new_cbm = true;
 
 	return 0;
 }
@@ -74,8 +75,8 @@ static int parse_cbm(char *buf, struct rdt_resource *r)
 /*
  * For each domain in this resource we expect to find a series of:
  *	id=mask
- * separated by ";". The "id" is in decimal, and must appear in the
- * right order.
+ * separated by ";". The "id" is in decimal, and must match one of
+ * the "id"s for this resource.
  */
 static int parse_line(char *line, struct rdt_resource *r)
 {
@@ -83,21 +84,21 @@ static int parse_line(char *line, struct rdt_resource *r)
 	struct rdt_domain *d;
 	unsigned long dom_id;
 
+next:
+	if (!line || line[0] == '\0')
+		return 0;
+	dom = strsep(&line, ";");
+	id = strsep(&dom, "=");
+	if (!dom || kstrtoul(id, 10, &dom_id))
+		return -EINVAL;
 	list_for_each_entry(d, &r->domains, list) {
-		dom = strsep(&line, ";");
-		if (!dom)
-			return -EINVAL;
-		id = strsep(&dom, "=");
-		if (kstrtoul(id, 10, &dom_id) || dom_id != d->id)
-			return -EINVAL;
-		if (parse_cbm(dom, r))
-			return -EINVAL;
+		if (d->id == dom_id) {
+			if (parse_cbm(dom, r, d))
+				return -EINVAL;
+			goto next;
+		}
 	}
-
-	/* Any garbage at the end of the line? */
-	if (line && line[0])
-		return -EINVAL;
-	return 0;
+	return -EINVAL;
 }
 
 static int update_domains(struct rdt_resource *r, int closid)
@@ -105,7 +106,7 @@ static int update_domains(struct rdt_resource *r, int closid)
 	struct msr_param msr_param;
 	cpumask_var_t cpu_mask;
 	struct rdt_domain *d;
-	int cpu, idx = 0;
+	int cpu;
 
 	if (!zalloc_cpumask_var(&cpu_mask, GFP_KERNEL))
 		return -ENOMEM;
@@ -115,9 +116,13 @@ static int update_domains(struct rdt_resource *r, int closid)
 	msr_param.res = r;
 
 	list_for_each_entry(d, &r->domains, list) {
-		cpumask_set_cpu(cpumask_any(&d->cpu_mask), cpu_mask);
-		d->cbm[msr_param.low] = r->tmp_cbms[idx++];
+		if (d->have_new_cbm && d->new_cbm != d->cbm[closid]) {
+			cpumask_set_cpu(cpumask_any(&d->cpu_mask), cpu_mask);
+			d->cbm[closid] = d->new_cbm;
+		}
 	}
+	if (cpumask_empty(cpu_mask))
+		goto done;
 	cpu = get_cpu();
 	/* Update CBM on this cpu if it's in cpu_mask. */
 	if (cpumask_test_cpu(cpu, cpu_mask))
@@ -126,6 +131,7 @@ static int update_domains(struct rdt_resource *r, int closid)
 	smp_call_function_many(cpu_mask, rdt_cbm_update, &msr_param, 1);
 	put_cpu();
 
+done:
 	free_cpumask_var(cpu_mask);
 
 	return 0;
@@ -135,10 +141,10 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
 				char *buf, size_t nbytes, loff_t off)
 {
 	struct rdtgroup *rdtgrp;
+	struct rdt_domain *dom;
 	struct rdt_resource *r;
 	char *tok, *resname;
 	int closid, ret = 0;
-	u32 *l3_cbms = NULL;
 
 	/* Valid input requires a trailing newline */
 	if (nbytes == 0 || buf[nbytes - 1] != '\n')
@@ -153,16 +159,9 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
 
 	closid = rdtgrp->closid;
 
-	/* 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),
-				      GFP_KERNEL);
-		if (!r->tmp_cbms) {
-			ret = -ENOMEM;
-			goto out;
-		}
-		r->num_tmp_cbms = 0;
-	}
+	for_each_enabled_rdt_resource(r)
+		list_for_each_entry(dom, &r->domains, list)
+			dom->have_new_cbm = false;
 
 	while ((tok = strsep(&buf, "\n")) != NULL) {
 		resname = strsep(&tok, ":");
@@ -185,14 +184,6 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
 		}
 	}
 
-	/* Did the parser find all the masks we need? */
-	for_each_enabled_rdt_resource(r) {
-		if (r->num_tmp_cbms != r->num_domains) {
-			ret = -EINVAL;
-			goto out;
-		}
-	}
-
 	for_each_enabled_rdt_resource(r) {
 		ret = update_domains(r, closid);
 		if (ret)
@@ -201,10 +192,6 @@ 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;
-	}
 	return ret ?: nbytes;
 }
 
-- 
2.7.4

Re: [PATCH] x86/intel_rdt: Implement "update" mode when writing schemata file

[Fenghua Yu]

On Fri, Mar 24, 2017 at 10:51:58AM -0700, Luck, Tony wrote:
> From: Tony Luck <tony.luck@intel.com>
> 
> The schemata file can have multiple lines and it is cumbersome to
> update from shell scripts.

"from shell scripts" makes people a bit confused. Not just shell scripts,
C or Java code also can be cumbersome to update the file, right?

> +Reading/writing the schemata file
> +---------------------------------
> +Reading the schemata file will show the state of all resources
> +on all domains. When writing you only need to specify those values
> +which you wish to change.  E.g.

User can still write all values including not changed and change just
like 4.10 does. We may say "When writing you can either write all values
or only need to specify those values which you wish to change."?

Thanks.

-Fenghua

Re: [PATCH] x86/intel_rdt: Implement "update" mode when writing schemata file

[Shivappa Vikas]

On Fri, 24 Mar 2017, Fenghua Yu wrote:

> On Fri, Mar 24, 2017 at 10:51:58AM -0700, Luck, Tony wrote:
>> From: Tony Luck <tony.luck@intel.com>
>>
>> The schemata file can have multiple lines and it is cumbersome to
>> update from shell scripts.
>
> "from shell scripts" makes people a bit confused. Not just shell scripts,
> C or Java code also can be cumbersome to update the file, right?
>
>> +Reading/writing the schemata file
>> +---------------------------------
>> +Reading the schemata file will show the state of all resources
>> +on all domains. When writing you only need to specify those values
>> +which you wish to change.  E.g.
>
> User can still write all values including not changed and change just
> like 4.10 does. We may say "When writing you can either write all values
> or only need to specify those values which you wish to change."?

Will integrate these changes to Tony's patch and send it as part of the next 
series.

Thanks,
Vikas

Re: [PATCH] x86/intel_rdt: Implement "update" mode when writing schemata file

[Thomas Gleixner]

On Fri, 24 Mar 2017, Luck, Tony wrote:
> +Reading/writing the schemata file
> +---------------------------------
> +Reading the schemata file will show the state of all resources
> +on all domains. When writing you only need to specify those values
> +which you wish to change.  E.g.
> +
> +# cat schemata
> +L3DATA:0=fffff;1=fffff;2=fffff;3=fffff
> +L3CODE:0=fffff;1=fffff;2=fffff;3=fffff
> +# echo "L3DATA:2=3c0;" > schemata
> +# cat schemata
> +L3DATA:0=fffff;1=fffff;2=3c0;3=fffff
> +L3CODE:0=fffff;1=fffff;2=fffff;3=fffff

It would be nice if the output would cover the mask width, i.e:

L3DATA:0=fffff;1=fffff;2=003c0;3=fffff
L3CODE:0=fffff;1=fffff;2=fffff;3=fffff

because that makes it tabular and simpler to read.

Thanks,

	tglx

Re: [PATCH] x86/intel_rdt: Implement "update" mode when writing schemata file

[Shivappa Vikas]

On Fri, 31 Mar 2017, Thomas Gleixner wrote:

> On Fri, 24 Mar 2017, Luck, Tony wrote:
>> +Reading/writing the schemata file
>> +---------------------------------
>> +Reading the schemata file will show the state of all resources
>> +on all domains. When writing you only need to specify those values
>> +which you wish to change.  E.g.
>> +
>> +# cat schemata
>> +L3DATA:0=fffff;1=fffff;2=fffff;3=fffff
>> +L3CODE:0=fffff;1=fffff;2=fffff;3=fffff
>> +# echo "L3DATA:2=3c0;" > schemata
>> +# cat schemata
>> +L3DATA:0=fffff;1=fffff;2=3c0;3=fffff
>> +L3CODE:0=fffff;1=fffff;2=fffff;3=fffff
>
> It would be nice if the output would cover the mask width, i.e:
>
> L3DATA:0=fffff;1=fffff;2=003c0;3=fffff
> L3CODE:0=fffff;1=fffff;2=fffff;3=fffff
>
> because that makes it tabular and simpler to read.

Will add this.

A sample with MBA:

   L3DATA:0=fffff;1=fffff;2=003c0;3=fffff
   L3CODE:0=fffff;1=fffff;2=fffff;3=fffff
       MB:0=   10;1=   20;2=  100;3=  100

Thanks,
Vikas

>
> Thanks,
>
> 	tglx
>

Re: [PATCH] x86/intel_rdt: Implement "update" mode when writing schemata file

[Thomas Gleixner]

On Fri, 31 Mar 2017, Shivappa Vikas wrote:
> On Fri, 31 Mar 2017, Thomas Gleixner wrote:
> 
> > On Fri, 24 Mar 2017, Luck, Tony wrote:
> > > +Reading/writing the schemata file
> > > +---------------------------------
> > > +Reading the schemata file will show the state of all resources
> > > +on all domains. When writing you only need to specify those values
> > > +which you wish to change.  E.g.
> > > +
> > > +# cat schemata
> > > +L3DATA:0=fffff;1=fffff;2=fffff;3=fffff
> > > +L3CODE:0=fffff;1=fffff;2=fffff;3=fffff
> > > +# echo "L3DATA:2=3c0;" > schemata
> > > +# cat schemata
> > > +L3DATA:0=fffff;1=fffff;2=3c0;3=fffff
> > > +L3CODE:0=fffff;1=fffff;2=fffff;3=fffff
> > 
> > It would be nice if the output would cover the mask width, i.e:
> > 
> > L3DATA:0=fffff;1=fffff;2=003c0;3=fffff
> > L3CODE:0=fffff;1=fffff;2=fffff;3=fffff
> > 
> > because that makes it tabular and simpler to read.
> 
> Will add this.
> 
> A sample with MBA:
> 
>   L3DATA:0=fffff;1=fffff;2=003c0;3=fffff
>   L3CODE:0=fffff;1=fffff;2=fffff;3=fffff
>       MB:0=   10;1=   20;2=  100;3=  100

Well done, the widest group sets the width for all others!

Thanks,

	tglx

Re: [PATCH] x86/intel_rdt: Implement "update" mode when writing schemata file

[Shivappa Vikas]

On Fri, 24 Mar 2017, Luck, Tony wrote:

> +++ b/arch/x86/kernel/cpu/intel_rdt_schemata.c
> @@ -56,7 +56,7 @@ static bool cbm_validate(unsigned long var, struct rdt_resource *r)
>  * Read one cache bit mask (hex). Check that it is valid for the current
>  * resource type.
>  */
> -static int parse_cbm(char *buf, struct rdt_resource *r)
> +static int parse_cbm(char *buf, struct rdt_resource *r, struct rdt_domain *d)
> {
> 	unsigned long data;
> 	int ret;
> @@ -66,7 +66,8 @@ static int parse_cbm(char *buf, struct rdt_resource *r)
> 		return ret;

Would need a check here for repeated domain entries to error out something like:

echo -e "L3:1=7f;1=7;1=7ff" | cat > /sys/fs/resctrl/p1/schemata

  	if (d->have_newcbm)
  		return -EINVAL;


> 	if (!cbm_validate(data, r))
> 		return -EINVAL;
> -	r->tmp_cbms[r->num_tmp_cbms++] = data;
> +	d->new_cbm = data;
> +	d->have_new_cbm = true;
>

[PATCH 3/5] x86/intel_rdt: Fail early on a resource with incorrect domains

[Vikas Shivappa]

When a schemata file is changed, user enters control values for all
domains and all resources in the below format (Consider L3 and L2
resources):

L3:<cache_id0>=<cbm>;<cache_id1>=<cbm>;...
L2:<cache_id0>=<cbm>;<cache_id1>=<cbm>;...

Return error as soon as we detect a resource not entering all domain
values in schemata rather than waiting till we parse all resources
because the entire change is atomic. Also this avoids looping all
enabled resources again.

Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
---
 arch/x86/kernel/cpu/intel_rdt_schemata.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel_rdt_schemata.c b/arch/x86/kernel/cpu/intel_rdt_schemata.c
index 64b43b1..527d042 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_cbms != r->num_domains)
+		return -EINVAL;
+
 	/* Any garbage at the end of the line? */
 	if (line && line[0])
 		return -EINVAL;
@@ -212,14 +216,6 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
 		}
 	}
 
-	/* Did the parser find all the masks we need? */
-	for_each_enabled_rdt_resource(r) {
-		if (r->num_tmp_cbms != r->num_domains) {
-			ret = -EINVAL;
-			goto out;
-		}
-	}
-
 	for_each_enabled_rdt_resource(r) {
 		ret = update_domains(r, closid);
 		if (ret)
-- 
1.9.1

Re: [PATCH 3/5] x86/intel_rdt: Fail early on a resource with incorrect domains

[Thomas Gleixner]

On Fri, 17 Feb 2017, Vikas Shivappa wrote:

> When a schemata file is changed, user enters control values for all
> domains and all resources in the below format (Consider L3 and L2
> resources):
> 
> L3:<cache_id0>=<cbm>;<cache_id1>=<cbm>;...
> L2:<cache_id0>=<cbm>;<cache_id1>=<cbm>;...
> 
> Return error as soon as we detect a resource not entering all domain
> values in schemata rather than waiting till we parse all resources
> because the entire change is atomic.

This sentence makes no sense at all. 

> Also this avoids looping all enabled resources again.

See previous mails. Context, Problem, Solution ....

Thanks,

	tglx

[PATCH 4/5] x86/intel_rdt: Reset the cbm MSR during rmdir

[Vikas Shivappa]

During rmdir reset the ctrl values to all 1s in the QOS_MSR for the
directory's closid. This is done so that that next time when the closid
is reused they dont reflect old values.

Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
---
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
index 8af04af..9b9565f 100644
--- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
+++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
@@ -780,7 +780,7 @@ static struct dentry *rdt_mount(struct file_system_type *fs_type,
 	return dentry;
 }
 
-static int reset_all_cbms(struct rdt_resource *r)
+static int reset_all_ctrls(struct rdt_resource *r, u32 sclosid, u32 eclosid)
 {
 	struct msr_param msr_param;
 	cpumask_var_t cpu_mask;
@@ -791,8 +791,8 @@ static int reset_all_cbms(struct rdt_resource *r)
 		return -ENOMEM;
 
 	msr_param.res = r;
-	msr_param.low = 0;
-	msr_param.high = r->num_closid;
+	msr_param.low = sclosid;
+	msr_param.high = eclosid;
 
 	/*
 	 * Disable resource control for this resource by setting all
@@ -802,7 +802,7 @@ static int reset_all_cbms(struct rdt_resource *r)
 	list_for_each_entry(d, &r->domains, list) {
 		cpumask_set_cpu(cpumask_any(&d->cpu_mask), cpu_mask);
 
-		for (i = 0; i < r->num_closid; i++)
+		for (i = sclosid; i < eclosid; i++)
 			d->cbm[i] = r->max_cbm;
 	}
 	cpu = get_cpu();
@@ -896,7 +896,7 @@ static void rdt_kill_sb(struct super_block *sb)
 
 	/*Put everything back to default values. */
 	for_each_enabled_rdt_resource(r)
-		reset_all_cbms(r);
+		reset_all_ctrls(r, 0, r->num_closid);
 	cdp_disable();
 	rmdir_all_sub();
 	static_branch_disable(&rdt_enable_key);
@@ -991,6 +991,7 @@ static int rdtgroup_rmdir(struct kernfs_node *kn)
 {
 	int ret, cpu, closid = rdtgroup_default.closid;
 	struct rdtgroup *rdtgrp;
+	struct rdt_resource *r;
 	cpumask_var_t tmpmask;
 
 	if (!zalloc_cpumask_var(&tmpmask, GFP_KERNEL))
@@ -1019,6 +1020,13 @@ static int rdtgroup_rmdir(struct kernfs_node *kn)
 	cpumask_or(tmpmask, tmpmask, &rdtgrp->cpu_mask);
 	rdt_update_closid(tmpmask, NULL);
 
+	/*
+	 * Put domain control values back to default for the
+	 * rdtgrp thats being removed.
+	 */
+	for_each_enabled_rdt_resource(r)
+		reset_all_ctrls(r, rdtgrp->closid, rdtgrp->closid + 1);
+
 	rdtgrp->flags = RDT_DELETED;
 	closid_free(rdtgrp->closid);
 	list_del(&rdtgrp->rdtgroup_list);
-- 
1.9.1

Re: [PATCH 4/5] x86/intel_rdt: Reset the cbm MSR during rmdir

[Thomas Gleixner]

On Fri, 17 Feb 2017, Vikas Shivappa wrote:

> During rmdir reset the ctrl values to all 1s in the QOS_MSR for the
> directory's closid. This is done so that that next time when the closid
> is reused they dont reflect old values.

Sigh.

> +static int reset_all_ctrls(struct rdt_resource *r, u32 sclosid, u32 eclosid)

What's so wrong with using 'start' and 'end' instead of these cryptic names? 

>  {
>  	struct msr_param msr_param;
>  	cpumask_var_t cpu_mask;
> @@ -791,8 +791,8 @@ static int reset_all_cbms(struct rdt_resource *r)
>  		return -ENOMEM;
>  
>  	msr_param.res = r;
> -	msr_param.low = 0;
> -	msr_param.high = r->num_closid;
> +	msr_param.low = sclosid;
> +	msr_param.high = eclosid;
>  
>  	/*
>  	 * Disable resource control for this resource by setting all
> @@ -802,7 +802,7 @@ static int reset_all_cbms(struct rdt_resource *r)
>  	list_for_each_entry(d, &r->domains, list) {
>  		cpumask_set_cpu(cpumask_any(&d->cpu_mask), cpu_mask);
>  
> -		for (i = 0; i < r->num_closid; i++)
> +		for (i = sclosid; i < eclosid; i++)

'eclosid' or even when named 'end' is an outright bogus argument to this
function. What you really want is:

static int reset_all_ctrls(struct rdt_resource *r, u32 closid, u32 count)

which works here:

>  	for_each_enabled_rdt_resource(r)
> -		reset_all_cbms(r);
> +		reset_all_ctrls(r, 0, r->num_closid);

and makes this part understandable:

> +	/*
> +	 * Put domain control values back to default for the
> +	 * rdtgrp thats being removed.
> +	 */
> +	for_each_enabled_rdt_resource(r)
> +		reset_all_ctrls(r, rdtgrp->closid, rdtgrp->closid + 1);

		reset_all_ctrls(r, rdtgrp->closid, 1);

What you have done: 

> +		reset_all_ctrls(r, rdtgrp->closid, rdtgrp->closid + 1);

made me really look twice to figure out what the heck this is about.

Thanks,

	tglx

Re: [PATCH 4/5] x86/intel_rdt: Reset the cbm MSR during rmdir

[Shivappa Vikas]

On Wed, 1 Mar 2017, Thomas Gleixner wrote:

> On Fri, 17 Feb 2017, Vikas Shivappa wrote:
>
>> During rmdir reset the ctrl values to all 1s in the QOS_MSR for the
>> directory's closid. This is done so that that next time when the closid
>> is reused they dont reflect old values.
>
> Sigh.
>
>> +static int reset_all_ctrls(struct rdt_resource *r, u32 sclosid, u32 eclosid)
>
> What's so wrong with using 'start' and 'end' instead of these cryptic names?
>
>>  {
>>  	struct msr_param msr_param;
>>  	cpumask_var_t cpu_mask;
>> @@ -791,8 +791,8 @@ static int reset_all_cbms(struct rdt_resource *r)
>>  		return -ENOMEM;
>>
>>  	msr_param.res = r;
>> -	msr_param.low = 0;
>> -	msr_param.high = r->num_closid;
>> +	msr_param.low = sclosid;
>> +	msr_param.high = eclosid;
>>
>>  	/*
>>  	 * Disable resource control for this resource by setting all
>> @@ -802,7 +802,7 @@ static int reset_all_cbms(struct rdt_resource *r)
>>  	list_for_each_entry(d, &r->domains, list) {
>>  		cpumask_set_cpu(cpumask_any(&d->cpu_mask), cpu_mask);
>>
>> -		for (i = 0; i < r->num_closid; i++)
>> +		for (i = sclosid; i < eclosid; i++)
>
> 'eclosid' or even when named 'end' is an outright bogus argument to this
> function. What you really want is:
>
> static int reset_all_ctrls(struct rdt_resource *r, u32 closid, u32 count)
>
> which works here:
>
>>  	for_each_enabled_rdt_resource(r)
>> -		reset_all_cbms(r);
>> +		reset_all_ctrls(r, 0, r->num_closid);
>
> and makes this part understandable:
>
>> +	/*
>> +	 * Put domain control values back to default for the
>> +	 * rdtgrp thats being removed.
>> +	 */
>> +	for_each_enabled_rdt_resource(r)
>> +		reset_all_ctrls(r, rdtgrp->closid, rdtgrp->closid + 1);
>
> 		reset_all_ctrls(r, rdtgrp->closid, 1);

Will fix the parameters to start and count..

>
> What you have done:
>
>> +		reset_all_ctrls(r, rdtgrp->closid, rdtgrp->closid + 1);
>
> made me really look twice to figure out what the heck this is about.
>
> Thanks,
>
> 	tglx
>

[PATCH 5/5] x86/intel_rdt: hotcpu updates for RDT

[Vikas Shivappa]

For closid and rmid, change both the per cpu cache and PQR_MSR to be
cleared only when offlining cpu at the respective handlers.  The other
places to clear them may not be required and is removed.  This can be
done at offlining so that the cache occupancy is not counted soon after
the cpu goes down, rather than waiting to clear it during online cpu.

Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
---
 arch/x86/events/intel/cqm.c     | 10 +++++-----
 arch/x86/kernel/cpu/intel_rdt.c |  1 -
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/x86/events/intel/cqm.c b/arch/x86/events/intel/cqm.c
index 8c00dc0..681e32f 100644
--- a/arch/x86/events/intel/cqm.c
+++ b/arch/x86/events/intel/cqm.c
@@ -1569,13 +1569,8 @@ static inline void cqm_pick_event_reader(int cpu)
 
 static int intel_cqm_cpu_starting(unsigned int cpu)
 {
-	struct intel_pqr_state *state = &per_cpu(pqr_state, cpu);
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
 
-	state->rmid = 0;
-	state->closid = 0;
-	state->rmid_usecnt = 0;
-
 	WARN_ON(c->x86_cache_max_rmid != cqm_max_rmid);
 	WARN_ON(c->x86_cache_occ_scale != cqm_l3_scale);
 
@@ -1585,12 +1580,17 @@ static int intel_cqm_cpu_starting(unsigned int cpu)
 
 static int intel_cqm_cpu_exit(unsigned int cpu)
 {
+	struct intel_pqr_state *state = &per_cpu(pqr_state, cpu);
 	int target;
 
 	/* Is @cpu the current cqm reader for this package ? */
 	if (!cpumask_test_and_clear_cpu(cpu, &cqm_cpumask))
 		return 0;
 
+	state->rmid = 0;
+	state->rmid_usecnt = 0;
+	wrmsr(MSR_IA32_PQR_ASSOC, 0, state->closid);
+
 	/* Find another online reader in this package */
 	target = cpumask_any_but(topology_core_cpumask(cpu), cpu);
 
diff --git a/arch/x86/kernel/cpu/intel_rdt.c b/arch/x86/kernel/cpu/intel_rdt.c
index 5a533fe..c8af5d9 100644
--- a/arch/x86/kernel/cpu/intel_rdt.c
+++ b/arch/x86/kernel/cpu/intel_rdt.c
@@ -350,7 +350,6 @@ static int intel_rdt_online_cpu(unsigned int cpu)
 		domain_add_cpu(cpu, r);
 	/* The cpu is set in default rdtgroup after online. */
 	cpumask_set_cpu(cpu, &rdtgroup_default.cpu_mask);
-	clear_closid(cpu);
 	mutex_unlock(&rdtgroup_mutex);
 
 	return 0;
-- 
1.9.1

Re: [PATCH 5/5] x86/intel_rdt: hotcpu updates for RDT

[Thomas Gleixner]

On Fri, 17 Feb 2017, Vikas Shivappa wrote:

> For closid and rmid, change both the per cpu cache and PQR_MSR to be
> cleared only when offlining cpu at the respective handlers.  The other
> places to clear them may not be required and is removed.  This can be
> done at offlining so that the cache occupancy is not counted soon after
> the cpu goes down, rather than waiting to clear it during online cpu.

Yet another unstructured lump of blurb describing the WHAT and not the WHY.

> diff --git a/arch/x86/events/intel/cqm.c b/arch/x86/events/intel/cqm.c
> index 8c00dc0..681e32f 100644
> --- a/arch/x86/events/intel/cqm.c
> +++ b/arch/x86/events/intel/cqm.c
> @@ -1569,13 +1569,8 @@ static inline void cqm_pick_event_reader(int cpu)
>  
>  static int intel_cqm_cpu_starting(unsigned int cpu)
>  {
> -	struct intel_pqr_state *state = &per_cpu(pqr_state, cpu);
>  	struct cpuinfo_x86 *c = &cpu_data(cpu);
>  
> -	state->rmid = 0;
> -	state->closid = 0;
> -	state->rmid_usecnt = 0;
> -
>  	WARN_ON(c->x86_cache_max_rmid != cqm_max_rmid);
>  	WARN_ON(c->x86_cache_occ_scale != cqm_l3_scale);
>  
> @@ -1585,12 +1580,17 @@ static int intel_cqm_cpu_starting(unsigned int cpu)
>  
>  static int intel_cqm_cpu_exit(unsigned int cpu)
>  {
> +	struct intel_pqr_state *state = &per_cpu(pqr_state, cpu);

Can be this_cpu_ptr() because the callback is guaranteed to run on the
outgoing CPU.

>  	int target;
>  
>  	/* Is @cpu the current cqm reader for this package ? */
>  	if (!cpumask_test_and_clear_cpu(cpu, &cqm_cpumask))
>  		return 0;

So if the CPU is not the current cqm reader then the per cpu state of this
CPU is left stale. Great improvement.

> +	state->rmid = 0;
> +	state->rmid_usecnt = 0;
> +	wrmsr(MSR_IA32_PQR_ASSOC, 0, state->closid);

What clears state->closid? And what guarantees that state->rmid is not
updated before the CPU has really gone away?

I doubt that this is correct, but if it is, then this lacks a big fat
comment explaining WHY.

Thanks,

	tglx

Re: [PATCH 5/5] x86/intel_rdt: hotcpu updates for RDT

[Shivappa Vikas]

On Wed, 1 Mar 2017, Thomas Gleixner wrote:

>>  	WARN_ON(c->x86_cache_occ_scale != cqm_l3_scale);
>>
>> @@ -1585,12 +1580,17 @@ static int intel_cqm_cpu_starting(unsigned int cpu)
>>
>>  static int intel_cqm_cpu_exit(unsigned int cpu)
>>  {
>> +	struct intel_pqr_state *state = &per_cpu(pqr_state, cpu);
>
> Can be this_cpu_ptr() because the callback is guaranteed to run on the
> outgoing CPU.

Will fix this. Assumed the calls are setup cache alloc way - 
cpuhp_setup_state(CPUHP_AP_ONLINE_DYN ..

>
>>  	int target;
>>
>>  	/* Is @cpu the current cqm reader for this package ? */
>>  	if (!cpumask_test_and_clear_cpu(cpu, &cqm_cpumask))
>>  		return 0;
>
> So if the CPU is not the current cqm reader then the per cpu state of this
> CPU is left stale. Great improvement.
>
>> +	state->rmid = 0;
>> +	state->rmid_usecnt = 0;
>> +	wrmsr(MSR_IA32_PQR_ASSOC, 0, state->closid);
>
> What clears state->closid? And what guarantees that state->rmid is not
> updated before the CPU has really gone away?

- The rdt code takes care of clearing closid state now. Will update the comment. 
- The cqm however was never writing a zero to PQR_ASSOC.

So the update needs to be - to remove the state->closid = 0 from cqm code as the 
rdt code takes care of closid state in clear_closid() called from both offline and 
online cpu.
And also write a rmid = 0 to PQR_ASSOC.

We can integrate the two of these hot cpu calls(from cat and cqm) to write PQR 
only once.

guess I can skip all of these and send it as part of cqm changes we planned 
anyways, because this is really a cqm change.

Thanks,
Vikas