On Fri, 11 Nov 2016, Fenghua Yu wrote:
> +/*
> + * MSR_IA32_PQR_ASSOC is scoped per logical CPU, so all updates
> + * are always in thread context.
And this comment tells us what? Nothing useful. It lacks the most important
information why this is safe against a logical CPU switching the MSR right
at this moment during context switch. It's safe because the pqr_switch_to
function is called with interrupts disabled.
> + */
> +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 ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of,
> char *buf, size_t nbytes, loff_t off)
> {
> @@ -238,6 +263,9 @@ static ssize_t rdtgroup_cpus_write(struct
> kernfs_open_file *of,
> &rdtgroup_default.cpu_mask, tmpmask);
> for_each_cpu(cpu, tmpmask)
> per_cpu(cpu_closid, cpu) = 0;
> +
> + /* Update PQR_ASSOC registers on the dropped cpus */
> + rdt_update_pqr_assoc(tmpmask, rdtgroup_default.closid);
Grr. You store the new closid now in the for_each_cpu() loop and in the smp
function call.
> }
>
> /*
> @@ -253,6 +281,9 @@ static ssize_t rdtgroup_cpus_write(struct
> kernfs_open_file *of,
> }
> for_each_cpu(cpu, tmpmask)
> per_cpu(cpu_closid, cpu) = rdtgrp->closid;
> +
> + /* Update PQR_ASSOC registers on the added cpus */
> + rdt_update_pqr_assoc(tmpmask, rdtgrp->closid);
Ditto.
> }
>
> /* Done pushing/pulling - update this group with new mask */
> @@ -783,18 +814,6 @@ static int reset_all_cbms(struct rdt_resource *r)
> }
>
> /*
> - * MSR_IA32_PQR_ASSOC is scoped per logical CPU, so all updates
> - * are always in thread context.
> - */
> -static void rdt_reset_pqr_assoc_closid(void *v)
> -{
> - struct intel_pqr_state *state = this_cpu_ptr(&pqr_state);
> -
> - state->closid = 0;
> - wrmsr(MSR_IA32_PQR_ASSOC, state->rmid, 0);
> -}
> -
> -/*
> * Forcibly remove all of subdirectories under root.
> */
> 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);
And here you have the extra closid loop later on. Really useful.
Thanks,
tglx
