| Message ID | 77973e75a10bf7ef9b33c664544667deee9e1a8e.1607036601.git.reinette.chatre@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] x86/resctrl: Move setting task's active CPU in a mask into helpers | expand |
On Thu, Dec 03, 2020 at 03:25:48PM -0800, Reinette Chatre wrote: > From: Fenghua Yu <fenghua.yu@intel.com> > > The code of setting the CPU on which a task is running in a CPU mask is > moved into a couple of helpers. Pls read section "2) Describe your changes" in Documentation/process/submitting-patches.rst for more details. More specifically: "Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behaviour." > The new helper task_on_cpu() will be reused shortly. "reused shortly"? I don't think so. > > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> > Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> > Reviewed-by: Tony Luck <tony.luck@intel.com> > Cc: stable@vger.kernel.org Fixes? I guess the same commit from the other two: Fixes: e02737d5b826 ("x86/intel_rdt: Add tasks files") ? > --- > arch/x86/kernel/cpu/resctrl/rdtgroup.c | 47 +++++++++++++++++++------- > 1 file changed, 34 insertions(+), 13 deletions(-) > > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > index 6f4ca4bea625..68db7d2dec8f 100644 > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > @@ -525,6 +525,38 @@ static void rdtgroup_remove(struct rdtgroup *rdtgrp) > kfree(rdtgrp); > } > > +#ifdef CONFIG_SMP > +/* Get the CPU if the task is on it. */ > +static bool task_on_cpu(struct task_struct *t, int *cpu) > +{ > + /* > + * This is safe on x86 w/o barriers as the ordering of writing to > + * task_cpu() and t->on_cpu is reverse to the reading here. The > + * detection is inaccurate as tasks might move or schedule before > + * the smp function call takes place. In such a case the function > + * call is pointless, but there is no other side effect. > + */ > + if (t->on_cpu) { > + *cpu = task_cpu(t); Why have an I/O parameter when you can make it simply: static int task_on_cpu(struct task_struct *t) { if (t->on_cpu) return task_cpu(t); return -1; } > + > + return true; > + } > + > + return false; > +} > + > +static void set_task_cpumask(struct task_struct *t, struct cpumask *mask) > +{ > + int cpu; > + > + if (mask && task_on_cpu(t, &cpu)) > + cpumask_set_cpu(cpu, mask); And that you can turn into: if (!mask) return; cpu = task_on_cpu(t); if (cpu < 0) return; cpumask_set_cpu(cpu, mask); Readable and simple. Hmm? -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Hi Borislav, Thank you very much for your review. On 12/7/2020 10:29 AM, Borislav Petkov wrote: > On Thu, Dec 03, 2020 at 03:25:48PM -0800, Reinette Chatre wrote: >> From: Fenghua Yu <fenghua.yu@intel.com> >> >> The code of setting the CPU on which a task is running in a CPU mask is >> moved into a couple of helpers. > > Pls read section "2) Describe your changes" in > Documentation/process/submitting-patches.rst for more details. > > More specifically: > > "Describe your changes in imperative mood, e.g. "make xyzzy do frotz" > instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy > to do frotz", as if you are giving orders to the codebase to change > its behaviour." > >> The new helper task_on_cpu() will be reused shortly. > > "reused shortly"? I don't think so. How about: "Move the setting of the CPU on which a task is running in a CPU mask into a couple of helpers. There is no functional change. This is a preparatory change for the fix in the following patch from where the Fixes tag is copied." > >> >> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> >> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> >> Reviewed-by: Tony Luck <tony.luck@intel.com> >> Cc: stable@vger.kernel.org > > Fixes? > > I guess the same commit from the other two: > > Fixes: e02737d5b826 ("x86/intel_rdt: Add tasks files") > > ? Correct. I will add it. The addition to the commit message above aims to explain a Fixes tag to a patch with no functional changes. >> --- >> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 47 +++++++++++++++++++------- >> 1 file changed, 34 insertions(+), 13 deletions(-) >> >> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> index 6f4ca4bea625..68db7d2dec8f 100644 >> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> @@ -525,6 +525,38 @@ static void rdtgroup_remove(struct rdtgroup *rdtgrp) >> kfree(rdtgrp); >> } >> >> +#ifdef CONFIG_SMP >> +/* Get the CPU if the task is on it. */ >> +static bool task_on_cpu(struct task_struct *t, int *cpu) >> +{ >> + /* >> + * This is safe on x86 w/o barriers as the ordering of writing to >> + * task_cpu() and t->on_cpu is reverse to the reading here. The >> + * detection is inaccurate as tasks might move or schedule before >> + * the smp function call takes place. In such a case the function >> + * call is pointless, but there is no other side effect. >> + */ >> + if (t->on_cpu) { >> + *cpu = task_cpu(t); > > Why have an I/O parameter when you can make it simply: > > static int task_on_cpu(struct task_struct *t) > { > if (t->on_cpu) > return task_cpu(t); > > return -1; > } > >> + >> + return true; >> + } >> + >> + return false; >> +} >> + >> +static void set_task_cpumask(struct task_struct *t, struct cpumask *mask) >> +{ >> + int cpu; >> + >> + if (mask && task_on_cpu(t, &cpu)) >> + cpumask_set_cpu(cpu, mask); > > And that you can turn into: > > if (!mask) > return; > > cpu = task_on_cpu(t); > if (cpu < 0) > return; > > cpumask_set_cpu(cpu, mask); > > Readable and simple. > > Hmm? > Will do. Thank you very much. Reinette
On Mon, Dec 07, 2020 at 01:24:51PM -0800, Reinette Chatre wrote: > How about: > "Move the setting of the CPU on which a task is running in a CPU mask into a > couple of helpers. > > There is no functional change. This is a preparatory change for the fix in > the following patch from where the Fixes tag is copied." Almost. Just not call it a "following patch" because once this is applied, the following one might be a different one depending on the ordering a git command has requested. So a "later patch" would be probably better. > Correct. I will add it. The addition to the commit message above aims to > explain a Fixes tag to a patch with no functional changes. Yes but you need to tell the stable people somehow that this one is a prerequisite and that they should pick it up too. Unless you can reorg your code this way that you don't need patch 1... Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Hi Borislav, On 12/8/2020 1:49 AM, Borislav Petkov wrote: > On Mon, Dec 07, 2020 at 01:24:51PM -0800, Reinette Chatre wrote: >> How about: >> "Move the setting of the CPU on which a task is running in a CPU mask into a >> couple of helpers. >> >> There is no functional change. This is a preparatory change for the fix in >> the following patch from where the Fixes tag is copied." > > Almost. Just not call it a "following patch" because once this is > applied, the following one might be a different one depending on the > ordering a git command has requested. So a "later patch" would be > probably better. Indeed, will do. Thank you. > >> Correct. I will add it. The addition to the commit message above aims to >> explain a Fixes tag to a patch with no functional changes. > > Yes but you need to tell the stable people somehow that this one is a > prerequisite and that they should pick it up too. Right. Thanks for guiding here. > > Unless you can reorg your code this way that you don't need patch 1... I think that the current organization, with patch 1 containing the preparatory work without functional changes, makes the fix in patch 2 easier to review. I thus plan to keep the code organization as is while surely following your suggestion on how to support the stable team. Thank you very much Reinette
Hi Reinette, Fenghua, On 03/12/2020 23:25, Reinette Chatre wrote: > From: Fenghua Yu <fenghua.yu@intel.com> > > The code of setting the CPU on which a task is running in a CPU mask is > moved into a couple of helpers. The new helper task_on_cpu() will be > reused shortly. > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > index 6f4ca4bea625..68db7d2dec8f 100644 > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > @@ -525,6 +525,38 @@ static void rdtgroup_remove(struct rdtgroup *rdtgrp) > +#ifdef CONFIG_SMP (using IS_ENABLED(CONFIG_SMP) lets the compiler check all the code in one go, then dead-code-remove the stuff that will never happen... its also easier on the eye!) > +/* Get the CPU if the task is on it. */ > +static bool task_on_cpu(struct task_struct *t, int *cpu) > +{ > + /* > + * This is safe on x86 w/o barriers as the ordering of writing to > + * task_cpu() and t->on_cpu is reverse to the reading here. The > + * detection is inaccurate as tasks might move or schedule before > + * the smp function call takes place. In such a case the function > + * call is pointless, but there is no other side effect. > + */ > + if (t->on_cpu) { kernel/sched/core.c calls out that there can be two tasks on one CPU with this set. (grep astute) I think that means this series will falsely match the old task for a CPU while the scheduler is running, and IPI it unnecessarily. task_curr() is the helper that knows not to do this. > + *cpu = task_cpu(t); > + > + return true; > + } > + > + return false; > +} Thanks, James
Hi James, Thank you very much for your review. On 12/9/2020 8:47 AM, James Morse wrote: > Hi Reinette, Fenghua, > > On 03/12/2020 23:25, Reinette Chatre wrote: >> From: Fenghua Yu <fenghua.yu@intel.com> >> >> The code of setting the CPU on which a task is running in a CPU mask is >> moved into a couple of helpers. The new helper task_on_cpu() will be >> reused shortly. > >> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> index 6f4ca4bea625..68db7d2dec8f 100644 >> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> @@ -525,6 +525,38 @@ static void rdtgroup_remove(struct rdtgroup *rdtgrp) > >> +#ifdef CONFIG_SMP > > (using IS_ENABLED(CONFIG_SMP) lets the compiler check all the code in one go, then > dead-code-remove the stuff that will never happen... its also easier on the eye!) Thank you for catching this. New fix (see below) uses this. >> +/* Get the CPU if the task is on it. */ >> +static bool task_on_cpu(struct task_struct *t, int *cpu) >> +{ >> + /* >> + * This is safe on x86 w/o barriers as the ordering of writing to >> + * task_cpu() and t->on_cpu is reverse to the reading here. The >> + * detection is inaccurate as tasks might move or schedule before >> + * the smp function call takes place. In such a case the function >> + * call is pointless, but there is no other side effect. >> + */ > >> + if (t->on_cpu) { > > kernel/sched/core.c calls out that there can be two tasks on one CPU with this set. > (grep astute) > I think that means this series will falsely match the old task for a CPU while the > scheduler is running, and IPI it unnecessarily. > > task_curr() is the helper that knows not to do this. > Thank you very much for catching this. I did not know this. This exposes an issue with the current implementation of moving tasks as part of directory removal. I now plan to replace this patch with a new fix to address this new issue you exposed: the fix will replace the current usage of t->on_cpu with task_curr(). Since I also follow your suggestion for patch #2 this original patch is no longer needed, which is something Borislav also suggested but I could not see a way to do it at the time. This new fix does seem to fall into the "This could be a problem.." category of issues referred to in stable-kernel-rules.rst so while I plan on adding a Fixes tag I plan to not cc the stable team on this one. I am unsure about the right thing to do here so if you have an opinion I would appreciate it. What do you think of this replacement patch (that will be moved to end of series)? Reinette ----8<------ x86/resctrl: Replace t->on_cpu with task_curr() to prevent unnecessary IPI James reported in [1] that there could be two tasks running on the same CPU with t->on_cpu set. Using t->on_cpu as a test if a task is running on a CPU may thus match the old task for a CPU while the scheduler is running and IPI it unnecessarily. task_curr() is the correct helper to use. While doing so move the #ifdef check of the CONFIG_SMP symbol to be a C conditional used to determine if this helper should be used so ensure the code is always checked for correctness by the compiler. [1] https://lore.kernel.org/lkml/a782d2f3-d2f6-795f-f4b1-9462205fd581@arm.com Fixes: 0efc89be9471 ("x86/intel_rdt: Update task closid immediately on CPU in rmdir and unmount") Reported-by: James Morse <james.morse@arm.com> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> --- arch/x86/kernel/cpu/resctrl/rdtgroup.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c index 5e5a49f38fa1..c64fb37f0aec 100644 --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c @@ -2317,19 +2317,9 @@ static void rdt_move_group_tasks(struct rdtgroup *from, struct rdtgroup *to, t->closid = to->closid; t->rmid = to->mon.rmid; -#ifdef CONFIG_SMP - /* - * This is safe on x86 w/o barriers as the ordering - * of writing to task_cpu() and t->on_cpu is - * reverse to the reading here. The detection is - * inaccurate as tasks might move or schedule - * before the smp function call takes place. In - * such a case the function call is pointless, but - * there is no other side effect. - */ - if (mask && t->on_cpu) + /* If the task is on a CPU, set the CPU in the mask. */ + if (IS_ENABLED(CONFIG_SMP) && mask && task_curr(t)) cpumask_set_cpu(task_cpu(t), mask); -#endif } } read_unlock(&tasklist_lock);
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c index 6f4ca4bea625..68db7d2dec8f 100644 --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c @@ -525,6 +525,38 @@ static void rdtgroup_remove(struct rdtgroup *rdtgrp) kfree(rdtgrp); } +#ifdef CONFIG_SMP +/* Get the CPU if the task is on it. */ +static bool task_on_cpu(struct task_struct *t, int *cpu) +{ + /* + * This is safe on x86 w/o barriers as the ordering of writing to + * task_cpu() and t->on_cpu is reverse to the reading here. The + * detection is inaccurate as tasks might move or schedule before + * the smp function call takes place. In such a case the function + * call is pointless, but there is no other side effect. + */ + if (t->on_cpu) { + *cpu = task_cpu(t); + + return true; + } + + return false; +} + +static void set_task_cpumask(struct task_struct *t, struct cpumask *mask) +{ + int cpu; + + if (mask && task_on_cpu(t, &cpu)) + cpumask_set_cpu(cpu, mask); +} +#else +static inline void +set_task_cpumask(struct task_struct *t, struct cpumask *mask) { } +#endif + struct task_move_callback { struct callback_head work; struct rdtgroup *rdtgrp; @@ -2327,19 +2359,8 @@ static void rdt_move_group_tasks(struct rdtgroup *from, struct rdtgroup *to, t->closid = to->closid; t->rmid = to->mon.rmid; -#ifdef CONFIG_SMP - /* - * This is safe on x86 w/o barriers as the ordering - * of writing to task_cpu() and t->on_cpu is - * reverse to the reading here. The detection is - * inaccurate as tasks might move or schedule - * before the smp function call takes place. In - * such a case the function call is pointless, but - * there is no other side effect. - */ - if (mask && t->on_cpu) - cpumask_set_cpu(task_cpu(t), mask); -#endif + /* If the task is on a CPU, set the CPU in the mask. */ + set_task_cpumask(t, mask); } } read_unlock(&tasklist_lock);