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

[Reinette Chatre <reinette.chatre@intel.com>]

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