lkml.org 
[lkml]   [2020]   [Dec]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/3] x86/resctrl: Move setting task's active CPU in a mask into helpers
From
Date
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

\
 
 \ /
  Last update: 2020-12-07 22:29    [W:0.170 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site