Messages in this thread | | | Date | Fri, 17 Feb 2023 09:05:55 -0600 | Subject | Re: [RFC v2 PATCH 1/7] x86/resctrl: Add multiple tasks to the resctrl group at once | From | "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@amd.com> >> --- >> 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
| |