| 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
| ||
|
Message-ID: <e719e506-77c6-a5ae-6c18-98b02b31983a@amd.com>
Date: Tue, 9 May 2023 12:13:39 -0500
From: "Moger, Babu" <babu.moger@....com>
To: Reinette Chatre <reinette.chatre@...el.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 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@...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)
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
Powered by blists - more mailing lists