[v4,1/7] x86/resctrl: Add multiple tasks to the resctrl group at once

  The resctrl task assignment for MONITOR or CONTROL group needs to be
done one at a time. For example:

  $mount -t resctrl resctrl /sys/fs/resctrl/
  $mkdir /sys/fs/resctrl/clos1
  $echo 123 > /sys/fs/resctrl/clos1/tasks
  $echo 456 > /sys/fs/resctrl/clos1/tasks
  $echo 789 > /sys/fs/resctrl/clos1/tasks

This is not user-friendly when dealing with hundreds of tasks.

It can be improved by supporting the multiple task id assignment in
one command with the tasks separated by commas. For example:

  $echo 123,456,789 > /sys/fs/resctrl/clos1/tasks

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 Documentation/x86/resctrl.rst          |    9 ++++++++-
 arch/x86/kernel/cpu/resctrl/rdtgroup.c |   31 ++++++++++++++++++++++++++++++-
 2 files changed, 38 insertions(+), 2 deletions(-)
  

On Mon, 17 Apr 2023, Babu Moger wrote:

> The resctrl task assignment for MONITOR or CONTROL group needs to be
> done one at a time. For example:
> 
>   $mount -t resctrl resctrl /sys/fs/resctrl/
>   $mkdir /sys/fs/resctrl/clos1
>   $echo 123 > /sys/fs/resctrl/clos1/tasks
>   $echo 456 > /sys/fs/resctrl/clos1/tasks
>   $echo 789 > /sys/fs/resctrl/clos1/tasks
> 
> This is not user-friendly when dealing with hundreds of tasks.
> 
> It can be improved by supporting the multiple task id assignment in
> one command with the tasks separated by commas. For example:
> 
>   $echo 123,456,789 > /sys/fs/resctrl/clos1/tasks
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  Documentation/x86/resctrl.rst          |    9 ++++++++-
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   31 ++++++++++++++++++++++++++++++-
>  2 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/x86/resctrl.rst b/Documentation/x86/resctrl.rst
> index 387ccbcb558f..f28ed1443a6a 100644
> --- a/Documentation/x86/resctrl.rst
> +++ b/Documentation/x86/resctrl.rst
> @@ -292,7 +292,14 @@ All groups contain the following files:
>  "tasks":
>  	Reading this file shows the list of all tasks that belong to
>  	this group. Writing a task id to the file will add a task to the
> -	group. If the group is a CTRL_MON group the task is removed from
> +	group. Multiple tasks can be added by separating the task ids
> +	with commas. Tasks will be assigned sequentially in the order it
> +	is entered.

"Tasks ... it is ..." doesn't sound correct.

> Failures while assigning the tasks will be aborted
> +	immediately and tasks next in the sequence will not be assigned.
> +	Users may need to retry them again. Failure details possibly with
> +	pid will be logged in /sys/fs/resctrl/info/last_cmd_status file.
> +
> +	If the group is a CTRL_MON group the task is removed from
>  	whichever previous CTRL_MON group owned the task and also from
>  	any MON group that owned the task. If the group is a MON group,
>  	then the task must already belong to the CTRL_MON parent of this
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 6ad33f355861..df5bd13440b0 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -696,18 +696,41 @@ static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
>  				    char *buf, size_t nbytes, loff_t off)
>  {
>  	struct rdtgroup *rdtgrp;
> +	char *pid_str;
>  	int ret = 0;
>  	pid_t pid;
>  
> -	if (kstrtoint(strstrip(buf), 0, &pid) || pid < 0)
> +	if (nbytes == 0)
>  		return -EINVAL;
> +
> +	buf[nbytes - 1] = '\0';
> +
>  	rdtgrp = rdtgroup_kn_lock_live(of->kn);
>  	if (!rdtgrp) {
>  		rdtgroup_kn_unlock(of->kn);
>  		return -ENOENT;
>  	}
> +
> +next:
> +	if (!buf || buf[0] == '\0')
> +		goto unlock;
> +
>  	rdt_last_cmd_clear();
>  
> +	pid_str = strim(strsep(&buf, ","));
> +
> +	if (kstrtoint(pid_str, 0, &pid)) {
> +		rdt_last_cmd_printf("Task list parsing error\n");
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	if (pid < 0) {
> +		rdt_last_cmd_printf("Invalid pid %d value\n", pid);
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
>  	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
>  	    rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
>  		ret = -EINVAL;
> @@ -716,6 +739,12 @@ static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
>  	}
>  
>  	ret = rdtgroup_move_task(pid, rdtgrp, of);
> +	if (ret) {
> +		rdt_last_cmd_printf("Error while processing task %d\n", pid);
> +		goto unlock;
> +	} else {
> +		goto next;
> +	}

Why is this not changed into a while () loop??

>  
>  unlock:
>  	rdtgroup_kn_unlock(of->kn);
> 
>
  

Hi Jarvinen,

On 4/19/23 07:58, Ilpo Järvinen wrote:
> On Mon, 17 Apr 2023, Babu Moger wrote:
> 
>> The resctrl task assignment for MONITOR or CONTROL group needs to be
>> done one at a time. For example:
>>
>>   $mount -t resctrl resctrl /sys/fs/resctrl/
>>   $mkdir /sys/fs/resctrl/clos1
>>   $echo 123 > /sys/fs/resctrl/clos1/tasks
>>   $echo 456 > /sys/fs/resctrl/clos1/tasks
>>   $echo 789 > /sys/fs/resctrl/clos1/tasks
>>
>> This is not user-friendly when dealing with hundreds of tasks.
>>
>> It can be improved by supporting the multiple task id assignment in
>> one command with the tasks separated by commas. For example:
>>
>>   $echo 123,456,789 > /sys/fs/resctrl/clos1/tasks
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  Documentation/x86/resctrl.rst          |    9 ++++++++-
>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   31 ++++++++++++++++++++++++++++++-
>>  2 files changed, 38 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/x86/resctrl.rst b/Documentation/x86/resctrl.rst
>> index 387ccbcb558f..f28ed1443a6a 100644
>> --- a/Documentation/x86/resctrl.rst
>> +++ b/Documentation/x86/resctrl.rst
>> @@ -292,7 +292,14 @@ All groups contain the following files:
>>  "tasks":
>>  	Reading this file shows the list of all tasks that belong to
>>  	this group. Writing a task id to the file will add a task to the
>> -	group. If the group is a CTRL_MON group the task is removed from
>> +	group. Multiple tasks can be added by separating the task ids
>> +	with commas. Tasks will be assigned sequentially in the order it
>> +	is entered.
> 
> "Tasks ... it is ..." doesn't sound correct.

How about "Tasks will be assigned sequentially in the order they are entered."

> 
>> Failures while assigning the tasks will be aborted
>> +	immediately and tasks next in the sequence will not be assigned.
>> +	Users may need to retry them again. Failure details possibly with
>> +	pid will be logged in /sys/fs/resctrl/info/last_cmd_status file.
>> +
>> +	If the group is a CTRL_MON group the task is removed from
>>  	whichever previous CTRL_MON group owned the task and also from
>>  	any MON group that owned the task. If the group is a MON group,
>>  	then the task must already belong to the CTRL_MON parent of this
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 6ad33f355861..df5bd13440b0 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -696,18 +696,41 @@ static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
>>  				    char *buf, size_t nbytes, loff_t off)
>>  {
>>  	struct rdtgroup *rdtgrp;
>> +	char *pid_str;
>>  	int ret = 0;
>>  	pid_t pid;
>>  
>> -	if (kstrtoint(strstrip(buf), 0, &pid) || pid < 0)
>> +	if (nbytes == 0)
>>  		return -EINVAL;
>> +
>> +	buf[nbytes - 1] = '\0';
>> +
>>  	rdtgrp = rdtgroup_kn_lock_live(of->kn);
>>  	if (!rdtgrp) {
>>  		rdtgroup_kn_unlock(of->kn);
>>  		return -ENOENT;
>>  	}
>> +
>> +next:
>> +	if (!buf || buf[0] == '\0')
>> +		goto unlock;
>> +
>>  	rdt_last_cmd_clear();
>>  
>> +	pid_str = strim(strsep(&buf, ","));
>> +
>> +	if (kstrtoint(pid_str, 0, &pid)) {
>> +		rdt_last_cmd_printf("Task list parsing error\n");
>> +		ret = -EINVAL;
>> +		goto unlock;
>> +	}
>> +
>> +	if (pid < 0) {
>> +		rdt_last_cmd_printf("Invalid pid %d value\n", pid);
>> +		ret = -EINVAL;
>> +		goto unlock;
>> +	}
>> +
>>  	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
>>  	    rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
>>  		ret = -EINVAL;
>> @@ -716,6 +739,12 @@ static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
>>  	}
>>  
>>  	ret = rdtgroup_move_task(pid, rdtgrp, of);
>> +	if (ret) {
>> +		rdt_last_cmd_printf("Error while processing task %d\n", pid);
>> +		goto unlock;
>> +	} else {
>> +		goto next;
>> +	}
> 
> Why is this not changed into a while () loop??

Yes. I can change that. It might look bit more cleaner.

> 
>>  
>>  unlock:
>>  	rdtgroup_kn_unlock(of->kn);
>>
>>
> 
>
  

On Wed, 19 Apr 2023, Moger, Babu wrote:

> Hi Jarvinen,
> 
> On 4/19/23 07:58, Ilpo Järvinen wrote:
> > On Mon, 17 Apr 2023, Babu Moger wrote:
> > 
> >> The resctrl task assignment for MONITOR or CONTROL group needs to be
> >> done one at a time. For example:
> >>
> >>   $mount -t resctrl resctrl /sys/fs/resctrl/
> >>   $mkdir /sys/fs/resctrl/clos1
> >>   $echo 123 > /sys/fs/resctrl/clos1/tasks
> >>   $echo 456 > /sys/fs/resctrl/clos1/tasks
> >>   $echo 789 > /sys/fs/resctrl/clos1/tasks
> >>
> >> This is not user-friendly when dealing with hundreds of tasks.
> >>
> >> It can be improved by supporting the multiple task id assignment in
> >> one command with the tasks separated by commas. For example:
> >>
> >>   $echo 123,456,789 > /sys/fs/resctrl/clos1/tasks
> >>
> >> Signed-off-by: Babu Moger <babu.moger@amd.com>
> >> ---
> >>  Documentation/x86/resctrl.rst          |    9 ++++++++-
> >>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   31 ++++++++++++++++++++++++++++++-
> >>  2 files changed, 38 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Documentation/x86/resctrl.rst b/Documentation/x86/resctrl.rst
> >> index 387ccbcb558f..f28ed1443a6a 100644
> >> --- a/Documentation/x86/resctrl.rst
> >> +++ b/Documentation/x86/resctrl.rst
> >> @@ -292,7 +292,14 @@ All groups contain the following files:
> >>  "tasks":
> >>  	Reading this file shows the list of all tasks that belong to
> >>  	this group. Writing a task id to the file will add a task to the
> >> -	group. If the group is a CTRL_MON group the task is removed from
> >> +	group. Multiple tasks can be added by separating the task ids
> >> +	with commas. Tasks will be assigned sequentially in the order it
> >> +	is entered.
> > 
> > "Tasks ... it is ..." doesn't sound correct.
> 
> How about "Tasks will be assigned sequentially in the order they are entered."

It sounds better.
  

Hi Babu,

On 4/17/2023 4:34 PM, Babu Moger wrote:
> The resctrl task assignment for MONITOR or CONTROL group needs to be
> done one at a time. For example:

Why all caps for monitor and control? If the intention is to use
the terms for these groups then it may be easier to use the same
terms as in the documentation, or you could just not use all caps
like you do in later patches.

> 
>   $mount -t resctrl resctrl /sys/fs/resctrl/
>   $mkdir /sys/fs/resctrl/clos1
>   $echo 123 > /sys/fs/resctrl/clos1/tasks
>   $echo 456 > /sys/fs/resctrl/clos1/tasks
>   $echo 789 > /sys/fs/resctrl/clos1/tasks
> 
> This is not user-friendly when dealing with hundreds of tasks.
> 
> It can be improved by supporting the multiple task id assignment in
> one command with the tasks separated by commas. For example:

Please use imperative mood (see Documentation/process/maintainer-tip.rst).

Something like:
"Improve multiple task id assignment ...."

> 
>   $echo 123,456,789 > /sys/fs/resctrl/clos1/tasks
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  Documentation/x86/resctrl.rst          |    9 ++++++++-
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   31 ++++++++++++++++++++++++++++++-
>  2 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/x86/resctrl.rst b/Documentation/x86/resctrl.rst
> index 387ccbcb558f..f28ed1443a6a 100644
> --- a/Documentation/x86/resctrl.rst
> +++ b/Documentation/x86/resctrl.rst
> @@ -292,7 +292,14 @@ All groups contain the following files:
>  "tasks":
>  	Reading this file shows the list of all tasks that belong to
>  	this group. Writing a task id to the file will add a task to the
> -	group. If the group is a CTRL_MON group the task is removed from
> +	group. Multiple tasks can be added by separating the task ids
> +	with commas. Tasks will be assigned sequentially in the order it

I think the "in the order it is entered." can be dropped so that it just
reads (note tense change): "Tasks are assigned sequentially."

> +	is entered. Failures while assigning the tasks will be aborted
> +	immediately and tasks next in the sequence will not be assigned.

Multiple failures are not supported. A single failure encountered while
attempting to assign a single task will cause the operation to abort.

> +	Users may need to retry them again. Failure details possibly with

I am not sure about this guidance. From what I can tell a failure could be
either that the pid does not exist or that the move is illegal. A retry
would not achieve a different outcome. I think you may thus mean that
the tasks that followed a task that could not be moved, but in that
case the "retry" is not clear to me.

> +	pid will be logged in /sys/fs/resctrl/info/last_cmd_status file.

Would it not always print the failing pid (if error was encountered while
attempting to move a task) ? Maybe just drop that so that it reads
"Failure details will be logged to ..." (adding file seems unnecessary).


> +
> +	If the group is a CTRL_MON group the task is removed from
>  	whichever previous CTRL_MON group owned the task and also from
>  	any MON group that owned the task. If the group is a MON group,
>  	then the task must already belong to the CTRL_MON parent of this
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 6ad33f355861..df5bd13440b0 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -696,18 +696,41 @@ static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
>  				    char *buf, size_t nbytes, loff_t off)
>  {
>  	struct rdtgroup *rdtgrp;
> +	char *pid_str;
>  	int ret = 0;
>  	pid_t pid;
>  
> -	if (kstrtoint(strstrip(buf), 0, &pid) || pid < 0)
> +	if (nbytes == 0)
>  		return -EINVAL;
> +
> +	buf[nbytes - 1] = '\0';
> +

This seems like another remnant of the schemata write code that
expects that the buffer ends with a '\n'. Since this code does not 
have this requirement the above may have unintended consequences if
a tool provides a buffer that does not end with '\n'.
I think you just want to ensure that the buffer is properly terminated
and from what I understand when looking at kernfs_fop_write_iter() this
is already taken care of.

>  	rdtgrp = rdtgroup_kn_lock_live(of->kn);
>  	if (!rdtgrp) {
>  		rdtgroup_kn_unlock(of->kn);
>  		return -ENOENT;
>  	}
> +
> +next:
> +	if (!buf || buf[0] == '\0')
> +		goto unlock;
> +
>  	rdt_last_cmd_clear();

Why is this buffer cleared on processing of each pid?

>  
> +	pid_str = strim(strsep(&buf, ","));
> +
> +	if (kstrtoint(pid_str, 0, &pid)) {
> +		rdt_last_cmd_printf("Task list parsing error\n");

rdt_last_cmd_puts()?

> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	if (pid < 0) {
> +		rdt_last_cmd_printf("Invalid pid %d value\n", pid);
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
>  	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
>  	    rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
>  		ret = -EINVAL;

The above code has nothing to do with the pid so repeating its
execution does not seem necessary.

> @@ -716,6 +739,12 @@ static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
>  	}
>  
>  	ret = rdtgroup_move_task(pid, rdtgrp, of);
> +	if (ret) {
> +		rdt_last_cmd_printf("Error while processing task %d\n", pid);
> +		goto unlock;
> +	} else {
> +		goto next;
> +	}
>  
>  unlock:
>  	rdtgroup_kn_unlock(of->kn);
> 
> 

Reinette
  

[AMD Official Use Only - General]

Hi Reinette,

> -----Original Message-----
> From: Reinette Chatre <reinette.chatre@intel.com>
> Sent: Thursday, May 4, 2023 1:58 PM
> To: Moger, Babu <Babu.Moger@amd.com>; corbet@lwn.net;
> tglx@linutronix.de; mingo@redhat.com; bp@alien8.de
> Cc: fenghua.yu@intel.com; dave.hansen@linux.intel.com; x86@kernel.org;
> hpa@zytor.com; paulmck@kernel.org; akpm@linux-foundation.org;
> quic_neeraju@quicinc.com; rdunlap@infradead.org;
> damien.lemoal@opensource.wdc.com; songmuchun@bytedance.com;
> peterz@infradead.org; jpoimboe@kernel.org; pbonzini@redhat.com;
> chang.seok.bae@intel.com; pawan.kumar.gupta@linux.intel.com;
> jmattson@google.com; daniel.sneddon@linux.intel.com; Das1, Sandipan
> <Sandipan.Das@amd.com>; tony.luck@intel.com; james.morse@arm.com;
> linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org;
> bagasdotme@gmail.com; eranian@google.com; christophe.leroy@csgroup.eu;
> jarkko@kernel.org; adrian.hunter@intel.com; quic_jiles@quicinc.com;
> peternewman@google.com
> Subject: Re: [PATCH v4 1/7] x86/resctrl: Add multiple tasks to the resctrl group
> at once
> 
> Hi Babu,
> 
> On 4/17/2023 4:34 PM, Babu Moger wrote:
> > The resctrl task assignment for MONITOR or CONTROL group needs to be
> > done one at a time. For example:
> 
> Why all caps for monitor and control? If the intention is to use the terms for
> these groups then it may be easier to use the same terms as in the
> documentation, or you could just not use all caps like you do in later patches.

Sure.
> 
> >
> >   $mount -t resctrl resctrl /sys/fs/resctrl/
> >   $mkdir /sys/fs/resctrl/clos1
> >   $echo 123 > /sys/fs/resctrl/clos1/tasks
> >   $echo 456 > /sys/fs/resctrl/clos1/tasks
> >   $echo 789 > /sys/fs/resctrl/clos1/tasks
> >
> > This is not user-friendly when dealing with hundreds of tasks.
> >
> > It can be improved by supporting the multiple task id assignment in
> > one command with the tasks separated by commas. For example:
> 
> Please use imperative mood (see Documentation/process/maintainer-tip.rst).
> 
> Something like:
> "Improve multiple task id assignment ...."

How about:
"Improve the assignment by supporting multiple task id assignment in
one command with the tasks separated by commas."

> 
> >
> >   $echo 123,456,789 > /sys/fs/resctrl/clos1/tasks
> >
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> >  Documentation/x86/resctrl.rst          |    9 ++++++++-
> >  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   31
> ++++++++++++++++++++++++++++++-
> >  2 files changed, 38 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/x86/resctrl.rst
> > b/Documentation/x86/resctrl.rst index 387ccbcb558f..f28ed1443a6a
> > 100644
> > --- a/Documentation/x86/resctrl.rst
> > +++ b/Documentation/x86/resctrl.rst
> > @@ -292,7 +292,14 @@ All groups contain the following files:
> >  "tasks":
> >  	Reading this file shows the list of all tasks that belong to
> >  	this group. Writing a task id to the file will add a task to the
> > -	group. If the group is a CTRL_MON group the task is removed from
> > +	group. Multiple tasks can be added by separating the task ids
> > +	with commas. Tasks will be assigned sequentially in the order it
> 
> I think the "in the order it is entered." can be dropped so that it just reads (note
> tense change): "Tasks are assigned sequentially."

Ok. Sure

> 
> > +	is entered. Failures while assigning the tasks will be aborted
> > +	immediately and tasks next in the sequence will not be assigned.
> 
> Multiple failures are not supported. A single failure encountered while
> attempting to assign a single task will cause the operation to abort.

Ok. Sure

> 
> > +	Users may need to retry them again. Failure details possibly with
> 
> I am not sure about this guidance. From what I can tell a failure could be either
> that the pid does not exist or that the move is illegal. A retry would not achieve
> a different outcome. I think you may thus mean that the tasks that followed a
> task that could not be moved, but in that case the "retry" is not clear to me.

Ok. Will drop "retry" sentence.

> 
> > +	pid will be logged in /sys/fs/resctrl/info/last_cmd_status file.
> 
> Would it not always print the failing pid (if error was encountered while

Not always. In this case it does not print the pid,
rdt_last_cmd_puts("Can't move task to different control group\n");
                        return -EINVAL;

> attempting to move a task) ? Maybe just drop that so that it reads "Failure
> details will be logged to ..." (adding file seems unnecessary).

Ok

> 
> 
> > +
> > +	If the group is a CTRL_MON group the task is removed from
> >  	whichever previous CTRL_MON group owned the task and also from
> >  	any MON group that owned the task. If the group is a MON group,
> >  	then the task must already belong to the CTRL_MON parent of this
> > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > index 6ad33f355861..df5bd13440b0 100644
> > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > @@ -696,18 +696,41 @@ static ssize_t rdtgroup_tasks_write(struct
> kernfs_open_file *of,
> >  				    char *buf, size_t nbytes, loff_t off)  {
> >  	struct rdtgroup *rdtgrp;
> > +	char *pid_str;
> >  	int ret = 0;
> >  	pid_t pid;
> >
> > -	if (kstrtoint(strstrip(buf), 0, &pid) || pid < 0)
> > +	if (nbytes == 0)
> >  		return -EINVAL;
> > +
> > +	buf[nbytes - 1] = '\0';
> > +
> 
> This seems like another remnant of the schemata write code that expects that
> the buffer ends with a '\n'. Since this code does not have this requirement the
> above may have unintended consequences if a tool provides a buffer that does
> not end with '\n'.
> I think you just want to ensure that the buffer is properly terminated and from
> what I understand when looking at kernfs_fop_write_iter() this is already taken
> care of.

Sure. Will check. Then I will have to change the check below to if (!buf).
> 
> >  	rdtgrp = rdtgroup_kn_lock_live(of->kn);
> >  	if (!rdtgrp) {
> >  		rdtgroup_kn_unlock(of->kn);
> >  		return -ENOENT;
> >  	}
> > +
> > +next:
> > +	if (!buf || buf[0] == '\0')
> > +		goto unlock;
> > +
> >  	rdt_last_cmd_clear();
> 
> Why is this buffer cleared on processing of each pid?

Will check.

> 
> >
> > +	pid_str = strim(strsep(&buf, ","));
> > +
> > +	if (kstrtoint(pid_str, 0, &pid)) {
> > +		rdt_last_cmd_printf("Task list parsing error\n");
> 
> rdt_last_cmd_puts()?

Sure.

> 
> > +		ret = -EINVAL;
> > +		goto unlock;
> > +	}
> > +
> > +	if (pid < 0) {
> > +		rdt_last_cmd_printf("Invalid pid %d value\n", pid);
> > +		ret = -EINVAL;
> > +		goto unlock;
> > +	}
> > +
> >  	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
> >  	    rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
> >  		ret = -EINVAL;
> 
> The above code has nothing to do with the pid so repeating its execution does
> not seem necessary.

Will remove..
Thanks
Babu
> 
> > @@ -716,6 +739,12 @@ static ssize_t rdtgroup_tasks_write(struct
> kernfs_open_file *of,
> >  	}
> >
> >  	ret = rdtgroup_move_task(pid, rdtgrp, of);
> > +	if (ret) {
> > +		rdt_last_cmd_printf("Error while processing task %d\n", pid);
> > +		goto unlock;
> > +	} else {
> > +		goto next;
> > +	}
> >
> >  unlock:
> >  	rdtgroup_kn_unlock(of->kn);
> >
> >
> 
> Reinette
  

Hi Babu,

On 5/5/2023 10:09 AM, Moger, Babu wrote:
> [AMD Official Use Only - General]
> 
> Hi Reinette,
> 
>> -----Original Message-----
>> From: Reinette Chatre <reinette.chatre@intel.com>
>> Sent: Thursday, May 4, 2023 1:58 PM
>> To: Moger, Babu <Babu.Moger@amd.com>; corbet@lwn.net;
>> tglx@linutronix.de; mingo@redhat.com; bp@alien8.de
>> Cc: fenghua.yu@intel.com; dave.hansen@linux.intel.com; x86@kernel.org;
>> hpa@zytor.com; paulmck@kernel.org; akpm@linux-foundation.org;
>> quic_neeraju@quicinc.com; rdunlap@infradead.org;
>> damien.lemoal@opensource.wdc.com; songmuchun@bytedance.com;
>> peterz@infradead.org; jpoimboe@kernel.org; pbonzini@redhat.com;
>> chang.seok.bae@intel.com; pawan.kumar.gupta@linux.intel.com;
>> jmattson@google.com; daniel.sneddon@linux.intel.com; Das1, Sandipan
>> <Sandipan.Das@amd.com>; tony.luck@intel.com; james.morse@arm.com;
>> linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org;
>> bagasdotme@gmail.com; eranian@google.com; christophe.leroy@csgroup.eu;
>> jarkko@kernel.org; adrian.hunter@intel.com; quic_jiles@quicinc.com;
>> peternewman@google.com
>> Subject: Re: [PATCH v4 1/7] x86/resctrl: Add multiple tasks to the resctrl group
>> at once
>>
>> Hi Babu,
>>
>> On 4/17/2023 4:34 PM, Babu Moger wrote:
>>> The resctrl task assignment for MONITOR or CONTROL group needs to be
>>> done one at a time. For example:
>>
>> Why all caps for monitor and control? If the intention is to use the terms for
>> these groups then it may be easier to use the same terms as in the
>> documentation, or you could just not use all caps like you do in later patches.
> 
> Sure.
>>
>>>
>>>   $mount -t resctrl resctrl /sys/fs/resctrl/
>>>   $mkdir /sys/fs/resctrl/clos1
>>>   $echo 123 > /sys/fs/resctrl/clos1/tasks
>>>   $echo 456 > /sys/fs/resctrl/clos1/tasks
>>>   $echo 789 > /sys/fs/resctrl/clos1/tasks
>>>
>>> This is not user-friendly when dealing with hundreds of tasks.
>>>
>>> It can be improved by supporting the multiple task id assignment in
>>> one command with the tasks separated by commas. For example:
>>
>> Please use imperative mood (see Documentation/process/maintainer-tip.rst).
>>
>> Something like:
>> "Improve multiple task id assignment ...."
> 
> How about:
> "Improve the assignment by supporting multiple task id assignment in
> one command with the tasks separated by commas."

The double use of 'assignment' can be confusing. This is also a
changelog where a clear context->problem->solution format can help.
If your changelog is clear regarding the context and problem then it
can end with brief solution description like:

"Support multiple task assignment in one command with tasks ids separated
by commas. For example: " (and also please use a non-x86 term for the group
name in your example)

>>>   $echo 123,456,789 > /sys/fs/resctrl/clos1/tasks
>>>

...

>>> +	pid will be logged in /sys/fs/resctrl/info/last_cmd_status file.
>>
>> Would it not always print the failing pid (if error was encountered while
> 
> Not always. In this case it does not print the pid,
> rdt_last_cmd_puts("Can't move task to different control group\n");
>                         return -EINVAL;
> 

What you quote above adds the relevant text to the last_cmd_status buffer ...
and later (see below) more text is added to the buffer that contains the
pid, no?

...

>>>  	struct rdtgroup *rdtgrp;
>>> +	char *pid_str;
>>>  	int ret = 0;
>>>  	pid_t pid;
>>>
>>> -	if (kstrtoint(strstrip(buf), 0, &pid) || pid < 0)
>>> +	if (nbytes == 0)
>>>  		return -EINVAL;
>>> +
>>> +	buf[nbytes - 1] = '\0';
>>> +
>>
>> This seems like another remnant of the schemata write code that expects that
>> the buffer ends with a '\n'. Since this code does not have this requirement the
>> above may have unintended consequences if a tool provides a buffer that does
>> not end with '\n'.
>> I think you just want to ensure that the buffer is properly terminated and from
>> what I understand when looking at kernfs_fop_write_iter() this is already taken
>> care of.
> 
> Sure. Will check. Then I will have to change the check below to if (!buf).

Please check what kernfs_fop_write_iter() does. From what I can tell it does
exactly what you are trying to do above, but without overwriting
part of the string that user space provides.
I thus do not think that the later check needs to change. From what I understand
it is used to handle the scenario if user space provides a string like "pid,"
(last character is the separator). Please do confirm that the code can handle
any variations that user space may throw at it.

>>> @@ -716,6 +739,12 @@ static ssize_t rdtgroup_tasks_write(struct
>> kernfs_open_file *of,
>>>  	}
>>>
>>>  	ret = rdtgroup_move_task(pid, rdtgrp, of);
>>> +	if (ret) {
>>> +		rdt_last_cmd_printf("Error while processing task %d\n", pid);

Note here the pid is added to the buffer that is printed when user space
views last_cmd_status. I think this is the first time two lines of error may
be added to the buffer so you could double check all works as expected.

Reinette
  

Hi Reinette,

On 5/5/23 13:49, Reinette Chatre wrote:
> Hi Babu,
> 
> On 5/5/2023 10:09 AM, Moger, Babu wrote:
>> [AMD Official Use Only - General]
>>
>> Hi Reinette,
>>
>>> -----Original Message-----
>>> From: Reinette Chatre <reinette.chatre@intel.com>
>>> Sent: Thursday, May 4, 2023 1:58 PM
>>> To: Moger, Babu <Babu.Moger@amd.com>; corbet@lwn.net;
>>> tglx@linutronix.de; mingo@redhat.com; bp@alien8.de
>>> Cc: fenghua.yu@intel.com; dave.hansen@linux.intel.com; x86@kernel.org;
>>> hpa@zytor.com; paulmck@kernel.org; akpm@linux-foundation.org;
>>> quic_neeraju@quicinc.com; rdunlap@infradead.org;
>>> damien.lemoal@opensource.wdc.com; songmuchun@bytedance.com;
>>> peterz@infradead.org; jpoimboe@kernel.org; pbonzini@redhat.com;
>>> chang.seok.bae@intel.com; pawan.kumar.gupta@linux.intel.com;
>>> jmattson@google.com; daniel.sneddon@linux.intel.com; Das1, Sandipan
>>> <Sandipan.Das@amd.com>; tony.luck@intel.com; james.morse@arm.com;
>>> linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org;
>>> bagasdotme@gmail.com; eranian@google.com; christophe.leroy@csgroup.eu;
>>> jarkko@kernel.org; adrian.hunter@intel.com; quic_jiles@quicinc.com;
>>> peternewman@google.com
>>> Subject: Re: [PATCH v4 1/7] x86/resctrl: Add multiple tasks to the resctrl group
>>> at once
>>>
>>> Hi Babu,
>>>
>>> On 4/17/2023 4:34 PM, Babu Moger wrote:
>>>> The resctrl task assignment for MONITOR or CONTROL group needs to be
>>>> done one at a time. For example:
>>>
>>> Why all caps for monitor and control? If the intention is to use the terms for
>>> these groups then it may be easier to use the same terms as in the
>>> documentation, or you could just not use all caps like you do in later patches.
>>
>> Sure.
>>>
>>>>
>>>>   $mount -t resctrl resctrl /sys/fs/resctrl/
>>>>   $mkdir /sys/fs/resctrl/clos1
>>>>   $echo 123 > /sys/fs/resctrl/clos1/tasks
>>>>   $echo 456 > /sys/fs/resctrl/clos1/tasks
>>>>   $echo 789 > /sys/fs/resctrl/clos1/tasks
>>>>
>>>> This is not user-friendly when dealing with hundreds of tasks.
>>>>
>>>> It can be improved by supporting the multiple task id assignment in
>>>> one command with the tasks separated by commas. For example:
>>>
>>> Please use imperative mood (see Documentation/process/maintainer-tip.rst).
>>>
>>> Something like:
>>> "Improve multiple task id assignment ...."
>>
>> How about:
>> "Improve the assignment by supporting multiple task id assignment in
>> one command with the tasks separated by commas."
> 
> The double use of 'assignment' can be confusing. This is also a
> changelog where a clear context->problem->solution format can help.
> If your changelog is clear regarding the context and problem then it
> can end with brief solution description like:
> 
> "Support multiple task assignment in one command with tasks ids separated
> by commas. For example: " (and also please use a non-x86 term for the group
> name in your example)

Sure.

> 
>>>>   $echo 123,456,789 > /sys/fs/resctrl/clos1/tasks
>>>>
> 
> ...
> 
>>>> +	pid will be logged in /sys/fs/resctrl/info/last_cmd_status file.
>>>
>>> Would it not always print the failing pid (if error was encountered while
>>
>> Not always. In this case it does not print the pid,
>> rdt_last_cmd_puts("Can't move task to different control group\n");
>>                         return -EINVAL;
>>
> 
> What you quote above adds the relevant text to the last_cmd_status buffer ...
> and later (see below) more text is added to the buffer that contains the
> pid, no?

Yes. That is correct.

> 
> ...
> 
>>>>  	struct rdtgroup *rdtgrp;
>>>> +	char *pid_str;
>>>>  	int ret = 0;
>>>>  	pid_t pid;
>>>>
>>>> -	if (kstrtoint(strstrip(buf), 0, &pid) || pid < 0)
>>>> +	if (nbytes == 0)
>>>>  		return -EINVAL;
>>>> +
>>>> +	buf[nbytes - 1] = '\0';
>>>> +
>>>
>>> This seems like another remnant of the schemata write code that expects that
>>> the buffer ends with a '\n'. Since this code does not have this requirement the
>>> above may have unintended consequences if a tool provides a buffer that does
>>> not end with '\n'.
>>> I think you just want to ensure that the buffer is properly terminated and from
>>> what I understand when looking at kernfs_fop_write_iter() this is already taken
>>> care of.
>>
>> Sure. Will check. Then I will have to change the check below to if (!buf).
> 
> Please check what kernfs_fop_write_iter() does. From what I can tell it does
> exactly what you are trying to do above, but without overwriting
> part of the string that user space provides.
> I thus do not think that the later check needs to change. From what I understand
> it is used to handle the scenario if user space provides a string like "pid,"
> (last character is the separator). Please do confirm that the code can handle
> any variations that user space may throw at it.

Sure. Thanks
Babu
> 
>>>> @@ -716,6 +739,12 @@ static ssize_t rdtgroup_tasks_write(struct
>>> kernfs_open_file *of,
>>>>  	}
>>>>
>>>>  	ret = rdtgroup_move_task(pid, rdtgrp, of);
>>>> +	if (ret) {
>>>> +		rdt_last_cmd_printf("Error while processing task %d\n", pid);
> 
> Note here the pid is added to the buffer that is printed when user space
> views last_cmd_status. I think this is the first time two lines of error may
> be added to the buffer so you could double check all works as expected.
> 
> Reinette