Messages in this thread | | | Date | Tue, 9 May 2023 12:13:39 -0500 | Subject | Re: [PATCH v4 1/7] x86/resctrl: Add multiple tasks to the resctrl group at once | From | "Moger, Babu" <> |
| |
Hi Reinette,
On 5/5/23 13:49, Reinette Chatre wrote: > Hi Babu, > > On 5/5/2023 10:09 AM, Moger, Babu wrote: >> [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." > > The double use of 'assignment' can be confusing. This is also a > changelog where a clear context->problem->solution format can help. > If your changelog is clear regarding the context and problem then it > can end with brief solution description like: > > "Support multiple task assignment in one command with tasks ids separated > by commas. For example: " (and also please use a non-x86 term for the group > name in your example)
Sure.
> >>>> $echo 123,456,789 > /sys/fs/resctrl/clos1/tasks >>>> > > ... > >>>> + 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; >> > > What you quote above adds the relevant text to the last_cmd_status buffer ... > and later (see below) more text is added to the buffer that contains the > pid, no?
Yes. That is correct.
> > ... > >>>> 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). > > Please check what kernfs_fop_write_iter() does. From what I can tell it does > exactly what you are trying to do above, but without overwriting > part of the string that user space provides. > I thus do not think that the later check needs to change. From what I understand > it is used to handle the scenario if user space provides a string like "pid," > (last character is the separator). Please do confirm that the code can handle > any variations that user space may throw at it.
Sure. 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); > > Note here the pid is added to the buffer that is printed when user space > views last_cmd_status. I think this is the first time two lines of error may > be added to the buffer so you could double check all works as expected. > > Reinette
-- Thanks Babu Moger
| |