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

["Moger, Babu" <babu.moger@amd.com>]

Hi Fenghua,

On 1/3/23 23:46, Yu, Fenghua wrote:
> 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@amd.com>
>> ---
>>  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.

Yes. That is what even I am thinking. Exit on a failed movement and record
the error. Don't need to reverse the successful movements.

>
> Then this info needs to be explained in the doc.
Sure.
>>  "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.
Sure.
>
>> +		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.

Yes, that makes more sense. Exit on a failed movement and record the error.

Thanks

Babu

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

-- 
Thanks
Babu Moger