linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Reinette Chatre <reinette.chatre@intel.com>
To: Borislav Petkov <bp@alien8.de>
Cc: tglx@linutronix.de, fenghua.yu@intel.com, tony.luck@intel.com,
	kuo-lang.tseng@intel.com, shakeelb@google.com,
	valentin.schneider@arm.com, mingo@redhat.com, babu.moger@amd.com,
	james.morse@arm.com, hpa@zytor.com, x86@kernel.org,
	linux-kernel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH 1/3] x86/resctrl: Move setting task's active CPU in a mask into helpers
Date: Mon, 7 Dec 2020 13:24:51 -0800	[thread overview]
Message-ID: <db6bea7e-b60b-2859-aa35-c3d2d5356eaa@intel.com> (raw)
In-Reply-To: <20201207182912.GF20489@zn.tnic>

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

  reply	other threads:[~2020-12-07 21:25 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-03 23:25 [PATCH 0/3] x86/resctrl: Fix a few issues in moving a task to a resource group Reinette Chatre
2020-12-03 23:25 ` [PATCH 1/3] x86/resctrl: Move setting task's active CPU in a mask into helpers Reinette Chatre
2020-12-07 18:29   ` Borislav Petkov
2020-12-07 21:24     ` Reinette Chatre [this message]
2020-12-08  9:49       ` Borislav Petkov
2020-12-08 16:35         ` Reinette Chatre
2020-12-09 16:47   ` James Morse
2020-12-10  0:21     ` Reinette Chatre
2020-12-03 23:25 ` [PATCH 2/3] x86/resctrl: Update PQR_ASSOC MSR synchronously when moving task to resource group Reinette Chatre
2020-12-09 16:51   ` James Morse
2020-12-10  0:22     ` Reinette Chatre
2020-12-11 20:46   ` Valentin Schneider
2020-12-14 18:41     ` Reinette Chatre
2020-12-16 17:41       ` Valentin Schneider
2020-12-16 18:26         ` Reinette Chatre
2020-12-17 10:39           ` Valentin Schneider
2020-12-03 23:25 ` [PATCH 3/3] x86/resctrl: Don't move a task to the same " Reinette Chatre
2020-12-11 20:46 ` [PATCH 0/3] x86/resctrl: Fix a few issues in moving a task to a " Valentin Schneider
2020-12-14 18:38   ` Reinette Chatre
2020-12-16 17:41     ` Valentin Schneider
2020-12-16 18:26       ` Reinette Chatre
2020-12-17 12:19 ` [PATCH 4/3] x86/intel_rdt: Apply READ_ONCE/WRITE_ONCE to task_struct .rmid & .closid Valentin Schneider

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=db6bea7e-b60b-2859-aa35-c3d2d5356eaa@intel.com \
    --to=reinette.chatre@intel.com \
    --cc=babu.moger@amd.com \
    --cc=bp@alien8.de \
    --cc=fenghua.yu@intel.com \
    --cc=hpa@zytor.com \
    --cc=james.morse@arm.com \
    --cc=kuo-lang.tseng@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=shakeelb@google.com \
    --cc=stable@vger.kernel.org \
    --cc=tglx@linutronix.de \
    --cc=tony.luck@intel.com \
    --cc=valentin.schneider@arm.com \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).