Message-ID: <090d80c5-47ad-c152-27cc-81019aa5daef@amd.com>
Date:   Fri, 17 Feb 2023 09:05:55 -0600
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Reply-To: babu.moger@amd.com
Subject: Re: [RFC v2 PATCH 1/7] x86/resctrl: Add multiple tasks to the resctrl
 group at once
Content-Language: en-US
To:     Fenghua Yu <fenghua.yu@intel.com>, Babu Moger <babu.moger@amd.com>,
        reinette.chatre@intel.com
Cc:     tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
        dave.hansen@linux.intel.com, x86@kernel.org, hpa@zytor.com,
        corbet@lwn.net, linux-kernel@vger.kernel.org,
        linux-doc@vger.kernel.org, eranian@google.com,
        peternewman@google.com
References: <167537433143.647488.9641864719195184123.stgit@bmoger-ubuntu>
 <167537441417.647488.18261538242584692230.stgit@bmoger-ubuntu>
 <3898cf3c-42e8-1fa0-395f-318bceda313d@intel.com>
From:   "Moger, Babu" <bmoger@amd.com>
In-Reply-To: <3898cf3c-42e8-1fa0-395f-318bceda313d@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?QVZZZk9GVmhhbXFQZmZvaTVVM1RNWTlBYTJMWmVzQWcvTGJPTXY2TzlJSGI1?=
 =?utf-8?B?UEJENVVPM25ZemNsNk9ISURsT1FoTFRYU2JNOW9xZHVRWkRYMmNuT0Ftdnh2?=
 =?utf-8?B?bjVocWxmZzFHbXBzYXltdzJBZVgyMTRjVlNuOEZzWUJCamVJTzNNUWlJSFRt?=
 =?utf-8?B?ZDJZWmVZS0V1b3VOeDZ6UDdLVUJsb0hUeUltRzVOdUYvdGE0YmRTMkQwN0Yx?=
 =?utf-8?B?R0lyOGJGdTdvSEdpVW5rUnFqUTRib3krSFJESHNNcFB1U1IxVmFvMjRmRDZD?=
 =?utf-8?B?S1NYblJxaUhQYVNSSjJFMldKQ1RMbUZwL3V0eTAyRFBNMUxvbWgyVlNEUmxV?=
 =?utf-8?B?S1VYc2ZIOFpnQ1E5MUJvMklWU254Nm5iQldvUHlZRk9FNzlYamE5NWt5QVQz?=
 =?utf-8?B?UGo0eHgrdThxVmZVekQ2b21uNklaQWJCUm50RlpRUTdyWGsyK1VMOW5kVzkx?=
 =?utf-8?B?WXlHQ2ZMdlVXR2o4ZHBDOStmQnlPVDYxbVBGczd5QkRuN2hiOG5aWmxzT0ll?=
 =?utf-8?B?cVc5MlBveXlCb3ByQmlNMlVjcjhhRFpxeUwxV05Pekw5WEQ2d0VGbWU5ZktX?=
 =?utf-8?B?RVROMElHTXlleHVCVmcrNHJ1MktPVzJGZjZpa04raWNiZG9rYUNRejBlZlI2?=
 =?utf-8?B?U2VDR0V6NThlL1g4UmZXMFZ5QkRnZ3lNNW9aeTc5dlFmZm8xTTZ1OTZ5NHNB?=
 =?utf-8?B?UWxrSWc1UFhmU0grRjc3SFQ0K0VVY1NrMkVPOHlsN3JmajBkV005RkxZNURQ?=
 =?utf-8?B?Zm5XMUhsQ1VPTHlYVXlQajlHRENTcWFJM1ViNzhKbVRxcXp1eE9oRzd0dkow?=
 =?utf-8?B?Z2E3bWQwV1BaczhhS2g5RDR1NWRSdXduQWtQcTJuQkxJWTdHY1lkT2VKeXU5?=
 =?utf-8?B?ZFA4TXI2c0dPWlozdEpoc1NGUXQ1QTZUZFFRSm9XMlNIM0VwckcrNU1jZ3hN?=
 =?utf-8?B?c0x1WjV3aDYySktMWDZ3cHZoR1BXY1BwTHgwdDZvbFpqdnRpeGxVbnhySFpW?=
 =?utf-8?B?SnRwT1VyNmxOdFpYMTlUZG5sSUJMYmlJZEU0OTgxYjlQSmJxRm04dW9rMmVZ?=
 =?utf-8?B?WlhuaERseEcrNnhFRC8yMjZwTW5NS1FBTVd5d3RPNURXSFZBTll6eXhrTkRl?=
 =?utf-8?B?NjBoR1pJanR4QUpDS3NrYlQ0NXFkZ2N4Q1RlamFORDJ0UXIrQWVwUldrZ3NF?=
 =?utf-8?B?VFlpVy9hQm9kWDRrMGZ5cVBjUVVCUHBUNVBWZ0R3K3g2SkVYQkhlZlkwRHVO?=
 =?utf-8?B?SStydzJndGFJSU16Z044RWllRllUS2taMzdMd1UyWEN6OFFkNXlUdzZ1Y09o?=
 =?utf-8?B?b2FzR3ZKcHdGVThzUXB3VHcvejJ0VlFQdjR3b1Y1NVQvblJvL3E2RUptbFRD?=
 =?utf-8?B?VkNiN1pTTExiT2pxTFVyeTRkclYwU21CcStwNW51dUFWdG45R2E3WHpOK2lr?=
 =?utf-8?B?OGJtVlZTbjJocFNyUkdDVnhhSkZtL1hudnIxQWRvOEdTaGR4NXQ1STFqSm0r?=
 =?utf-8?B?eXJ4M0t1TXoyclozS3dlOGVOd1BTaHFKQWE1Zjltd3RjczJWYkp0V2dreklT?=
 =?utf-8?B?YVJWd2hudjB4ZHBEUzQvQ0FHQ2ZpR1ljZWRJTmc1OTkrVXdqbTBUYkdBQWRD?=
 =?utf-8?B?MUpHSndqc2JFcmlBb0oyNjIxMGU1Q3RuYkV1TXJsZGE4UmpybjJQWWRldXVB?=
 =?utf-8?B?dW91WWlndWNuTXZFYW5kTWJRWWFvd0tHMjVCeU43TGdkTEtkZXRqbXlybWt1?=
 =?utf-8?B?bG9uZlVVM3g1MmN1eUF2dC95dUlISWc3V0J3Z2FnaDU5TTZCaFQwREVVdFRC?=
 =?utf-8?B?Rk5zS3VUV21pNjE5dmZWTkE1SFpya3NGZ05YVHFpTHhUV0IxNitVVjd6Nnhu?=
 =?utf-8?B?aXVUM3J5WERCeDZNUU1kWkxTMHh1ZkNmempKT1hMNkwvYWY3THMrTS9XZVZX?=
 =?utf-8?B?Q05qbHdJS3pweDFKN2hFdDVIVStXa1p2OG1hUkRQRzU2MXcxSDBUYU1tNkNL?=
 =?utf-8?B?ZlhIdEtyd21LTmlkRXpOTDF4QXEweVYzRFZ3U0NjN1lnak5RWCtDT2Zsd1M3?=
 =?utf-8?B?S2xLQ3FXUXlCOEFzTGEzYXZCTGdiYjFIZGlDeGRvZG5zUjhGR3R1di80K0hz?=
 =?utf-8?Q?esjkQpGaUcczpuNb8an31UZvQ?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: cd57f1e6-f5fd-4a05-25ab-08db10f87a26
X-MS-Exchange-CrossTenant-AuthSource: MW3PR12MB4553.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Feb 2023 15:05:59.2342
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: +2C8O472qvFhwoCt+WIwrriE0kzrZh4mjUEfIP/CEb3H69672TArVoMoxt59gbkj
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR12MB7429
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Fenghua,

On 2/16/2023 4:27 PM, Fenghua Yu wrote:
> Hi, Babu,
>
> On 2/2/23 13:46, Babu Moger wrote:
>> The resctrl task assignment for MONITOR or CONTROL group needs to be
>> done one at a time. For example:
>>
>>    $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.
>
> Maybe add something like "poor performance due to syscall overhead...".
Ok. Sure
>
>>
>> Improve the user experience by supporting the multiple task assignment
>> in one command with the tasks separated by commas. For example:
>>
>>    $echo 123,456,789 > /sys/fs/resctrl/clos1/tasks
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>   Documentation/x86/resctrl.rst          |    9 +++++++--
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c |   24 +++++++++++++++++++++++-
>>   2 files changed, 30 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/x86/resctrl.rst 
>> b/Documentation/x86/resctrl.rst
>> index 058257dc56c8..58b76fc75cb7 100644
>> --- a/Documentation/x86/resctrl.rst
>> +++ b/Documentation/x86/resctrl.rst
>> @@ -292,13 +292,18 @@ All groups contain the following files:
>>   "tasks":
>>       Reading this file shows the list of all tasks that belong to
>>       this group. Writing a task id to the file will add a task to the
>> -    group. If the group is a CTRL_MON group the task is removed from
>> +    group. Multiple tasks can be assigned together in one command by
>> +    inputting the tasks separated by commas. Tasks will be assigned
>> +    sequentially in the order it is provided. Failure while assigning
>> +    the tasks will be aborted immediately and tasks next in the
>> +    sequence will not be assigned. Users may need to retry them again.
>
> May need to add "tasks before the failure are assigned...".
Sure.
>
> To retry movement, user needs to know which pid fails. So it's better
> to add "last_command_status shows the failure pid and user can parse
> it to retry assignment starting from the failure pid".
Sure.
>
>> +
>> +    If the group is a CTRL_MON group the task is removed from
>>       whichever previous CTRL_MON group owned the task and also from
>>       any MON group that owned the task. If the group is a MON group,
>>       then the task must already belong to the CTRL_MON parent of this
>>       group. The task is removed from any previous MON group.
>>   -
>>   "cpus":
>>       Reading this file shows a bitmask of the logical CPUs owned by
>>       this group. Writing a mask to this file will add and remove
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c 
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index e2c1599d1b37..13b7c5f3a27c 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -683,16 +683,34 @@ static ssize_t rdtgroup_tasks_write(struct 
>> kernfs_open_file *of,
>>                       char *buf, size_t nbytes, loff_t off)
>>   {
>>       struct rdtgroup *rdtgrp;
>> +    char *pid_str;
>>       int ret = 0;
>>       pid_t pid;
>>   -    if (kstrtoint(strstrip(buf), 0, &pid) || pid < 0)
>> +    /* Valid input requires a trailing newline */
>> +    if (nbytes == 0 || buf[nbytes - 1] != '\n')
>>           return -EINVAL;
>> +
>> +    buf[nbytes - 1] = '\0';
>> +
>>       rdtgrp = rdtgroup_kn_lock_live(of->kn);
>>       if (!rdtgrp) {
>>           rdtgroup_kn_unlock(of->kn);
>>           return -ENOENT;
>>       }
>> +
>> +next:
>> +    if (!buf || buf[0] == '\0')
>> +        goto unlock;
>> +
>> +    pid_str = strim(strsep(&buf, ","));
>> +
>> +    if (kstrtoint(pid_str, 0, &pid) || pid < 0) {
>> +        rdt_last_cmd_puts("Invalid pid value\n");
>
> Better to add pid_str in failure info. Then user knows where the 
> failure pid happens and can re-do the movement starting from the 
> failed pid.

ok.

>
>> +        ret = -EINVAL;
>> +        goto unlock;
>> +    }
>> +
>>       rdt_last_cmd_clear();
>>         if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
>> @@ -703,6 +721,10 @@ static ssize_t rdtgroup_tasks_write(struct 
>> kernfs_open_file *of,
>>       }
>>         ret = rdtgroup_move_task(pid, rdtgrp, of);
>> +    if (ret)
>
> May need to report "Failed at %d\n", pid;

Ok. Dont want to overwrite the last command status. I may need to update 
it with pid. Will check on this.

thanks

Babu

>
>> +        goto unlock;
>> +    else
>> +        goto next;
>>     unlock:
>>       rdtgroup_kn_unlock(of->kn);
>>
>>
>
> Thanks.
>
> -Fenghua