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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 5 May 2023 11:49:11 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     "Moger, Babu" <Babu.Moger@....com>,
        "corbet@....net" <corbet@....net>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>
CC:     "fenghua.yu@...el.com" <fenghua.yu@...el.com>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "x86@...nel.org" <x86@...nel.org>, "hpa@...or.com" <hpa@...or.com>,
        "paulmck@...nel.org" <paulmck@...nel.org>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "quic_neeraju@...cinc.com" <quic_neeraju@...cinc.com>,
        "rdunlap@...radead.org" <rdunlap@...radead.org>,
        "damien.lemoal@...nsource.wdc.com" <damien.lemoal@...nsource.wdc.com>,
        "songmuchun@...edance.com" <songmuchun@...edance.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "jpoimboe@...nel.org" <jpoimboe@...nel.org>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "chang.seok.bae@...el.com" <chang.seok.bae@...el.com>,
        "pawan.kumar.gupta@...ux.intel.com" 
        <pawan.kumar.gupta@...ux.intel.com>,
        "jmattson@...gle.com" <jmattson@...gle.com>,
        "daniel.sneddon@...ux.intel.com" <daniel.sneddon@...ux.intel.com>,
        "Das1, Sandipan" <Sandipan.Das@....com>,
        "tony.luck@...el.com" <tony.luck@...el.com>,
        "james.morse@....com" <james.morse@....com>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "bagasdotme@...il.com" <bagasdotme@...il.com>,
        "eranian@...gle.com" <eranian@...gle.com>,
        "christophe.leroy@...roup.eu" <christophe.leroy@...roup.eu>,
        "jarkko@...nel.org" <jarkko@...nel.org>,
        "adrian.hunter@...el.com" <adrian.hunter@...el.com>,
        "quic_jiles@...cinc.com" <quic_jiles@...cinc.com>,
        "peternewman@...gle.com" <peternewman@...gle.com>
Subject: Re: [PATCH v4 1/7] x86/resctrl: Add multiple tasks to the resctrl
 group at once

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@...el.com>
>> Sent: Thursday, May 4, 2023 1:58 PM
>> To: Moger, Babu <Babu.Moger@....com>; corbet@....net;
>> tglx@...utronix.de; mingo@...hat.com; bp@...en8.de
>> Cc: fenghua.yu@...el.com; dave.hansen@...ux.intel.com; x86@...nel.org;
>> hpa@...or.com; paulmck@...nel.org; akpm@...ux-foundation.org;
>> quic_neeraju@...cinc.com; rdunlap@...radead.org;
>> damien.lemoal@...nsource.wdc.com; songmuchun@...edance.com;
>> peterz@...radead.org; jpoimboe@...nel.org; pbonzini@...hat.com;
>> chang.seok.bae@...el.com; pawan.kumar.gupta@...ux.intel.com;
>> jmattson@...gle.com; daniel.sneddon@...ux.intel.com; Das1, Sandipan
>> <Sandipan.Das@....com>; tony.luck@...el.com; james.morse@....com;
>> linux-doc@...r.kernel.org; linux-kernel@...r.kernel.org;
>> bagasdotme@...il.com; eranian@...gle.com; christophe.leroy@...roup.eu;
>> jarkko@...nel.org; adrian.hunter@...el.com; quic_jiles@...cinc.com;
>> peternewman@...gle.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)

>>>   $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