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

[Moger, Babu]

[AMD Official Use Only - General]

Hi Reinette,

> -----Original Message-----
> From: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> Sent: Thursday, May 4, 2023 1:58 PM
> To: Moger, Babu <Babu.Moger@xxxxxxx>; corbet@xxxxxxx;
> tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; bp@xxxxxxxxx
> Cc: fenghua.yu@xxxxxxxxx; dave.hansen@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx;
> hpa@xxxxxxxxx; paulmck@xxxxxxxxxx; akpm@xxxxxxxxxxxxxxxxxxxx;
> quic_neeraju@xxxxxxxxxxx; rdunlap@xxxxxxxxxxxxx;
> damien.lemoal@xxxxxxxxxxxxxxxxxx; songmuchun@xxxxxxxxxxxxx;
> peterz@xxxxxxxxxxxxx; jpoimboe@xxxxxxxxxx; pbonzini@xxxxxxxxxx;
> chang.seok.bae@xxxxxxxxx; pawan.kumar.gupta@xxxxxxxxxxxxxxx;
> jmattson@xxxxxxxxxx; daniel.sneddon@xxxxxxxxxxxxxxx; Das1, Sandipan
> <Sandipan.Das@xxxxxxx>; tony.luck@xxxxxxxxx; james.morse@xxxxxxx;
> linux-doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> bagasdotme@xxxxxxxxx; eranian@xxxxxxxxxx; christophe.leroy@xxxxxxxxxx;
> jarkko@xxxxxxxxxx; adrian.hunter@xxxxxxxxx; quic_jiles@xxxxxxxxxxx;
> peternewman@xxxxxxxxxx
> 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@xxxxxxx>
> > ---
> >  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