[AMD Official Use Only - General]
Hi Reinette,
> -----Original Message-----
> From: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> Sent: Wednesday, March 15, 2023 1:33 PM
> To: Moger, Babu <Babu.Moger@xxxxxxx>; corbet@xxxxxxx;
> tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; bp@xxxxxxxxx
> Cc: fenghua.yu@xxxxxxxxx; dave.hansen@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx;
> hpa@xxxxxxxxx; paulmck@xxxxxxxxxx; akpm@xxxxxxxxxxxxxxxxxxxx;
> quic_neeraju@xxxxxxxxxxx; rdunlap@xxxxxxxxxxxxx;
> damien.lemoal@xxxxxxxxxxxxxxxxxx; songmuchun@xxxxxxxxxxxxx;
> peterz@xxxxxxxxxxxxx; jpoimboe@xxxxxxxxxx; pbonzini@xxxxxxxxxx;
> chang.seok.bae@xxxxxxxxx; pawan.kumar.gupta@xxxxxxxxxxxxxxx;
> jmattson@xxxxxxxxxx; daniel.sneddon@xxxxxxxxxxxxxxx; Das1, Sandipan
> <Sandipan.Das@xxxxxxx>; tony.luck@xxxxxxxxx; james.morse@xxxxxxx;
> linux-doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> bagasdotme@xxxxxxxxx; eranian@xxxxxxxxxx; christophe.leroy@xxxxxxxxxx;
> jarkko@xxxxxxxxxx; adrian.hunter@xxxxxxxxx; quic_jiles@xxxxxxxxxxx;
> peternewman@xxxxxxxxxx
> Subject: Re: [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.
Agree. It may not be significant performance gain. Will remove this line.
>
> >
> > 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@xxxxxxx>
> > ---
> > 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"
Will change it to " Multiple tasks can be assigned together in one command 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.
Ok. 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.
> >
> > -
>
> Why is this line removal needed?
Not 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?
I have tested these changes with intel_cmt_cat tool. It didn’t have any problems.
We are already doing newline check for few other inputs.
>
> > +
> > + 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?
Yes. We could that also. May not be required for the simple case like this.
>
> > + 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?
Yea. I need to separate these failure cases. One is parsing error, and another is invalid pid.
>
> > + 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.
You are right we are not doing anything special here. pid failures error was already there.
I will have to change the text here.
Thanks
Babu
>
> > unlock:
> > rdtgroup_kn_unlock(of->kn);
> >
> >
>
> Reinette
