Re: [PATCH 1/3] x86/resctrl: Move setting task's active CPU in a mask into helpers

[Reinette Chatre]

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@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Signed-off-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> > Reviewed-by: Tony Luck <tony.luck@xxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
>
> 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