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@xxxxxxxxx> >> Sent: Thursday, May 4, 2023 1:58 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 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) >>> $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? ... >>> 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. >>> @@ -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