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

[Thomas Gleixner <tglx@linutronix.de>]

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