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

[Yu, Fenghua]

Hi, Babu,

> Right now, the resctrl task assignment for the MONITOR or CONTROL group
> needs to be 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.
> 
> Improve the user experience by supporting the multiple task assignment in one
> command with tasks separated by commas. For example:
>   $echo 123,456,789 > /sys/fs/resctrl/clos1/tasks
> 
> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
> ---
>  Documentation/x86/resctrl.rst          |   13 ++++++------
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   35 ++++++++++++++++++++++++++--
> ----
>  2 files changed, 35 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/x86/resctrl.rst b/Documentation/x86/resctrl.rst
> index 71a531061e4e..f26e16412bcb 100644
> --- a/Documentation/x86/resctrl.rst
> +++ b/Documentation/x86/resctrl.rst
> @@ -208,12 +208,13 @@ 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
> -	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
> -	group. The task is removed from any previous MON group.
> -
> +	group. Multiple tasks can be assigned at once with each task
> +	separated by commas. 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 group. The task is removed from any previous MON
> +	group.

Multiple tasks movement may fail in the middle. How to handle the failure
in the middle? Abort on all previous success movements?

Seems simple way is to exit from the failed task movement. That means
all previous successful movements will not be reversed and all tasks won't
be moved since the failure.

Then this info needs to be explained in the doc.
> 
>  "cpus":
>  	Reading this file shows a bitmask of the logical CPUs owned by diff --git
> a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index e5a48f05e787..344607853f4c 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -686,28 +686,49 @@ 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)
> +	/* Valid input requires a trailing newline */
> +	if (nbytes == 0 || buf[nbytes - 1] != '\n')
>  		return -EINVAL;
> +
> +	buf[nbytes - 1] = '\0';
> +
> +	cpus_read_lock();
>  	rdtgrp = rdtgroup_kn_lock_live(of->kn);
>  	if (!rdtgrp) {
> -		rdtgroup_kn_unlock(of->kn);
> -		return -ENOENT;
> +		ret = -ENOENT;
> +		goto exit;
> +	}
> +
> +next:
> +	if (!buf || buf[0] == '\0')
> +		goto exit;
> +
> +	pid_str = strim(strsep(&buf, ","));
> +
> +	if (kstrtoint(pid_str, 0, &pid) || pid < 0) {
> +		ret = -EINVAL;

rdt_last_cmd_puts() to record the error.

> +		goto exit;
>  	}
> +
>  	rdt_last_cmd_clear();
> 
>  	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
> -	    rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
> -		ret = -EINVAL;
> +			rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
>  		rdt_last_cmd_puts("Pseudo-locking in progress\n");
> -		goto unlock;
> +		ret = -EINVAL;
> +		goto exit;
>  	}
> 
>  	ret = rdtgroup_move_task(pid, rdtgrp, of);

Do you want to exit if there is error in rdtgroup_move_task()?
Otherwise, the failure won't be captured if later take movement succeeds.

> 
> -unlock:
> +	goto next;
> +
> +exit:
> +	cpus_read_unlock();
>  	rdtgroup_kn_unlock(of->kn);
> 
>  	return ret ?: nbytes;
> 

Thanks.

-Fenghua