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

[Reinette Chatre <reinette.chatre@intel.com>]

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.

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

> + *                     Files: bit_usage, cbm_mask, min_cbm_bits,
> + *                            num_closids, shareable_bits
> + *
> + *            --> RFTYPE_RES_MB
> + *                --> <MB/SMBA> directory

Same here ... perhaps "MB,SMBA"

> + *                     Files: bandwidth_gran, delay_linear, min_bandwidth,
> + *                            num_closids

Missing thread_throttle_mode

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

Reinette