Messages in this thread Patch in this message | | | From | "Moger, Babu" <> | Subject | RE: [PATCH v4 1/7] x86/resctrl: Add multiple tasks to the resctrl group at once | Date | Fri, 5 May 2023 17:09:37 +0000 |
| |
[AMD Official Use Only - General]
Hi Reinette,
> -----Original Message----- > From: Reinette Chatre <reinette.chatre@intel.com> > Sent: Thursday, May 4, 2023 1:58 PM > To: Moger, Babu <Babu.Moger@amd.com>; corbet@lwn.net; > tglx@linutronix.de; mingo@redhat.com; bp@alien8.de > Cc: fenghua.yu@intel.com; dave.hansen@linux.intel.com; x86@kernel.org; > hpa@zytor.com; paulmck@kernel.org; akpm@linux-foundation.org; > quic_neeraju@quicinc.com; rdunlap@infradead.org; > damien.lemoal@opensource.wdc.com; songmuchun@bytedance.com; > peterz@infradead.org; jpoimboe@kernel.org; pbonzini@redhat.com; > chang.seok.bae@intel.com; pawan.kumar.gupta@linux.intel.com; > jmattson@google.com; daniel.sneddon@linux.intel.com; Das1, Sandipan > <Sandipan.Das@amd.com>; tony.luck@intel.com; james.morse@arm.com; > linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org; > bagasdotme@gmail.com; eranian@google.com; christophe.leroy@csgroup.eu; > jarkko@kernel.org; adrian.hunter@intel.com; quic_jiles@quicinc.com; > peternewman@google.com > Subject: Re: [PATCH v4 1/7] x86/resctrl: Add multiple tasks to the resctrl group > at once > > Hi Babu, > > On 4/17/2023 4:34 PM, Babu Moger wrote: > > The resctrl task assignment for MONITOR or CONTROL group needs to be > > done one at a time. For example: > > Why all caps for monitor and control? If the intention is to use the terms for > these groups then it may be easier to use the same terms as in the > documentation, or you could just not use all caps like you do in later patches.
Sure. > > > > > $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. > > > > It can be improved by supporting the multiple task id assignment in > > one command with the tasks separated by commas. For example: > > Please use imperative mood (see Documentation/process/maintainer-tip.rst). > > Something like: > "Improve multiple task id assignment ...."
How about: "Improve the assignment by supporting multiple task id assignment in one command with the tasks separated by commas."
> > > > > $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 | 31 > ++++++++++++++++++++++++++++++- > > 2 files changed, 38 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/x86/resctrl.rst > > b/Documentation/x86/resctrl.rst index 387ccbcb558f..f28ed1443a6a > > 100644 > > --- a/Documentation/x86/resctrl.rst > > +++ b/Documentation/x86/resctrl.rst > > @@ -292,7 +292,14 @@ 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 added by separating the task ids > > + with commas. Tasks will be assigned sequentially in the order it > > I think the "in the order it is entered." can be dropped so that it just reads (note > tense change): "Tasks are assigned sequentially."
Ok. Sure
> > > + is entered. Failures while assigning the tasks will be aborted > > + immediately and tasks next in the sequence will not be assigned. > > Multiple failures are not supported. A single failure encountered while > attempting to assign a single task will cause the operation to abort.
Ok. Sure
> > > + Users may need to retry them again. Failure details possibly with > > I am not sure about this guidance. From what I can tell a failure could be either > that the pid does not exist or that the move is illegal. A retry would not achieve > a different outcome. I think you may thus mean that the tasks that followed a > task that could not be moved, but in that case the "retry" is not clear to me.
Ok. Will drop "retry" sentence.
> > > + pid will be logged in /sys/fs/resctrl/info/last_cmd_status file. > > Would it not always print the failing pid (if error was encountered while
Not always. In this case it does not print the pid, rdt_last_cmd_puts("Can't move task to different control group\n"); return -EINVAL;
> attempting to move a task) ? Maybe just drop that so that it reads "Failure > details will be logged to ..." (adding file seems unnecessary).
Ok
> > > > + > > + 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 > > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c > > b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > > index 6ad33f355861..df5bd13440b0 100644 > > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c > > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > > @@ -696,18 +696,41 @@ 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) > > + if (nbytes == 0) > > return -EINVAL; > > + > > + buf[nbytes - 1] = '\0'; > > + > > This seems like another remnant of the schemata write code that expects that > the buffer ends with a '\n'. Since this code does not have this requirement the > above may have unintended consequences if a tool provides a buffer that does > not end with '\n'. > I think you just want to ensure that the buffer is properly terminated and from > what I understand when looking at kernfs_fop_write_iter() this is already taken > care of.
Sure. Will check. Then I will have to change the check below to if (!buf). > > > rdtgrp = rdtgroup_kn_lock_live(of->kn); > > if (!rdtgrp) { > > rdtgroup_kn_unlock(of->kn); > > return -ENOENT; > > } > > + > > +next: > > + if (!buf || buf[0] == '\0') > > + goto unlock; > > + > > rdt_last_cmd_clear(); > > Why is this buffer cleared on processing of each pid?
Will check.
> > > > > + pid_str = strim(strsep(&buf, ",")); > > + > > + if (kstrtoint(pid_str, 0, &pid)) { > > + rdt_last_cmd_printf("Task list parsing error\n"); > > rdt_last_cmd_puts()?
Sure.
> > > + ret = -EINVAL; > > + goto unlock; > > + } > > + > > + if (pid < 0) { > > + rdt_last_cmd_printf("Invalid pid %d value\n", pid); > > + ret = -EINVAL; > > + goto unlock; > > + } > > + > > if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED || > > rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) { > > ret = -EINVAL; > > The above code has nothing to do with the pid so repeating its execution does > not seem necessary.
Will remove.. Thanks Babu > > > @@ -716,6 +739,12 @@ static ssize_t rdtgroup_tasks_write(struct > kernfs_open_file *of, > > } > > > > ret = rdtgroup_move_task(pid, rdtgrp, of); > > + if (ret) { > > + rdt_last_cmd_printf("Error while processing task %d\n", pid); > > + goto unlock; > > + } else { > > + goto next; > > + } > > > > unlock: > > rdtgroup_kn_unlock(of->kn); > > > > > > Reinette
| |