linux-kernel - Re: [PATCH 3/3] x86/intel_rdt: Update closid in PQR_ASSOC registers in synchronous mode when changing "cpus"

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.20.1611151435550.3602@nanos>
Date:   Tue, 15 Nov 2016 14:43:52 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Fenghua Yu <fenghua.yu@...el.com>
cc:     "H. Peter Anvin" <h.peter.anvin@...el.com>,
        Ingo Molnar <mingo@...e.hu>, Tony Luck <tony.luck@...el.com>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        Sai Prakhya <sai.praneeth.prakhya@...el.com>,
        Vikas Shivappa <vikas.shivappa@...ux.intel.com>,
        linux-kernel <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org>
Subject: Re: [PATCH 3/3] x86/intel_rdt: Update closid in PQR_ASSOC registers
 in synchronous mode when changing "cpus"

On Fri, 11 Nov 2016, Fenghua Yu wrote:

> x86/intel_rdt: Update closid in PQR_ASSOC registers in synchronous mode when changing "cpus"

Subject lines are supposed to be short and precise.

> When "cpus" is changed in a rdtgroup, the current code doesn't update
> PQR_ASSOC registers with new closid in the cpus write operation.
> The PQR_ASSOC registers are updated asynchronously later when new
> processes are scheduled in on the CPUs.
> 
> A process may run on a CPU for long time without being switched to
> another process, e.g. high performance computing or real time cases.
> Then closid in the PQR_ASSOC register on the CPU is not updated for
> long time until a new process is switched in.

Can you please be a bit more careful about your changelogs? The above is
reads like a fairy tale.
 
> This patch updates closid in PQR_ASSOC synchronously when writing

'This patch' - Grrr! I told you more than once that 'This patch' is
crap. We all know that this is a PATCH. See
Documentation/SubmittingPatches.

> +/*
> + * MSR_IA32_PQR_ASSOC is scoped per logical CPU, so all updates
> + * are always in thread context.
> + */
> +static void rdt_update_pqr_assoc_closid(void *v)
> +{
> +	struct intel_pqr_state *state = this_cpu_ptr(&pqr_state);
> +
> +	state->closid = *(int *)v;
> +	wrmsr(MSR_IA32_PQR_ASSOC, state->rmid, state->closid);
> +}
> +
> +static void rdt_update_pqr_assoc(const struct cpumask *cpu_mask, int closid)
> +{
> +	int cpu = get_cpu();
> +
> +	/* Update PQR_ASSOC MSR on this cpu if it's in cpu_mask. */
> +	if (cpumask_test_cpu(cpu, cpu_mask))
> +		rdt_update_pqr_assoc_closid(&closid);
> +	/* Update PQR_ASSOC MSR on the rest of cpus. */
> +	smp_call_function_many(cpu_mask, rdt_update_pqr_assoc_closid,
> +			       &closid, 1);
> +	put_cpu();
> +}

>  static void rmdir_all_sub(void)
> @@ -809,13 +828,9 @@ static void rmdir_all_sub(void)
>  		t->closid = 0;
>  	read_unlock(&tasklist_lock);
>  
> -	get_cpu();
> -	/* Reset PQR_ASSOC MSR on this cpu. */
> -	rdt_reset_pqr_assoc_closid(NULL);
> -	/* Reset PQR_ASSOC MSR on the rest of cpus. */
> -	smp_call_function_many(cpu_online_mask, rdt_reset_pqr_assoc_closid,
> -			       NULL, 1);
> -	put_cpu();
> +	/* Reset PQR_ASSOC MSR on all online cpus */
> +	rdt_update_pqr_assoc(cpu_online_mask, 0);

cpu_online_mask is not protected against changing before you hit the
get_cpu() in rdt_update_pqr_assoc().

Thanks,

	tglx