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

[Thomas Gleixner <tglx@linutronix.de>]

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