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

[Reinette Chatre]

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@xxxxxxx>
>>> ---
>>>  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

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



> - *                 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
*

Reinette