Re: [PATCH v7 4/8] x86/resctrl: Add comments on RFTYPE flags hierarchy

["Moger, Babu" <babu.moger@amd.com>]

Hi Reinette,

On 8/16/23 13:36, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/16/2023 8:34 AM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 8/15/23 17:45, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 8/11/2023 1:09 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 |   49 ++++++++++++++++++++++++++++++++
>>>>  1 file changed, 49 insertions(+)
>>>>
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> index 2051179a3b91..37800724e002 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> @@ -240,6 +240,55 @@ 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 (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.)
>>>> + *
>>>> + *	info directory structure
>>>> + *	------------------------------------------------------------------
>>>> + *	--> RFTYPE_INFO
>>>> + *	--> <info> directory
>>>> + *		--> RFTYPE_TOP_INFO
>>>> + *		    Files: last_cmd_status
>>>> + *
>>>> + *		--> RFTYPE_MON_INFO
>>>> + *		--> <L3_MON> directory
>>>> + *		    Files: max_threshold_occupancy, mon_features,
>>>> + *			   num_rmids, mbm_total_bytes_config,
>>>> + *			   mbm_local_bytes_config
>>>> + *
>>>
>>> I think the monitor files need the same split as what you did for
>>> control files in this version. That is, there are some files
>>> that depend on RFTYPE_MON_INFO and others that depend on
>>> RFTYPE_MON_INFO | RFTYPE_RES_CACHE. In above it appears that
>>> all files depend on RFTYPE_MON_INFO only.
>>
>> ok. Sure.
>>
>>
>>>> + *		--> RFTYPE_CTRL_INFO
>>>> + *		    Files: num_closids
>>>> + *
>>>
>>> Looking at this closer I can see some potential confusion about where these
>>> files appear. A note saying that these files appear in all sub-directories
>>> may be helpful because at this point it appears that this file is at the
>>> same level as the directories.
>>
>> Sure..
>>
>> With both these changes. Here is the diff on top of current patch.
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 37800724e002..412a9ef98171 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -264,12 +264,16 @@ struct rdtgroup {
>>   *
>>   *             --> RFTYPE_MON_INFO
>>   *             --> <L3_MON> directory
> 
> I understand that resctrl does not use flags for directories
> but I do find it inconsistent how the L3_MON directory is
> layered when compared to how the, for example, L3 directory
> is layered below. I actually start to wonder if it may not
> simplify interpretation if the names of directories
> are removed entirely from this documentation

Yes. I agree. But I was thinking of adding notes about directory may be
helpful. See the patch below.

> 
> I also think that the current hierarchy is confusing since it
> uses combined flags while also creating a hierarchy.
> What I mean is, the documentation looks like;
> 
>       *	--> RFTYPE_INFO
>       *	--> <info> directory
>       *		--> RFTYPE_TOP_INFO
>       *		    Files: last_cmd_status
> 
> If I understand correctly the hierarchy is intended to mean
> that all items below flag at that level has that flag set.
> The confusing part is when combined flags are also used, like
> above where RFTYPE_TOP_INFO is (RFTYPE_INFO | RFTYPE_TOP)
> If hierarchy is followed, should it not rather be:
> 
>       *	--> RFTYPE_INFO
>       *	--> <info> directory
>       *		--> RFTYPE_TOP
>       *		    Files: last_cmd_status
> 

Yes. Agree. This makes more sense.
> 
> 
>> - *                 Files: max_threshold_occupancy, mon_features,
>> - *                        num_rmids, mbm_total_bytes_config,
>> - *                        mbm_local_bytes_config
>> + *                 Files: mon_features, num_rmids
>> + *
>> + *                     --> RFTYPE_RES_CACHE
>> + *                         Files: max_threshold_occupancy,
>> + *                                mbm_total_bytes_config,
>> + *                                mbm_local_bytes_config
>>   *
>>   *             --> RFTYPE_CTRL_INFO
>>   *                 Files: num_closids
>> + *                        These files appear in all the sub-directories.
>>   *
>>   *                     --> RFTYPE_RES_CACHE
>>   *                     --> <L2,L3> directories
> 
> 
> I made an attempt at capturing all the items I mention
> above. Please do not just copy this into your next version but
> consider first if it makes sense to you with the goal of
> reducing ambiguity.
> 
> *	info directory structure
> *	------------------------------------------------------------------
> *	--> RFTYPE_INFO
> *		--> RFTYPE_TOP (Files in top level of info directory)
> *		    Files: last_cmd_status
> *
> *		--> RFTYPE_MON (Files for all monitoring resources)
> *		    Files: mon_features, num_rmids
> *
> *		    --> RFTYPE_RES_CACHE (Files for cache monitoring resources)
> *		    Files: max_threshold_occupancy,
> *			   mbm_total_bytes_config,
> *			   mbm_local_bytes_config
> *
> *		--> RFTYPE_CTRL (Files for all control resources)
> *		    Files: num_closids
> *
> *			--> RFTYPE_RES_CACHE (Files for cache control resources)
> *			    Files: bit_usage, cbm_mask, min_cbm_bits,
> *				   shareable_bits
> *
> *			--> RFTYPE_RES_MB (Files for MBA and SMBA control resources)
> *			    Files: bandwidth_gran, delay_linear,
> *				   min_bandwidth, thread_throttle_mode
> *
> *	base directory structure
> *	------------------------------------------------------------------
> *	--> RFTYPE_BASE (Files for both MON and CTRL groups)
> *	    Files: cpus, cpus_list, tasks
> *
> *		--> RFTYPE_CTRL (Files only for CTRL group)
> *	    	Files: mode, schemata, size
> *

Yea. We can go with it. How about adding a little note about directories?
That might be easy to compare directory structure on a systems with these
flags. Something like this.

+ *
+ * RESCTRL filesystem has two main components
+ *     a. info
+ *     b. base
+ *
+ * /sys/fs/resctrl/
+ *     |
+ *     --> 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.)
+ *
+ *     Note: resctrl does not use flags for directories. Directories are
+ *           created based on the resource type. Added directories below
+ *           for better understanding.
+ *
+ *     info directory structure
+ *     ------------------------------------------------------------------
+ *     --> RFTYPE_INFO
+ *         directory: info
+ *             --> RFTYPE_TOP (Files in top level of info directory)
+ *                 Files: last_cmd_status
+ *
+ *             --> RFTYPE_MON (Files for all monitoring resources)
+ *                 directory: L3_MON
+ *                     Files: mon_features, num_rmids
+ *
+ *                     --> RFTYPE_RES_CACHE (Files for cache monitoring
resources)
+ *                         Files: max_threshold_occupancy,
+ *                                mbm_total_bytes_config,
+ *                                mbm_local_bytes_config
+ *
+ *             --> RFTYPE_CTRL (Files for all control resources)
+ *                 Files: num_closids
+ *
+ *                     --> RFTYPE_RES_CACHE (Files for cache control
resources)
+ *                         directories: L2,L3
+ *                               Files: bit_usage, cbm_mask, min_cbm_bits,
+ *                                      shareable_bits
+ *
+ *                     --> RFTYPE_RES_MB (Files for memory control resources)
+ *                         directories: MB,SMBA
+ *                               Files: bandwidth_gran, delay_linear,
+ *                                      min_bandwidth, thread_throttle_mode
+ *
+ *     base directory structure
+ *     ------------------------------------------------------------------
+ *     --> RFTYPE_BASE (Files common for both MON and CTRL groups)
+ *         Files: cpus, cpus_list, tasks
+ *
+ *             --> RFTYPE_CTRL (Files only for CTRL group)
+ *                 Files: mode, schemata, size


-- 
Thanks
Babu Moger