diff mbox series

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

Message ID 168564609148.527584.17320328540832247657.stgit@bmoger-ubuntu
State New
Headers
Series x86/resctrl: Miscellaneous resctrl features |

Commit Message

Moger, Babu June 1, 2023, 7:01 p.m. UTC 
  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(+)

Comments

Reinette Chatre July 7, 2023, 9:39 p.m. UTC | #1 
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
Moger, Babu July 11, 2023, 11:19 p.m. UTC | #2 
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
diff mbox series

Patch

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)
+ *
+ *     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
+ *                     Files: bit_usage, cbm_mask, min_cbm_bits,
+ *                            num_closids, shareable_bits
+ *
+ *            --> RFTYPE_RES_MB
+ *                --> <MB/SMBA> directory
+ *                     Files: bandwidth_gran, delay_linear, min_bandwidth,
+ *                            num_closids
+ *
+ *     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)