Re: [PATCH v4 1/7] x86/resctrl: Add multiple tasks to the resctrl group at once

[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@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)

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