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

Groups > linux.kernel > #1583673 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] x86/intel_rdt: Improvements/Fixes to RDT framework  Vikas Shivappa <vikas.shivappa@linux.intel.com> - 2017-02-17 20:50 +0100
    [PATCH 1/5] x86/intel_rdt: Update control registers only when user really modifies it Vikas Shivappa <vikas.shivappa@linux.intel.com> - 2017-02-17 20:50 +0100
      Re: [PATCH 1/5] x86/intel_rdt: Update control registers only when  user really modifies it Thomas Gleixner <tglx@linutronix.de> - 2017-03-01 16:10 +0100
        Re: [PATCH 1/5] x86/intel_rdt: Update control registers only when  user really modifies it Shivappa Vikas <vikas.shivappa@linux.intel.com> - 2017-03-10 01:00 +0100
    [PATCH 5/5] x86/intel_rdt: hotcpu updates for RDT Vikas Shivappa <vikas.shivappa@linux.intel.com> - 2017-02-17 20:50 +0100
      Re: [PATCH 5/5] x86/intel_rdt: hotcpu updates for RDT Thomas Gleixner <tglx@linutronix.de> - 2017-03-01 16:10 +0100
    [PATCH 3/5] x86/intel_rdt: Fail early on a resource with incorrect domains Vikas Shivappa <vikas.shivappa@linux.intel.com> - 2017-02-17 20:50 +0100
      Re: [PATCH 3/5] x86/intel_rdt: Fail early on a resource with incorrect  domains Thomas Gleixner <tglx@linutronix.de> - 2017-03-01 16:40 +0100

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

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

[toc] | [next] | [standalone]

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

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

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

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

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

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

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

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
>

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

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

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

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

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

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

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

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

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

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

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web