Date: Wed, 1 Mar 2017 15:24:59 +0100 (CET)
From: Thomas Gleixner <tglx@linutronix.de>
To: Vikas Shivappa <vikas.shivappa@linux.intel.com>
cc: vikas.shivappa@intel.com, linux-kernel@vger.kernel.org, x86@kernel.org,
        hpa@zytor.com, mingo@kernel.org, peterz@infradead.org,
        ravi.v.shankar@intel.com, tony.luck@intel.com, fenghua.yu@intel.com,
        andi.kleen@intel.com
Subject: Re: [PATCH 5/5] x86/intel_rdt: hotcpu updates for RDT
In-Reply-To: <1487360328-6768-6-git-send-email-vikas.shivappa@linux.intel.com>
Message-ID: <alpine.DEB.2.20.1703011511320.4005@nanos>
References: <1487360328-6768-1-git-send-email-vikas.shivappa@linux.intel.com> <1487360328-6768-6-git-send-email-vikas.shivappa@linux.intel.com>
User-Agent: Alpine 2.20 (DEB 67 2015-01-07)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1974
Lines: 59

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