lkml.org 
[lkml]   [2023]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3 1/7] x86/resctrl: Add multiple tasks to the resctrl group at once
    Hi Babu,

    On 3/2/2023 12:24 PM, 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. Also,
    > there is a syscall overhead for each command executed from user space.

    To support this change it may also be helpful to add that
    moving tasks take the mutex so attempting to move tasks in
    parallel will not achieve a significant performance gain.

    >
    > It can be improved 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@amd.com>
    > ---
    > Documentation/x86/resctrl.rst | 11 +++++++++--
    > arch/x86/kernel/cpu/resctrl/rdtgroup.c | 24 +++++++++++++++++++++++-
    > 2 files changed, 32 insertions(+), 3 deletions(-)
    >
    > diff --git a/Documentation/x86/resctrl.rst b/Documentation/x86/resctrl.rst
    > index 058257dc56c8..25203f20002d 100644
    > --- a/Documentation/x86/resctrl.rst
    > +++ b/Documentation/x86/resctrl.rst
    > @@ -292,13 +292,20 @@ 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

    How about "tasks separated" -> "task ids separated" or "by inputting the tasks
    separated by commas" -> "by separating the task ids with commas"

    > + sequentially in the order it is provided. Failure while assigning
    > + the tasks will be aborted immediately. The tasks before the failure
    > + will be assigned and the tasks next in the sequence will not be
    > + assigned. Users may need to retry them again. The failure details
    > + will be logged in resctrl/info/last_cmd_status file.

    Please use full path as is done in rest of doc.

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

    Why is this line removal needed?

    > "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..15ea5b550fe9 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;

    The resctrl files should be seen as user space API. With the above change
    you take an interface that did not require a newline and dictate that it
    should have a trailing newline. How convinced are you that this does not
    break any current user space scripts or applications? Why does this
    feature require a trailing newline?

    > +
    > + 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, ","));
    > +

    Could lib/cmdline.c:get_option() be useful?

    > + if (kstrtoint(pid_str, 0, &pid) || pid < 0) {
    > + rdt_last_cmd_printf("Invalid pid %d value\n", pid);

    This is risky. What will pid be if kstrtoint() failed?

    > + 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)
    > + goto unlock;
    > + else
    > + goto next;
    >

    The documentation states "The failure details will be logged
    in resctrl/info/last_cmd_status file." but I do not see how
    this is happening here. From what I can tell this implementation
    does not do anything beyond what last_cmd_status already does
    so any special mention in the docs is not clear to me. The
    cover letter stated "Added pid in last_cmd_status when
    applicable." - it sounded as though last_cmd_status would
    contain the error with the pid that encountered the error but
    I do not see this happening here.

    > unlock:
    > rdtgroup_kn_unlock(of->kn);
    >
    >

    Reinette

    \
     
     \ /
      Last update: 2023-03-27 01:01    [W:4.345 / U:0.424 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site