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@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.
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
