Messages in this thread | | | Date | Wed, 1 Mar 2017 15:24:59 +0100 (CET) | From | Thomas Gleixner <> | Subject | 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
| |