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