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

[Moger, Babu]

Hi Fenghua,

On 2/16/2023 4:27 PM, Fenghua Yu wrote:
> Hi, Babu,
>
> On 2/2/23 13:46, Babu Moger wrote:
> > The resctrl task assignment for MONITOR or CONTROL group needs to be
> > done 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.
>
> Maybe add something like "poor performance due to syscall overhead...".
Ok. Sure
> > Improve the user experience by supporting the multiple task assignment
> > in one command with the 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          |    9 +++++++--
> >   arch/x86/kernel/cpu/resctrl/rdtgroup.c |   24 +++++++++++++++++++++++-
> >   2 files changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/x86/resctrl.rst 
> > b/Documentation/x86/resctrl.rst
> > index 058257dc56c8..58b76fc75cb7 100644
> > --- a/Documentation/x86/resctrl.rst
> > +++ b/Documentation/x86/resctrl.rst
> > @@ -292,13 +292,18 @@ 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 assigned together in one command by
> > +    inputting the tasks separated by commas. Tasks will be assigned
> > +    sequentially in the order it is provided. Failure while assigning
> > +    the tasks will be aborted immediately and tasks next in the
> > +    sequence will not be assigned. Users may need to retry them again.
>
> May need to add "tasks before the failure are assigned...".
Sure.
> To retry movement, user needs to know which pid fails. So it's better
> to add "last_command_status shows the failure pid and user can parse
> it to retry assignment starting from the failure pid".
Sure.
> > +
> > +    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.
> >   -
> >   "cpus":
> >       Reading this file shows a bitmask of the logical CPUs owned by
> >       this group. Writing a mask to this file will add and remove
> > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c 
> > b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > index e2c1599d1b37..13b7c5f3a27c 100644
> > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > @@ -683,16 +683,34 @@ 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';
> > +
> >       rdtgrp = rdtgroup_kn_lock_live(of->kn);
> >       if (!rdtgrp) {
> >           rdtgroup_kn_unlock(of->kn);
> >           return -ENOENT;
> >       }
> > +
> > +next:
> > +    if (!buf || buf[0] == '\0')
> > +        goto unlock;
> > +
> > +    pid_str = strim(strsep(&buf, ","));
> > +
> > +    if (kstrtoint(pid_str, 0, &pid) || pid < 0) {
> > +        rdt_last_cmd_puts("Invalid pid value\n");
>
> Better to add pid_str in failure info. Then user knows where the 
> failure pid happens and can re-do the movement starting from the 
> failed pid.

ok.

> > +        ret = -EINVAL;
> > +        goto unlock;
> > +    }
> > +
> >       rdt_last_cmd_clear();
> >         if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
> > @@ -703,6 +721,10 @@ static ssize_t rdtgroup_tasks_write(struct 
> > kernfs_open_file *of,
> >       }
> >         ret = rdtgroup_move_task(pid, rdtgrp, of);
> > +    if (ret)
>
> May need to report "Failed at %d\n", pid;

Ok. Dont want to overwrite the last command status. I may need to update 
it with pid. Will check on this.

thanks

Babu

> > +        goto unlock;
> > +    else
> > +        goto next;
> >     unlock:
> >       rdtgroup_kn_unlock(of->kn);
>
> Thanks.
>
> -Fenghua