linux-kernel - Re: [PATCH v8 1/8] 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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <8ed66ba8-f611-ec92-2e0c-9d4d403894c2@intel.com>
Date:   Thu, 7 Sep 2023 07:51:46 -0700
From:   Fenghua Yu <fenghua.yu@...el.com>
To:     <babu.moger@....com>, <corbet@....net>,
        <reinette.chatre@...el.com>, <tglx@...utronix.de>,
        <mingo@...hat.com>, <bp@...en8.de>
CC:     <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>, <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 v8 1/8] x86/resctrl: Add multiple tasks to the resctrl
 group at once

Hi, Babu,

On 9/7/23 07:44, Moger, Babu wrote:
> Hi Fenghua,
> 
> On 9/6/2023 3:42 PM, Fenghua Yu wrote:
>> Hi, Babu,
>>
>> On 9/6/23 07:56, Moger, Babu wrote:
>>>>> @@ -715,7 +714,27 @@ static ssize_t rdtgroup_tasks_write(struct 
>>>>> kernfs_open_file *of,
>>>>>           goto unlock;
>>>>>       }
>>>>>   -    ret = rdtgroup_move_task(pid, rdtgrp, of);
>>>>> +    while (buf && buf[0] != '\0' && buf[0] != '\n') {
>>>>> +        pid_str = strim(strsep(&buf, ","));
>>>>> +
>>>>> +        if (kstrtoint(pid_str, 0, &pid)) {
>>>>> +            rdt_last_cmd_puts("Task list parsing error\n");
>>>>
>>>> It would be better to show the failed pid string in the failure report:
>>>> +            rdt_last_cmd_puts("Task list parsing error pid %s\n", 
>>>> pid_str);
>>>>
>>>> So user will know which pid string causes the failure?
>>>
>>> It was already discussed. Printing the characters during parsing 
>>> error may not be much useful.
>>
>> Could you please let me know where printing "pid_str" is discussed?
> 
> My bad.  Should have read your comments more carefully.

Thank you for your clarification.

> 
> 
>>
>> My understanding is a similar thing is discussed in v3:
>> https://lore.kernel.org/all/167778866506.1053859.2329229096484796501.stgit@bmoger-ubuntu/ 
>>
>>
>> Then v4 has this code without printing pid_str. In v4, there is a 
>> similar discussion of printing pid, but not pid_str.
>>
>> But I cannot find a discussion of why "pid_str" is not printed.
>>
>> If kstritoint(pid_str, 0, &pid) fails, without printing pid_str, how 
>> can user know which pid string fails? e.g. user tries to move 100 pids 
>> and the 51st pid parsing fails. It's hard for user to know the 51st 
>> pid fails without showing pid_str in the error info and then it's hard 
>> for the user to decide to re-do moving or aborting moving etc.
> 
> That is correct.  Will add following print statement o print the pid_str.
> 
> rdt_last_cmd_printf("Task list parsing error pid %s\n", pid_str);

Great!

Thanks.

-Fenghua