Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <fe2990d3-eb98-332e-4720-0a01b55f80ef@amd.com>
Date:   Tue, 11 Jul 2023 18:19:06 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Reply-To: babu.moger@amd.com
Subject: Re: [PATCH v5 4/8] x86/resctrl: Add comments on RFTYPE flags
 hierarchy
Content-Language: en-US
To:     Reinette Chatre <reinette.chatre@intel.com>, corbet@lwn.net,
        tglx@linutronix.de, mingo@redhat.com, bp@alien8.de
Cc:     fenghua.yu@intel.com, dave.hansen@linux.intel.com, x86@kernel.org,
        hpa@zytor.com, paulmck@kernel.org, akpm@linux-foundation.org,
        quic_neeraju@quicinc.com, rdunlap@infradead.org,
        damien.lemoal@opensource.wdc.com, songmuchun@bytedance.com,
        peterz@infradead.org, jpoimboe@kernel.org, pbonzini@redhat.com,
        chang.seok.bae@intel.com, pawan.kumar.gupta@linux.intel.com,
        jmattson@google.com, daniel.sneddon@linux.intel.com,
        sandipan.das@amd.com, tony.luck@intel.com, james.morse@arm.com,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        bagasdotme@gmail.com, eranian@google.com,
        christophe.leroy@csgroup.eu, jarkko@kernel.org,
        adrian.hunter@intel.com, quic_jiles@quicinc.com,
        peternewman@google.com
References: <168564586603.527584.10518315376465080920.stgit@bmoger-ubuntu>
 <168564609148.527584.17320328540832247657.stgit@bmoger-ubuntu>
 <ca0f3541-5d84-8996-9fc6-b473387d8f35@intel.com>
From:   "Moger, Babu" <babu.moger@amd.com>
In-Reply-To: <ca0f3541-5d84-8996-9fc6-b473387d8f35@intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SW9ZMSs3NkRBRHI0ZHpWSzNpYXMrYTdCWk1ZRW15c3lRK0JsR3JwOUQvQ3ov?=
 =?utf-8?B?YmtRWmEvbEk4eVpXdE1xSVJyRVVOQmVQRVRORWp0K2hWdWN2RndjSUNUaTl4?=
 =?utf-8?B?a2ZYczQwaEgyZEVTY3h6TmtlRm5pY3lBd2hqa0FKZU1COFdpN2gwKzVpMldD?=
 =?utf-8?B?cHgram9YNmNJbnhLTnFGVXR5UUwzS3JmTDNBeEg1dVRFUW1sUXFNVEU4djhJ?=
 =?utf-8?B?NFpzZWJhV3B5Y29kT2s2Z0NWcHV6VGZUbVZndVVxcTJDc3c3eWFiZnVOdlhW?=
 =?utf-8?B?SUg2VVRCMmpncHlUZmtGMndiaGZ5ZHhWazZCMk5aMnNoWHpXV3JpeTJmbmto?=
 =?utf-8?B?SGdSb2o3OUQ0bXFPbU5ZZXowRzJjbjdFeERUYUNndnFYdGdwWXllMGZOZjdq?=
 =?utf-8?B?UVB1dVJtaXNxMHpMUmg1dmNPdUVpMWs4Skszem44VGRvWkRKcmpnQ05aQjBZ?=
 =?utf-8?B?S0tQNlh6Y0FXai9DVnVqd2lNRmFidHl3dFlFaFRyQ1FyZ2ZUK2xLSWVMMnZS?=
 =?utf-8?B?Sk41bmhFbUM2NklqdWNMVkNYY2haeTl4Qk9ubGlsVm5iM0JaUFB5aURMY2Fn?=
 =?utf-8?B?cEQzWkliRDR0L0N1L251cXhucjRIK0xBRlhtVFNXOXg4Mml1bXloeGI5NjFC?=
 =?utf-8?B?TnYzYmtTTHI3bDc2MmNSVXFRTXZiMzFyWnhTQ2hMV0VuWklwV2F1bFpPampP?=
 =?utf-8?B?NG5TZkE1aThjWmpyc1hWS29SLzRGcjVsVFo3Z2l0TFlDMUc2SVBxVTR6RE83?=
 =?utf-8?B?MDg0Qk5ZSTRya0Rza3FqaSs3MWhzR3NpNnc3RE44czRZMjBlVEFOTGVCY3RV?=
 =?utf-8?B?MjlTekdkYmxMbDJJTzlOQ2RlWjZmTFFrK2tkcFhhT1JkcXhhTlNmTkhIWCtr?=
 =?utf-8?B?d0RxTDBSTWVQa3QxYWJIVUdMbU9xYkJ5dnFKaW50V0lnNXcreWF6SUE5M3dE?=
 =?utf-8?B?Y1dqeEcrRDBDbVdoeFIvd0YrV3ZoRS9ja2RTNURyb2pwS1VDZkFua2NTN3lR?=
 =?utf-8?B?QjVtOVZXYnI3MVFlRjlOWGVCbm0zcGc5SnQycXN2L3NOd1lDZitXdUVNY0dl?=
 =?utf-8?B?aTV6VEhxRU0wa3g1TUVTYnJuUHI2MU02OTErTEVUNGUzYzc0bEhxSFRYaktj?=
 =?utf-8?B?Z2tkSXZrYVlsWkxxc2Vvc08xTHBKcE50M1ptZG5QaHRZU1ZDZjZUV2UwOS9K?=
 =?utf-8?B?RVYzWjhOR1dSNGd4SVJ5REZqUDNGVHVuSmNrTWtSRVYwMXhYb2tMS3JCL014?=
 =?utf-8?B?V01IOTY0M2hwY3FzdngwYy93T1hBVDVORlR5YmFEeGJTMzBMSWZ0Q0dveDNa?=
 =?utf-8?B?MklaWFhCSUJIbE9vT2g4RHlha1c4QVBKR1hKYmpGN055UUpSaURmb0FYRzZy?=
 =?utf-8?B?VzJ1R0dydmtjSnRRNC9UOTM0QSttVzRiV1VZQU9rK1QrVkQ2RnlINmxFUUNw?=
 =?utf-8?B?a3ZFVU0wYjhGeS9UVkwydDc2WlhJYTNjN2M3dHd5WDJaOS8zNFd5V2lsRllK?=
 =?utf-8?B?K3NxbEU3TlF4NkowS0kzWFF4OHVsNWYrcnR6WUFBMzliZVhoUVh4bkhwcXNV?=
 =?utf-8?B?dlZTdUVpQUtkSWxyVzZoVVRtZ1FDZ0VpL1phanE2akkvcWd6OVVHcEt5MzY0?=
 =?utf-8?B?SDVzdExHTnlHQS9BenYxZEVuUHgyR2NyRWx2cTF6cHR4UmpMYm85KyszMG5U?=
 =?utf-8?B?czh5ZjVBRkUzTTR2U3J0QkZ6ZUFKdlVBbXptKzczY21BNzVjSU5GZlo0bDFD?=
 =?utf-8?B?WVFjNmE5MlV5ZVVtdzdSMm9raVVzL29Bc3FLa0daKzlRYUFQamVvMXNqWlF5?=
 =?utf-8?B?Lzcwa2E3Q2VoM2JuTFkzWE5FbjNxbUZtQVNLanduYWpqb1AxWFRjTjUzcWdu?=
 =?utf-8?B?cFNzUFRQL2lHVHN4Ni84QWxsdWJKbk4rYnRkWGszVDNrQXIyQ0lBZi9DLzAy?=
 =?utf-8?B?M0RrWVFnQnp1a01zc3pySE1haE1QaW1ibmNQNGh1TlBKdjRzUUdSajZmaTJN?=
 =?utf-8?B?VU1xMXJQSHNQTFVyUGNPcmpmWEp0b1NwK3JtUkM5Q0ZKLzhrdi9GQ2pCMkZ0?=
 =?utf-8?B?R1FJRjhROEhUV3RNWWZZazdTTDdWL3c0REJFK05vaFh0QVVJK1ZxamRReE5t?=
 =?utf-8?Q?SMJs=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b5f97b54-1b0e-4dbf-9bc2-08db82653c26
X-MS-Exchange-CrossTenant-AuthSource: MW3PR12MB4553.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Jul 2023 23:19:11.8836
 (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: LKcrTnYJR6zlaN9QFhUYdBCqqwn6f6pF9674coXVmIwu8Spwf5q/oISaqFyecFTr
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB5056
X-Spam-Status: No, score=-1.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Reinette,

On 7/7/23 16:39, Reinette Chatre wrote:
> Hi Babu,
> 
> On 6/1/2023 12:01 PM, Babu Moger wrote:
>> resctrl uses RFTYPE flags for creating resctrl directory structure.
>>
>> Definitions and directory structures are not documented. Add
>> comments to improve the readability and help future additions.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  arch/x86/kernel/cpu/resctrl/internal.h |   45 ++++++++++++++++++++++++++++++++
>>  1 file changed, 45 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 2051179a3b91..c20cd6acb7a3 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -240,6 +240,51 @@ struct rdtgroup {
>>  
>>  /*
>>   * Define the file type flags for base and info directories.
>> + *
>> + * RESCTRL filesystem has two main components
>> + *   a. info
>> + *   b. base
>> + *
>> + * /sys/fs/resctrl/
>> + *     |
>> + *     --> info (directory and provides details on control
>> + *     |         and monitoring resources)
>> + *     |
>> + *     --> base (Lists the files and information to interact with control
>> + *               or monitor groups. Provides details on default control
>> + *               group when filesystem is created. There is no directory
>> + *               with name base)
>> + *
> 
> In the above I think it may help to split the comment into two parts:
> (a) which directory/directories are referred to, and (b) which files can be
> found in the directory/directories.
> 
> For example,
> 
> --> info (Top level directory named "info". Contains files that provide
>           details on control and monitoring resources.)
> 
> --> base (Root directory associated with default resource group as well as
>           directories created by user for MON and CTRL groups. Contains
>           files to interact with MON and CTRL groups.)
> 
> Please feel free to improve.

Looks good.

> 
>> + *     info structure
>> + *    -------------------------------------------------------------
>> + *    --> RFTYPE_INFO
>> + *        --> <info> directory
>> + *            --> RFTYPE_TOP_INFO
>> + *                Files: last_cmd_status
>> + *
>> + *        --> RFTYPE_MON_INFO
>> + *            --> <L3_MON> directory
>> + *                 Files: max_threshold_occupancy, mbm_local_bytes_config,
>> + *                        mbm_total_bytes_config, mon_features, num_rmids
>> + *
>> + *        --> RFTYPE_CTRL_INFO
>> + *            --> RFTYPE_RES_CACHE
>> + *                --> <L2/L3> directory
> 
> Maybe a nitpick but I wonder if it should be "L2,L3" to not create impression
> that it is either/or?

Sure.
> 
>> + *                     Files: bit_usage, cbm_mask, min_cbm_bits,
>> + *                            num_closids, shareable_bits
>> + *
>> + *            --> RFTYPE_RES_MB
>> + *                --> <MB/SMBA> directory
> 
> Same here ... perhaps "MB,SMBA"

Sure.
> 
>> + *                     Files: bandwidth_gran, delay_linear, min_bandwidth,
>> + *                            num_closids
> 
> Missing thread_throttle_mode

Will add it.
> 
>> + *
>> + *     base structure
>> + *     -----------------------------------------------------------
>> + *     --> RFTYPE_BASE (Files common for both MON and CTRL groups)
>> + *         Files: cpus, cpus_list, tasks
>> + *
>> + *     --> RFTYPE_CTRL_BASE (Files only for CTRL group)
>> + *         Files: mode, schemata, size
>>   */
>>  #define RFTYPE_INFO			BIT(0)
>>  #define RFTYPE_BASE			BIT(1)
>>
>>
> 
> I think this is a helpful addition. Thanks for doing this.

Sure. Welcome.Thanks
Babu Moger