* [PATCH 1/3] x86/intel_rdt: Fix kernfs_to_rdtgroup to know about "info/*" directories @ 2016-11-12 1:02 Fenghua Yu 2016-11-12 1:02 ` [PATCH 2/3] x86/intel_rdt: Return state to default on umount Fenghua Yu ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Fenghua Yu @ 2016-11-12 1:02 UTC (permalink / raw) To: Thomas Gleixner Cc: H. Peter Anvin, Ingo Molnar, Tony Luck, Ravi V Shankar, Sai Prakhya, Vikas Shivappa, linux-kernel, x86, Fenghua Yu From: Fenghua Yu <fenghua.yu@intel.com> We saw a kernel oops on NULL pointer when we ran "rmdir info". This is because the "info" directory and per-resource subdirectories of "info" do not use "kn->priv" to point to a "struct rdtgroup". Also change error code from -ENOENT to the more appropriate -EPERM so the user sees "Operation not permitted". Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> --- arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c index 4795880..cff286e 100644 --- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c @@ -644,16 +644,29 @@ static int parse_rdtgroupfs_options(char *data) */ static struct rdtgroup *kernfs_to_rdtgroup(struct kernfs_node *kn) { - if (kernfs_type(kn) == KERNFS_DIR) - return kn->priv; - else + if (kernfs_type(kn) == KERNFS_DIR) { + /* + * All the resource directories use "kn->priv" + * to point to the "struct rdtgroup" for the + * resource. "info" and its subdirectories don't + * have rdtgroup structures, so return NULL here. + */ + if (kn == kn_info || kn->parent == kn_info) + return NULL; + else + return kn->priv; + } else { return kn->parent->priv; + } } struct rdtgroup *rdtgroup_kn_lock_live(struct kernfs_node *kn) { struct rdtgroup *rdtgrp = kernfs_to_rdtgroup(kn); + if (!rdtgrp) + return NULL; + atomic_inc(&rdtgrp->waitcount); kernfs_break_active_protection(kn); @@ -670,6 +683,9 @@ void rdtgroup_kn_unlock(struct kernfs_node *kn) { struct rdtgroup *rdtgrp = kernfs_to_rdtgroup(kn); + if (!rdtgrp) + return; + mutex_unlock(&rdtgroup_mutex); if (atomic_dec_and_test(&rdtgrp->waitcount) && @@ -918,7 +934,7 @@ static int rdtgroup_rmdir(struct kernfs_node *kn) rdtgrp = rdtgroup_kn_lock_live(kn); if (!rdtgrp) { rdtgroup_kn_unlock(kn); - return -ENOENT; + return -EPERM; } /* Give any tasks back to the default group */ -- 2.5.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] x86/intel_rdt: Return state to default on umount 2016-11-12 1:02 [PATCH 1/3] x86/intel_rdt: Fix kernfs_to_rdtgroup to know about "info/*" directories Fenghua Yu @ 2016-11-12 1:02 ` Fenghua Yu 2016-11-15 13:35 ` Thomas Gleixner 2016-11-15 17:44 ` [tip:x86/cache] x86/intel_rdt: Reset per cpu closids on unmount tip-bot for Fenghua Yu 2016-11-12 1:02 ` [PATCH 3/3] x86/intel_rdt: Update closid in PQR_ASSOC registers in synchronous mode when changing "cpus" Fenghua Yu 2016-11-15 17:42 ` [tip:x86/cache] x86/intel_rdt: Protect info directory from removal tip-bot for Fenghua Yu 2 siblings, 2 replies; 11+ messages in thread From: Fenghua Yu @ 2016-11-12 1:02 UTC (permalink / raw) To: Thomas Gleixner Cc: H. Peter Anvin, Ingo Molnar, Tony Luck, Ravi V Shankar, Sai Prakhya, Vikas Shivappa, linux-kernel, x86, Fenghua Yu From: Fenghua Yu <fenghua.yu@intel.com> All CPUs in a rdtgroup are given back to the default rdtgroup before the rdtgroup is removed during umount. After umount, the default rdtgroup contains all online CPUs. cpu_closid needs to be cleared for each cpu in root rdtgroup during umount. Otherwise, PQR_ASSOC will be written to the non-zero value stored in cpu_closid after next mount. Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> --- arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c index cff286e..416b95e 100644 --- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c @@ -801,6 +801,7 @@ static void rmdir_all_sub(void) { struct rdtgroup *rdtgrp, *tmp; struct task_struct *p, *t; + int cpu; /* move all tasks to default resource group */ read_lock(&tasklist_lock); @@ -819,10 +820,19 @@ static void rmdir_all_sub(void) /* Remove each rdtgroup other than root */ if (rdtgrp == &rdtgroup_default) continue; + + /* Give any CPUs back to the default group */ + cpumask_or(&rdtgroup_default.cpu_mask, + &rdtgroup_default.cpu_mask, &rdtgrp->cpu_mask); + kernfs_remove(rdtgrp->kn); list_del(&rdtgrp->rdtgroup_list); kfree(rdtgrp); } + + for_each_cpu(cpu, &rdtgroup_default.cpu_mask) + per_cpu(cpu_closid, cpu) = 0; + kernfs_remove(kn_info); } -- 2.5.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] x86/intel_rdt: Return state to default on umount 2016-11-12 1:02 ` [PATCH 2/3] x86/intel_rdt: Return state to default on umount Fenghua Yu @ 2016-11-15 13:35 ` Thomas Gleixner 2016-11-15 17:28 ` Thomas Gleixner 2016-11-15 17:44 ` [tip:x86/cache] x86/intel_rdt: Reset per cpu closids on unmount tip-bot for Fenghua Yu 1 sibling, 1 reply; 11+ messages in thread From: Thomas Gleixner @ 2016-11-15 13:35 UTC (permalink / raw) To: Fenghua Yu Cc: H. Peter Anvin, Ingo Molnar, Tony Luck, Ravi V Shankar, Sai Prakhya, Vikas Shivappa, linux-kernel, x86 On Fri, 11 Nov 2016, Fenghua Yu wrote: > @@ -801,6 +801,7 @@ static void rmdir_all_sub(void) > { > struct rdtgroup *rdtgrp, *tmp; > struct task_struct *p, *t; > + int cpu; > > /* move all tasks to default resource group */ > read_lock(&tasklist_lock); > @@ -819,10 +820,19 @@ static void rmdir_all_sub(void) > /* Remove each rdtgroup other than root */ > if (rdtgrp == &rdtgroup_default) > continue; > + > + /* Give any CPUs back to the default group */ > + cpumask_or(&rdtgroup_default.cpu_mask, > + &rdtgroup_default.cpu_mask, &rdtgrp->cpu_mask); That's a pointless exercise. We can simply copy cpu_online_mask to the default group mask and be done with it. I'll fix that up. Thanks, tglx ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] x86/intel_rdt: Return state to default on umount 2016-11-15 13:35 ` Thomas Gleixner @ 2016-11-15 17:28 ` Thomas Gleixner 0 siblings, 0 replies; 11+ messages in thread From: Thomas Gleixner @ 2016-11-15 17:28 UTC (permalink / raw) To: Fenghua Yu Cc: H. Peter Anvin, Ingo Molnar, Tony Luck, Ravi V Shankar, Sai Prakhya, Vikas Shivappa, linux-kernel, x86 On Tue, 15 Nov 2016, Thomas Gleixner wrote: > On Fri, 11 Nov 2016, Fenghua Yu wrote: > > @@ -801,6 +801,7 @@ static void rmdir_all_sub(void) > > { > > struct rdtgroup *rdtgrp, *tmp; > > struct task_struct *p, *t; > > + int cpu; > > > > /* move all tasks to default resource group */ > > read_lock(&tasklist_lock); > > @@ -819,10 +820,19 @@ static void rmdir_all_sub(void) > > /* Remove each rdtgroup other than root */ > > if (rdtgrp == &rdtgroup_default) > > continue; > > + > > + /* Give any CPUs back to the default group */ > > + cpumask_or(&rdtgroup_default.cpu_mask, > > + &rdtgroup_default.cpu_mask, &rdtgrp->cpu_mask); > > That's a pointless exercise. We can simply copy cpu_online_mask to the > default group mask and be done with it. I'll fix that up. Actually we cannot. A CPU which already run the offline callback might still be marked online. So the above is correct. Thanks, tglx ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tip:x86/cache] x86/intel_rdt: Reset per cpu closids on unmount 2016-11-12 1:02 ` [PATCH 2/3] x86/intel_rdt: Return state to default on umount Fenghua Yu 2016-11-15 13:35 ` Thomas Gleixner @ 2016-11-15 17:44 ` tip-bot for Fenghua Yu 1 sibling, 0 replies; 11+ messages in thread From: tip-bot for Fenghua Yu @ 2016-11-15 17:44 UTC (permalink / raw) To: linux-tip-commits Cc: mingo, h.peter.anvin, hpa, tglx, vikas.shivappa, mingo, fenghua.yu, sai.praneeth.prakhya, linux-kernel, tony.luck, ravi.v.shankar Commit-ID: c7cc0cc10cdecc275211c8749defba6c41aaf5de Gitweb: http://git.kernel.org/tip/c7cc0cc10cdecc275211c8749defba6c41aaf5de Author: Fenghua Yu <fenghua.yu@intel.com> AuthorDate: Fri, 11 Nov 2016 17:02:37 -0800 Committer: Thomas Gleixner <tglx@linutronix.de> CommitDate: Tue, 15 Nov 2016 18:35:50 +0100 x86/intel_rdt: Reset per cpu closids on unmount All CPUs in a rdtgroup are given back to the default rdtgroup before the rdtgroup is removed during umount. After umount, the default rdtgroup contains all online CPUs, but the per cpu closids are not cleared. As a result the stale closid value will be used immediately after the next mount. Move all cpus to the default group and update the percpu closid storage. [ tglx: Massaged changelong ] Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> Cc: "Ravi V Shankar" <ravi.v.shankar@intel.com> Cc: "Tony Luck" <tony.luck@intel.com> Cc: "Sai Prakhya" <sai.praneeth.prakhya@intel.com> Cc: "Vikas Shivappa" <vikas.shivappa@linux.intel.com> Cc: "Ingo Molnar" <mingo@elte.hu> Cc: "H. Peter Anvin" <h.peter.anvin@intel.com> Link: http://lkml.kernel.org/r/1478912558-55514-2-git-send-email-fenghua.yu@intel.com Signed-off-by: Thomas Gleixner <tglx@linutronix.de> --- arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c index 2f54931..d6bad09 100644 --- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c @@ -799,6 +799,7 @@ static void rmdir_all_sub(void) { struct rdtgroup *rdtgrp, *tmp; struct task_struct *p, *t; + int cpu; /* move all tasks to default resource group */ read_lock(&tasklist_lock); @@ -813,14 +814,29 @@ static void rmdir_all_sub(void) smp_call_function_many(cpu_online_mask, rdt_reset_pqr_assoc_closid, NULL, 1); put_cpu(); + list_for_each_entry_safe(rdtgrp, tmp, &rdt_all_groups, rdtgroup_list) { /* Remove each rdtgroup other than root */ if (rdtgrp == &rdtgroup_default) continue; + + /* + * Give any CPUs back to the default group. We cannot copy + * cpu_online_mask because a CPU might have executed the + * offline callback already, but is still marked online. + */ + cpumask_or(&rdtgroup_default.cpu_mask, + &rdtgroup_default.cpu_mask, &rdtgrp->cpu_mask); + kernfs_remove(rdtgrp->kn); list_del(&rdtgrp->rdtgroup_list); kfree(rdtgrp); } + + /* Reset all per cpu closids to the default value */ + for_each_cpu(cpu, &rdtgroup_default.cpu_mask) + per_cpu(cpu_closid, cpu) = 0; + kernfs_remove(kn_info); } ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] x86/intel_rdt: Update closid in PQR_ASSOC registers in synchronous mode when changing "cpus" 2016-11-12 1:02 [PATCH 1/3] x86/intel_rdt: Fix kernfs_to_rdtgroup to know about "info/*" directories Fenghua Yu 2016-11-12 1:02 ` [PATCH 2/3] x86/intel_rdt: Return state to default on umount Fenghua Yu @ 2016-11-12 1:02 ` Fenghua Yu 2016-11-15 13:43 ` Thomas Gleixner ` (2 more replies) 2016-11-15 17:42 ` [tip:x86/cache] x86/intel_rdt: Protect info directory from removal tip-bot for Fenghua Yu 2 siblings, 3 replies; 11+ messages in thread From: Fenghua Yu @ 2016-11-12 1:02 UTC (permalink / raw) To: Thomas Gleixner Cc: H. Peter Anvin, Ingo Molnar, Tony Luck, Ravi V Shankar, Sai Prakhya, Vikas Shivappa, linux-kernel, x86, Fenghua Yu From: Fenghua Yu <fenghua.yu@intel.com> 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. This patch updates closid in PQR_ASSOC synchronously when writing cpus to avoid the above issue. Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> --- arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 53 ++++++++++++++++++++------------ 1 file changed, 34 insertions(+), 19 deletions(-) diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c index 416b95e..a45f2ba 100644 --- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c @@ -191,6 +191,31 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of, return ret; } +/* + * 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 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); } /* @@ -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); } /* 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); + list_for_each_entry_safe(rdtgrp, tmp, &rdt_all_groups, rdtgroup_list) { /* Remove each rdtgroup other than root */ if (rdtgrp == &rdtgroup_default) -- 2.5.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] x86/intel_rdt: Update closid in PQR_ASSOC registers in synchronous mode when changing "cpus" 2016-11-12 1:02 ` [PATCH 3/3] x86/intel_rdt: Update closid in PQR_ASSOC registers in synchronous mode when changing "cpus" Fenghua Yu @ 2016-11-15 13:43 ` Thomas Gleixner 2016-11-15 14:00 ` Thomas Gleixner 2016-11-15 17:44 ` [tip:x86/cache] x86/intel_rdt: Update percpu closid immeditately on CPUs affected by changee tip-bot for Fenghua Yu 2 siblings, 0 replies; 11+ messages in thread From: Thomas Gleixner @ 2016-11-15 13:43 UTC (permalink / raw) To: Fenghua Yu Cc: H. Peter Anvin, Ingo Molnar, Tony Luck, Ravi V Shankar, Sai Prakhya, Vikas Shivappa, linux-kernel, x86 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] x86/intel_rdt: Update closid in PQR_ASSOC registers in synchronous mode when changing "cpus" 2016-11-12 1:02 ` [PATCH 3/3] x86/intel_rdt: Update closid in PQR_ASSOC registers in synchronous mode when changing "cpus" Fenghua Yu 2016-11-15 13:43 ` Thomas Gleixner @ 2016-11-15 14:00 ` Thomas Gleixner 2016-11-15 16:40 ` Thomas Gleixner 2016-11-15 17:44 ` [tip:x86/cache] x86/intel_rdt: Update percpu closid immeditately on CPUs affected by changee tip-bot for Fenghua Yu 2 siblings, 1 reply; 11+ messages in thread From: Thomas Gleixner @ 2016-11-15 14:00 UTC (permalink / raw) To: Fenghua Yu Cc: H. Peter Anvin, Ingo Molnar, Tony Luck, Ravi V Shankar, Sai Prakhya, Vikas Shivappa, linux-kernel, x86 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] x86/intel_rdt: Update closid in PQR_ASSOC registers in synchronous mode when changing "cpus" 2016-11-15 14:00 ` Thomas Gleixner @ 2016-11-15 16:40 ` Thomas Gleixner 0 siblings, 0 replies; 11+ messages in thread From: Thomas Gleixner @ 2016-11-15 16:40 UTC (permalink / raw) To: Fenghua Yu Cc: H. Peter Anvin, Ingo Molnar, Tony Luck, Ravi V Shankar, Sai Prakhya, Vikas Shivappa, linux-kernel, x86 On Tue, 15 Nov 2016, Thomas Gleixner wrote: > 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); Furthermore, unconditionally writting the MSR is Just Wrong because the current executing task might have it's own closid set which gets overwritten by this. No cookies! tglx ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tip:x86/cache] x86/intel_rdt: Update percpu closid immeditately on CPUs affected by changee 2016-11-12 1:02 ` [PATCH 3/3] x86/intel_rdt: Update closid in PQR_ASSOC registers in synchronous mode when changing "cpus" Fenghua Yu 2016-11-15 13:43 ` Thomas Gleixner 2016-11-15 14:00 ` Thomas Gleixner @ 2016-11-15 17:44 ` tip-bot for Fenghua Yu 2 siblings, 0 replies; 11+ messages in thread From: tip-bot for Fenghua Yu @ 2016-11-15 17:44 UTC (permalink / raw) To: linux-tip-commits Cc: mingo, h.peter.anvin, ravi.v.shankar, sai.praneeth.prakhya, tglx, tony.luck, vikas.shivappa, linux-kernel, fenghua.yu, hpa, mingo Commit-ID: f410770293a1fbc08906474c24104a7a11943eb6 Gitweb: http://git.kernel.org/tip/f410770293a1fbc08906474c24104a7a11943eb6 Author: Fenghua Yu <fenghua.yu@intel.com> AuthorDate: Fri, 11 Nov 2016 17:02:38 -0800 Committer: Thomas Gleixner <tglx@linutronix.de> CommitDate: Tue, 15 Nov 2016 18:35:50 +0100 x86/intel_rdt: Update percpu closid immeditately on CPUs affected by changee If CPUs are moved to or removed from a rdtgroup, the percpu closid storage is updated. If tasks running on an affected CPU use the percpu closid then the PQR_ASSOC MSR is only updated when the task runs through a context switch. Up to the context switch the CPUs operate on the wrong closid. This state is potentially unbound. Make the change immediately effective by invoking a smp function call on the affected CPUs which stores the new closid in the perpu storage and calls the rdt_sched_in() function which updates the MSR, if the current task uses the percpu closid. [ tglx: Made it work and massaged changelog once more ] Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> Cc: "Ravi V Shankar" <ravi.v.shankar@intel.com> Cc: "Tony Luck" <tony.luck@intel.com> Cc: "Sai Prakhya" <sai.praneeth.prakhya@intel.com> Cc: "Vikas Shivappa" <vikas.shivappa@linux.intel.com> Cc: "Ingo Molnar" <mingo@elte.hu> Cc: "H. Peter Anvin" <h.peter.anvin@intel.com> Link: http://lkml.kernel.org/r/1478912558-55514-3-git-send-email-fenghua.yu@intel.com Signed-off-by: Thomas Gleixner <tglx@linutronix.de> --- arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 72 ++++++++++++++++---------------- 1 file changed, 36 insertions(+), 36 deletions(-) diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c index d6bad09..98edba4 100644 --- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c @@ -191,12 +191,40 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of, return ret; } +/* + * This is safe against intel_rdt_sched_in() called from __switch_to() + * because __switch_to() is executed with interrupts disabled. A local call + * from rdt_update_percpu_closid() is proteced against __switch_to() because + * preemption is disabled. + */ +static void rdt_update_cpu_closid(void *v) +{ + this_cpu_write(cpu_closid, *(int *)v); + /* + * We cannot unconditionally write the MSR because the current + * executing task might have its own closid selected. Just reuse + * the context switch code. + */ + intel_rdt_sched_in(); +} + +/* Update the per cpu closid and eventually the PGR_ASSOC MSR */ +static void rdt_update_percpu_closid(const struct cpumask *cpu_mask, int closid) +{ + int cpu = get_cpu(); + + if (cpumask_test_cpu(cpu, cpu_mask)) + rdt_update_cpu_closid(&closid); + smp_call_function_many(cpu_mask, rdt_update_cpu_closid, &closid, 1); + put_cpu(); +} + static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of, char *buf, size_t nbytes, loff_t off) { cpumask_var_t tmpmask, newmask; struct rdtgroup *rdtgrp, *r; - int ret, cpu; + int ret; if (!buf) return -EINVAL; @@ -236,8 +264,7 @@ static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of, /* Give any dropped cpus to rdtgroup_default */ cpumask_or(&rdtgroup_default.cpu_mask, &rdtgroup_default.cpu_mask, tmpmask); - for_each_cpu(cpu, tmpmask) - per_cpu(cpu_closid, cpu) = 0; + rdt_update_percpu_closid(tmpmask, rdtgroup_default.closid); } /* @@ -251,8 +278,7 @@ static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of, continue; cpumask_andnot(&r->cpu_mask, &r->cpu_mask, tmpmask); } - for_each_cpu(cpu, tmpmask) - per_cpu(cpu_closid, cpu) = rdtgrp->closid; + rdt_update_percpu_closid(tmpmask, rdtgroup_default.closid); } /* Done pushing/pulling - update this group with new mask */ @@ -781,25 +807,12 @@ 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) { struct rdtgroup *rdtgrp, *tmp; struct task_struct *p, *t; - int cpu; /* move all tasks to default resource group */ read_lock(&tasklist_lock); @@ -807,14 +820,6 @@ 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(); - list_for_each_entry_safe(rdtgrp, tmp, &rdt_all_groups, rdtgroup_list) { /* Remove each rdtgroup other than root */ if (rdtgrp == &rdtgroup_default) @@ -828,15 +833,13 @@ static void rmdir_all_sub(void) cpumask_or(&rdtgroup_default.cpu_mask, &rdtgroup_default.cpu_mask, &rdtgrp->cpu_mask); + rdt_update_percpu_closid(&rdtgrp->cpu_mask, + rdtgroup_default.closid); + kernfs_remove(rdtgrp->kn); list_del(&rdtgrp->rdtgroup_list); kfree(rdtgrp); } - - /* Reset all per cpu closids to the default value */ - for_each_cpu(cpu, &rdtgroup_default.cpu_mask) - per_cpu(cpu_closid, cpu) = 0; - kernfs_remove(kn_info); } @@ -943,7 +946,6 @@ static int rdtgroup_rmdir(struct kernfs_node *kn) { struct task_struct *p, *t; struct rdtgroup *rdtgrp; - int cpu, ret = 0; rdtgrp = rdtgroup_kn_lock_live(kn); if (!rdtgrp) { @@ -962,8 +964,7 @@ static int rdtgroup_rmdir(struct kernfs_node *kn) /* Give any CPUs back to the default group */ cpumask_or(&rdtgroup_default.cpu_mask, &rdtgroup_default.cpu_mask, &rdtgrp->cpu_mask); - for_each_cpu(cpu, &rdtgrp->cpu_mask) - per_cpu(cpu_closid, cpu) = 0; + rdt_update_percpu_closid(&rdtgrp->cpu_mask, rdtgroup_default.closid); rdtgrp->flags = RDT_DELETED; closid_free(rdtgrp->closid); @@ -977,8 +978,7 @@ static int rdtgroup_rmdir(struct kernfs_node *kn) kernfs_remove(rdtgrp->kn); rdtgroup_kn_unlock(kn); - - return ret; + return 0; } static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = { ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [tip:x86/cache] x86/intel_rdt: Protect info directory from removal 2016-11-12 1:02 [PATCH 1/3] x86/intel_rdt: Fix kernfs_to_rdtgroup to know about "info/*" directories Fenghua Yu 2016-11-12 1:02 ` [PATCH 2/3] x86/intel_rdt: Return state to default on umount Fenghua Yu 2016-11-12 1:02 ` [PATCH 3/3] x86/intel_rdt: Update closid in PQR_ASSOC registers in synchronous mode when changing "cpus" Fenghua Yu @ 2016-11-15 17:42 ` tip-bot for Fenghua Yu 2 siblings, 0 replies; 11+ messages in thread From: tip-bot for Fenghua Yu @ 2016-11-15 17:42 UTC (permalink / raw) To: linux-tip-commits Cc: mingo, mingo, tony.luck, fenghua.yu, sai.praneeth.prakhya, ravi.v.shankar, h.peter.anvin, linux-kernel, tglx, vikas.shivappa, hpa Commit-ID: f57b308728902d9ffade53466e9201e999a870e4 Gitweb: http://git.kernel.org/tip/f57b308728902d9ffade53466e9201e999a870e4 Author: Fenghua Yu <fenghua.yu@intel.com> AuthorDate: Fri, 11 Nov 2016 17:02:36 -0800 Committer: Thomas Gleixner <tglx@linutronix.de> CommitDate: Tue, 15 Nov 2016 18:35:49 +0100 x86/intel_rdt: Protect info directory from removal The info directory and the per-resource subdirectories of the info directory have no reference to a struct rdtgroup in kn->priv. An attempt to remove one of those directories results in a NULL pointer dereference. Protect the directories from removal and return -EPERM instead of -ENOENT. [ tglx: Massaged changelog ] Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> Cc: "Ravi V Shankar" <ravi.v.shankar@intel.com> Cc: "Tony Luck" <tony.luck@intel.com> Cc: "Sai Prakhya" <sai.praneeth.prakhya@intel.com> Cc: "Vikas Shivappa" <vikas.shivappa@linux.intel.com> Cc: "Ingo Molnar" <mingo@elte.hu> Cc: "H. Peter Anvin" <h.peter.anvin@intel.com> Link: http://lkml.kernel.org/r/1478912558-55514-1-git-send-email-fenghua.yu@intel.com Signed-off-by: Thomas Gleixner <tglx@linutronix.de> --- arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c index 4795880..cff286e 100644 --- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c @@ -644,16 +644,29 @@ static int parse_rdtgroupfs_options(char *data) */ static struct rdtgroup *kernfs_to_rdtgroup(struct kernfs_node *kn) { - if (kernfs_type(kn) == KERNFS_DIR) - return kn->priv; - else + if (kernfs_type(kn) == KERNFS_DIR) { + /* + * All the resource directories use "kn->priv" + * to point to the "struct rdtgroup" for the + * resource. "info" and its subdirectories don't + * have rdtgroup structures, so return NULL here. + */ + if (kn == kn_info || kn->parent == kn_info) + return NULL; + else + return kn->priv; + } else { return kn->parent->priv; + } } struct rdtgroup *rdtgroup_kn_lock_live(struct kernfs_node *kn) { struct rdtgroup *rdtgrp = kernfs_to_rdtgroup(kn); + if (!rdtgrp) + return NULL; + atomic_inc(&rdtgrp->waitcount); kernfs_break_active_protection(kn); @@ -670,6 +683,9 @@ void rdtgroup_kn_unlock(struct kernfs_node *kn) { struct rdtgroup *rdtgrp = kernfs_to_rdtgroup(kn); + if (!rdtgrp) + return; + mutex_unlock(&rdtgroup_mutex); if (atomic_dec_and_test(&rdtgrp->waitcount) && @@ -918,7 +934,7 @@ static int rdtgroup_rmdir(struct kernfs_node *kn) rdtgrp = rdtgroup_kn_lock_live(kn); if (!rdtgrp) { rdtgroup_kn_unlock(kn); - return -ENOENT; + return -EPERM; } /* Give any tasks back to the default group */ ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-11-15 17:46 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-12 1:02 [PATCH 1/3] x86/intel_rdt: Fix kernfs_to_rdtgroup to know about "info/*" directories Fenghua Yu 2016-11-12 1:02 ` [PATCH 2/3] x86/intel_rdt: Return state to default on umount Fenghua Yu 2016-11-15 13:35 ` Thomas Gleixner 2016-11-15 17:28 ` Thomas Gleixner 2016-11-15 17:44 ` [tip:x86/cache] x86/intel_rdt: Reset per cpu closids on unmount tip-bot for Fenghua Yu 2016-11-12 1:02 ` [PATCH 3/3] x86/intel_rdt: Update closid in PQR_ASSOC registers in synchronous mode when changing "cpus" Fenghua Yu 2016-11-15 13:43 ` Thomas Gleixner 2016-11-15 14:00 ` Thomas Gleixner 2016-11-15 16:40 ` Thomas Gleixner 2016-11-15 17:44 ` [tip:x86/cache] x86/intel_rdt: Update percpu closid immeditately on CPUs affected by changee tip-bot for Fenghua Yu 2016-11-15 17:42 ` [tip:x86/cache] x86/intel_rdt: Protect info directory from removal tip-bot for Fenghua Yu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).