[v13,04/10] x86/resctrl: Add comments on RFTYPE flags hierarchy

  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>
---
v13: Fixed the format issues in the documentation. Needed few minor format
     changes to address the syntax issues.(Reinette)
     Removed "Reviewed-by and Tested-by" flags as the patch has changed.

v12: Moved the comments from arch/x86/kernel/cpu/resctrl/internal.h
     to Documentation/arch/x86/resctrl.rst. (Boris)
---
 Documentation/arch/x86/resctrl.rst | 64 ++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)
  

Hi Babu,

On 10/13/2023 1:25 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>
> ---
> v13: Fixed the format issues in the documentation. Needed few minor format
>      changes to address the syntax issues.(Reinette)
>      Removed "Reviewed-by and Tested-by" flags as the patch has changed.
> 
> v12: Moved the comments from arch/x86/kernel/cpu/resctrl/internal.h
>      to Documentation/arch/x86/resctrl.rst. (Boris)
> ---
>  Documentation/arch/x86/resctrl.rst | 64 ++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
> index 178ab1d8f747..1163da74f734 100644
> --- a/Documentation/arch/x86/resctrl.rst
> +++ b/Documentation/arch/x86/resctrl.rst
> @@ -369,6 +369,70 @@ When monitoring is enabled all MON groups will also contain:
>  	the sum for all tasks in the CTRL_MON group and all tasks in
>  	MON groups. Please see example section for more details on usage.
>  
> +RESCTRL filesystem implementation notes
> +=======================================
> +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
> +

Could you please do a "make htmldocs" and then ensure that the output looks sane?
The resulting output does not look right to me. For example, the above turns into
a single line that looks like:

/sys/fs/resctrl/ | -> info | | (Top level directory named "info". Contains files that | provide details on control and monitoring resources") | -> base


The spacing also looks off when viewing this in html.

> +	   (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 uses flags for files, not for directories.
> +	      Directories are created based on the resource type. Added
> +	      directories below for better understanding. The RFTYPE flags
> +	      are defined in arch/x86/kernel/cpu/resctrl/internal.h.
> +
> +	info directory structure
> +

You removed the separator in order to pass the syntax issue but now there is
no indication that this is a heading and the content becomes harder to understand.

> +	-> RFTYPE_INFO
> +	   Directory: info
> +
> +	-> RFTYPE_TOP (Files in top level of info directory)
> +	   File: 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)
> +		   Directory: L3_MON
> +		       Files: max_threshold_occupancy,
> +		              mbm_total_bytes_config,
> +		              mbm_local_bytes_config
> +
> +	-> RFTYPE_CTRL (Files for all control resources)
> +	   Directories: L2, L3, MB, SMBA, L2CODE, L2DATA, L3CODE, L3DATA
> +	          File: num_closids
> +
> +		-> RFTYPE_RES_CACHE (Files for cache control resources)
> +		   Directories: L2, L3, L2CODE, L2DATA, L3CODE, L3DATA
> +		         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
> +

Same here.

> +	-> RFTYPE_BASE (Files common for both MON and CTRL groups)
> +	   Files: cpus, cpus_list, tasks
> +
> +		-> RFTYPE_CTRL (Files for CTRL group)
> +		   Files: mode, schemata, size
> +
>  Resource allocation rules
>  -------------------------
>  

Reinette
  

Hi Reinette,

On 10/13/2023 4:23 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 10/13/2023 1:25 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>
>> ---
>> v13: Fixed the format issues in the documentation. Needed few minor format
>>       changes to address the syntax issues.(Reinette)
>>       Removed "Reviewed-by and Tested-by" flags as the patch has changed.
>>
>> v12: Moved the comments from arch/x86/kernel/cpu/resctrl/internal.h
>>       to Documentation/arch/x86/resctrl.rst. (Boris)
>> ---
>>   Documentation/arch/x86/resctrl.rst | 64 ++++++++++++++++++++++++++++++
>>   1 file changed, 64 insertions(+)
>>
>> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
>> index 178ab1d8f747..1163da74f734 100644
>> --- a/Documentation/arch/x86/resctrl.rst
>> +++ b/Documentation/arch/x86/resctrl.rst
>> @@ -369,6 +369,70 @@ When monitoring is enabled all MON groups will also contain:
>>   	the sum for all tasks in the CTRL_MON group and all tasks in
>>   	MON groups. Please see example section for more details on usage.
>>   
>> +RESCTRL filesystem implementation notes
>> +=======================================
>> +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
>> +
> Could you please do a "make htmldocs" and then ensure that the output looks sane?
> The resulting output does not look right to me. For example, the above turns into
> a single line that looks like:
>
> /sys/fs/resctrl/ | -> info | | (Top level directory named "info". Contains files that | provide details on control and monitoring resources") | -> base
>
>
> The spacing also looks off when viewing this in html.


Yes. Messed up again.  Thought fixing the syntax was enough. Will look 
into this.


>
>> +	   (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 uses flags for files, not for directories.
>> +	      Directories are created based on the resource type. Added
>> +	      directories below for better understanding. The RFTYPE flags
>> +	      are defined in arch/x86/kernel/cpu/resctrl/internal.h.
>> +
>> +	info directory structure
>> +
> You removed the separator in order to pass the syntax issue but now there is
> no indication that this is a heading and the content becomes harder to understand.
Yes. I played with this little bit. Dont know how to make this look as 
header. Will check it.
>
>> +	-> RFTYPE_INFO
>> +	   Directory: info
>> +
>> +	-> RFTYPE_TOP (Files in top level of info directory)
>> +	   File: 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)
>> +		   Directory: L3_MON
>> +		       Files: max_threshold_occupancy,
>> +		              mbm_total_bytes_config,
>> +		              mbm_local_bytes_config
>> +
>> +	-> RFTYPE_CTRL (Files for all control resources)
>> +	   Directories: L2, L3, MB, SMBA, L2CODE, L2DATA, L3CODE, L3DATA
>> +	          File: num_closids
>> +
>> +		-> RFTYPE_RES_CACHE (Files for cache control resources)
>> +		   Directories: L2, L3, L2CODE, L2DATA, L3CODE, L3DATA
>> +		         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
>> +
> Same here.

Yes. Will check it.

thanks

Babu
  

Hi Reinette,

On 10/13/2023 4:39 PM, Moger, Babu wrote:
> Hi Reinette,
>
> On 10/13/2023 4:23 PM, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 10/13/2023 1:25 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>
>>> ---
>>> v13: Fixed the format issues in the documentation. Needed few minor 
>>> format
>>>       changes to address the syntax issues.(Reinette)
>>>       Removed "Reviewed-by and Tested-by" flags as the patch has 
>>> changed.
>>>
>>> v12: Moved the comments from arch/x86/kernel/cpu/resctrl/internal.h
>>>       to Documentation/arch/x86/resctrl.rst. (Boris)
>>> ---
>>>   Documentation/arch/x86/resctrl.rst | 64 
>>> ++++++++++++++++++++++++++++++
>>>   1 file changed, 64 insertions(+)
>>>
>>> diff --git a/Documentation/arch/x86/resctrl.rst 
>>> b/Documentation/arch/x86/resctrl.rst
>>> index 178ab1d8f747..1163da74f734 100644
>>> --- a/Documentation/arch/x86/resctrl.rst
>>> +++ b/Documentation/arch/x86/resctrl.rst
>>> @@ -369,6 +369,70 @@ When monitoring is enabled all MON groups will 
>>> also contain:
>>>       the sum for all tasks in the CTRL_MON group and all tasks in
>>>       MON groups. Please see example section for more details on usage.
>>>   +RESCTRL filesystem implementation notes
>>> +=======================================
>>> +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
>>> +
>> Could you please do a "make htmldocs" and then ensure that the output 
>> looks sane?
>> The resulting output does not look right to me. For example, the 
>> above turns into
>> a single line that looks like:
>>
>> /sys/fs/resctrl/ | -> info | | (Top level directory named "info". 
>> Contains files that | provide details on control and monitoring 
>> resources") | -> base
>>
>>
>> The spacing also looks off when viewing this in html.
>
I have fixed the all the format issues. Please let me know if it looks 
ok. I will send the final version after that.

Attached the patch and also added the diff inline.

Thanks

diff --git a/Documentation/arch/x86/resctrl.rst 
b/Documentation/arch/x86/resctrl.rst
index 178ab1d8f747..e990272e9a4f 100644
--- a/Documentation/arch/x86/resctrl.rst
+++ b/Documentation/arch/x86/resctrl.rst
@@ -396,6 +396,67 @@ Resource monitoring rules
  3) Otherwise RDT events for the task will be reported in the root level
     "mon_data" group.

+RESCTRL filesystem implementation notes
+---------------------------------------
+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 uses flags for files, not for directories. 
Directories
+                     are created based on the resource type. Added the 
directories
+                     below for better understanding. The RFTYPE flags 
are defined
+                     in arch/x86/kernel/cpu/resctrl/internal.h.
+
+"info directory structure"::
+
+       --> RFTYPE_INFO
+           Directory: info
+               --> RFTYPE_TOP (Files in top level of info directory)
+                   File: 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)
+                           Directory: L3_MON
+                               Files: max_threshold_occupancy,
+                                      mbm_total_bytes_config,
+                                      mbm_local_bytes_config
+
+               --> RFTYPE_CTRL (Files for all control resources)
+                   Directories: L2, L3, MB, SMBA, L2CODE, L2DATA, 
L3CODE, L3DATA
+                          File: num_closids
+
+                       --> RFTYPE_RES_CACHE (Files for cache control 
resources)
+                           Directories: L2, L3, L2CODE, L2DATA, L3CODE, 
L3DATA
+                                 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 for CTRL group)
+                   Files: mode, schemata, size

  Notes on cache occupancy monitoring and control
  ===============================================
commit 6fc3d73890386c18b69568444411ac9763391752
Author: Babu Moger <babu.moger@amd.com>
Date:   Fri Oct 13 10:45:13 2023 -0500

    x86/resctrl: Add comments on RFTYPE flags hierarchy
    
    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>
    ---
    v13: Fixed the format issues in the documentation. Needed few minor format
         changes to address the syntax issues.(Reinette)
         Removed "Reviewed-by and Tested-by" flags as the patch has changed.
    
    v12: Moved the comments from arch/x86/kernel/cpu/resctrl/internal.h
         to Documentation/arch/x86/resctrl.rst. (Boris)

diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
index 178ab1d8f747..e990272e9a4f 100644
--- a/Documentation/arch/x86/resctrl.rst
+++ b/Documentation/arch/x86/resctrl.rst
@@ -396,6 +396,67 @@ Resource monitoring rules
 3) Otherwise RDT events for the task will be reported in the root level
    "mon_data" group.
 
+RESCTRL filesystem implementation notes
+---------------------------------------
+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 uses flags for files, not for directories. Directories
+		      are created based on the resource type. Added the directories
+		      below for better understanding. The RFTYPE flags are defined
+		      in arch/x86/kernel/cpu/resctrl/internal.h.
+
+"info directory structure"::
+
+	--> RFTYPE_INFO
+	    Directory: info
+		--> RFTYPE_TOP (Files in top level of info directory)
+		    File: 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)
+			    Directory: L3_MON
+			        Files: max_threshold_occupancy,
+			               mbm_total_bytes_config,
+			               mbm_local_bytes_config
+
+		--> RFTYPE_CTRL (Files for all control resources)
+		    Directories: L2, L3, MB, SMBA, L2CODE, L2DATA, L3CODE, L3DATA
+		           File: num_closids
+
+			--> RFTYPE_RES_CACHE (Files for cache control resources)
+			    Directories: L2, L3, L2CODE, L2DATA, L3CODE, L3DATA
+			          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 for CTRL group)
+		    Files: mode, schemata, size
 
 Notes on cache occupancy monitoring and control
 ===============================================
  

Hi Babu,

On 10/14/2023 4:06 PM, Moger, Babu wrote:
> On 10/13/2023 4:39 PM, Moger, Babu wrote:
>> On 10/13/2023 4:23 PM, Reinette Chatre wrote:
>>> On 10/13/2023 1:25 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>
>>>> ---
>>>> v13: Fixed the format issues in the documentation. Needed few minor format
>>>>       changes to address the syntax issues.(Reinette)
>>>>       Removed "Reviewed-by and Tested-by" flags as the patch has changed.
>>>>
>>>> v12: Moved the comments from arch/x86/kernel/cpu/resctrl/internal.h
>>>>       to Documentation/arch/x86/resctrl.rst. (Boris)
>>>> ---
>>>>   Documentation/arch/x86/resctrl.rst | 64 ++++++++++++++++++++++++++++++
>>>>   1 file changed, 64 insertions(+)
>>>>
>>>> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
>>>> index 178ab1d8f747..1163da74f734 100644
>>>> --- a/Documentation/arch/x86/resctrl.rst
>>>> +++ b/Documentation/arch/x86/resctrl.rst
>>>> @@ -369,6 +369,70 @@ When monitoring is enabled all MON groups will also contain:
>>>>       the sum for all tasks in the CTRL_MON group and all tasks in
>>>>       MON groups. Please see example section for more details on usage.
>>>>   +RESCTRL filesystem implementation notes
>>>> +=======================================
>>>> +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
>>>> +
>>> Could you please do a "make htmldocs" and then ensure that the output looks sane?
>>> The resulting output does not look right to me. For example, the above turns into
>>> a single line that looks like:
>>>
>>> /sys/fs/resctrl/ | -> info | | (Top level directory named "info". Contains files that | provide details on control and monitoring resources") | -> base
>>>
>>>
>>> The spacing also looks off when viewing this in html.
>>
> I have fixed the all the format issues. Please let me know if it
> looks ok. I will send the final version after that.
> 
> Attached the patch and also added the diff inline.

This still does not look as though you consider how the document 
looks after the changes.

Consider the organization. Before your changes (assume numbering starts
at 1):

1. User Interface for Resource Control feature
1.1 Info directory
1.2 Resource alloc and monitor groups
1.2.1 Resource allocation rules
1.2.2 Resource monitoring rules
1.3 Notes on cache occupancy monitoring and control
...

After your changes:

1. User Interface for Resource Control feature
1.1 Info directory
1.2 Resource alloc and monitor groups
1.2.1 Resource allocation rules
1.2.2 Resource monitoring rules
1.2.3 RESCTRL filesystem implementation notes
1.3 Notes on cache occupancy monitoring and control
...

Note how the "RESCTRL filesystem implementation notes" is inserted
as a subsection of resource and monitoring groups. Since the text
describes all files in resctrl (not just resource groups) I expect
that it would not be buried as a subsection of resource groups.

This addition also ignores existing customs. Nowhere in the
entire document will you find "RESCTRL" (well, except for the
config option).
 
> Thanks
> 
> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
> index 178ab1d8f747..e990272e9a4f 100644
> --- a/Documentation/arch/x86/resctrl.rst
> +++ b/Documentation/arch/x86/resctrl.rst
> @@ -396,6 +396,67 @@ Resource monitoring rules
>  3) Otherwise RDT events for the task will be reported in the root level
>     "mon_data" group.
> 
> +RESCTRL filesystem implementation notes
> +---------------------------------------
> +RESCTRL filesystem has two main components.

This documentation was extracted as-is from the header file where it
was next to the flags being documented. With this move all that context
is lost so it may be helpful to summarize what is documented here.

> +
> +a. info
> +b. base
> +
> +::

If I understand correctly, instead of adjusting to the target format you
hardcode all the text as pre-formatted? That may be ok. I cannot speak to
whether this matches expectations of the proposal to move the documentation
here.

> +
> +       /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 uses flags for files, not for directories. Directories
> +                     are created based on the resource type. Added the directories
> +                     below for better understanding. The RFTYPE flags are defined
> +                     in arch/x86/kernel/cpu/resctrl/internal.h.
> +
> +"info directory structure"::
> +

This unexpected. In your previous response you noted that you did not know
how to make it look like a header. I expected that you would read the manual
to answer your own question but instead you just placed the title in quotes? I do
not see how placing text in quotes create impression that it is a header.
There is syntax to indicate section headers.

Reinette
  

On Mon, Oct 16, 2023 at 01:46:42PM -0700, Reinette Chatre wrote:
> This still does not look as though you consider how the document 
> looks after the changes.

Looking how this would take longer, Babu, you could leave out the
documentation changes for now and send the rest of the pile which has
been reviewed.

And then you can take your time and do the documentation stuff after
that.

HTH.
  

Hi Boris,

On 10/16/2023 4:10 PM, Borislav Petkov wrote:
> On Mon, Oct 16, 2023 at 01:46:42PM -0700, Reinette Chatre wrote:
>> This still does not look as though you consider how the document
>> looks after the changes.
> Looking how this would take longer, Babu, you could leave out the
> documentation changes for now and send the rest of the pile which has
> been reviewed.
>
> And then you can take your time and do the documentation stuff after
> that.

Yes. Sure. Will send it  ASAP.

Thanks

Babu
  

Hi Reinette,

Boris pulled rest of the patches. Thank you very much for the feedback and
patience.

I can send this as a separate patch with all the FTYPE documentation
update. This need go later.


On 10/16/23 15:46, Reinette Chatre wrote:
> Hi Babu,
> 
> On 10/14/2023 4:06 PM, Moger, Babu wrote:
>> On 10/13/2023 4:39 PM, Moger, Babu wrote:
>>> On 10/13/2023 4:23 PM, Reinette Chatre wrote:
>>>> On 10/13/2023 1:25 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>
>>>>> ---
>>>>> v13: Fixed the format issues in the documentation. Needed few minor format
>>>>>       changes to address the syntax issues.(Reinette)
>>>>>       Removed "Reviewed-by and Tested-by" flags as the patch has changed.
>>>>>
>>>>> v12: Moved the comments from arch/x86/kernel/cpu/resctrl/internal.h
>>>>>       to Documentation/arch/x86/resctrl.rst. (Boris)
>>>>> ---
>>>>>   Documentation/arch/x86/resctrl.rst | 64 ++++++++++++++++++++++++++++++
>>>>>   1 file changed, 64 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
>>>>> index 178ab1d8f747..1163da74f734 100644
>>>>> --- a/Documentation/arch/x86/resctrl.rst
>>>>> +++ b/Documentation/arch/x86/resctrl.rst
>>>>> @@ -369,6 +369,70 @@ When monitoring is enabled all MON groups will also contain:
>>>>>       the sum for all tasks in the CTRL_MON group and all tasks in
>>>>>       MON groups. Please see example section for more details on usage.
>>>>>   +RESCTRL filesystem implementation notes
>>>>> +=======================================
>>>>> +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
>>>>> +
>>>> Could you please do a "make htmldocs" and then ensure that the output looks sane?
>>>> The resulting output does not look right to me. For example, the above turns into
>>>> a single line that looks like:
>>>>
>>>> /sys/fs/resctrl/ | -> info | | (Top level directory named "info". Contains files that | provide details on control and monitoring resources") | -> base
>>>>
>>>>
>>>> The spacing also looks off when viewing this in html.
>>>
>> I have fixed the all the format issues. Please let me know if it
>> looks ok. I will send the final version after that.
>>
>> Attached the patch and also added the diff inline.
> 
> This still does not look as though you consider how the document 
> looks after the changes.
> 
> Consider the organization. Before your changes (assume numbering starts
> at 1):
> 
> 1. User Interface for Resource Control feature
> 1.1 Info directory
> 1.2 Resource alloc and monitor groups
> 1.2.1 Resource allocation rules
> 1.2.2 Resource monitoring rules
> 1.3 Notes on cache occupancy monitoring and control
> ...
> 
> After your changes:
> 
> 1. User Interface for Resource Control feature
> 1.1 Info directory
> 1.2 Resource alloc and monitor groups
> 1.2.1 Resource allocation rules
> 1.2.2 Resource monitoring rules
> 1.2.3 RESCTRL filesystem implementation notes
> 1.3 Notes on cache occupancy monitoring and control
> ...
> 
> Note how the "RESCTRL filesystem implementation notes" is inserted
> as a subsection of resource and monitoring groups. Since the text
> describes all files in resctrl (not just resource groups) I expect
> that it would not be buried as a subsection of resource groups.

we can make a separate sub chapter for this. Something like this.

1. User Interface for Resource Control feature
1.1 Info directory
1.2 Resource alloc and monitor groups
1.2.1 Resource allocation rules
1.2.2 Resource monitoring rules
1.3 RESCTRL filesystem implementation notes
1.3.1 info directory structure
1.3.2 base directory structure
1.3 Notes on cache occupancy monitoring and control

> 
> This addition also ignores existing customs. Nowhere in the
> entire document will you find "RESCTRL" (well, except for the
> config option).

Sure. We can change this to "resctrl"

>  
>> Thanks
>>
>> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
>> index 178ab1d8f747..e990272e9a4f 100644
>> --- a/Documentation/arch/x86/resctrl.rst
>> +++ b/Documentation/arch/x86/resctrl.rst
>> @@ -396,6 +396,67 @@ Resource monitoring rules
>>  3) Otherwise RDT events for the task will be reported in the root level
>>     "mon_data" group.
>>
>> +RESCTRL filesystem implementation notes
>> +---------------------------------------
>> +RESCTRL filesystem has two main components.
> 
> This documentation was extracted as-is from the header file where it
> was next to the flags being documented. With this move all that context
> is lost so it may be helpful to summarize what is documented here.

I can move the notes here with some modifications.

> 
>> +
>> +a. info
>> +b. base
>> +
>> +::
> 
> If I understand correctly, instead of adjusting to the target format you
> hardcode all the text as pre-formatted? That may be ok. I cannot speak to
> whether this matches expectations of the proposal to move the documentation
> here.

I am not sure either.

> 
>> +
>> +       /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 uses flags for files, not for directories. Directories
>> +                     are created based on the resource type. Added the directories
>> +                     below for better understanding. The RFTYPE flags are defined
>> +                     in arch/x86/kernel/cpu/resctrl/internal.h.
>> +
>> +"info directory structure"::
>> +
> 
> This unexpected. In your previous response you noted that you did not know
> how to make it look like a header. I expected that you would read the manual
> to answer your own question but instead you just placed the title in quotes? I do
> not see how placing text in quotes create impression that it is a header.
> There is syntax to indicate section headers.

Actually, i have kind of understood all the formatting details. Still
learning though. Attached the patch.
  

Hi Babu,

On 10/17/2023 10:47 AM, Moger, Babu wrote:
> Hi Reinette,
> 
> Boris pulled rest of the patches. Thank you very much for the feedback and
> patience.
> 
> I can send this as a separate patch with all the FTYPE documentation
> update. This need go later.

ok

> 
> 
> On 10/16/23 15:46, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 10/14/2023 4:06 PM, Moger, Babu wrote:
>>> On 10/13/2023 4:39 PM, Moger, Babu wrote:
>>>> On 10/13/2023 4:23 PM, Reinette Chatre wrote:
>>>>> On 10/13/2023 1:25 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>
>>>>>> ---
>>>>>> v13: Fixed the format issues in the documentation. Needed few minor format
>>>>>>       changes to address the syntax issues.(Reinette)
>>>>>>       Removed "Reviewed-by and Tested-by" flags as the patch has changed.
>>>>>>
>>>>>> v12: Moved the comments from arch/x86/kernel/cpu/resctrl/internal.h
>>>>>>       to Documentation/arch/x86/resctrl.rst. (Boris)
>>>>>> ---
>>>>>>   Documentation/arch/x86/resctrl.rst | 64 ++++++++++++++++++++++++++++++
>>>>>>   1 file changed, 64 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
>>>>>> index 178ab1d8f747..1163da74f734 100644
>>>>>> --- a/Documentation/arch/x86/resctrl.rst
>>>>>> +++ b/Documentation/arch/x86/resctrl.rst
>>>>>> @@ -369,6 +369,70 @@ When monitoring is enabled all MON groups will also contain:
>>>>>>       the sum for all tasks in the CTRL_MON group and all tasks in
>>>>>>       MON groups. Please see example section for more details on usage.
>>>>>>   +RESCTRL filesystem implementation notes
>>>>>> +=======================================
>>>>>> +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
>>>>>> +
>>>>> Could you please do a "make htmldocs" and then ensure that the output looks sane?
>>>>> The resulting output does not look right to me. For example, the above turns into
>>>>> a single line that looks like:
>>>>>
>>>>> /sys/fs/resctrl/ | -> info | | (Top level directory named "info". Contains files that | provide details on control and monitoring resources") | -> base
>>>>>
>>>>>
>>>>> The spacing also looks off when viewing this in html.
>>>>
>>> I have fixed the all the format issues. Please let me know if it
>>> looks ok. I will send the final version after that.
>>>
>>> Attached the patch and also added the diff inline.
>>
>> This still does not look as though you consider how the document 
>> looks after the changes.
>>
>> Consider the organization. Before your changes (assume numbering starts
>> at 1):
>>
>> 1. User Interface for Resource Control feature
>> 1.1 Info directory
>> 1.2 Resource alloc and monitor groups
>> 1.2.1 Resource allocation rules
>> 1.2.2 Resource monitoring rules
>> 1.3 Notes on cache occupancy monitoring and control
>> ...
>>
>> After your changes:
>>
>> 1. User Interface for Resource Control feature
>> 1.1 Info directory
>> 1.2 Resource alloc and monitor groups
>> 1.2.1 Resource allocation rules
>> 1.2.2 Resource monitoring rules
>> 1.2.3 RESCTRL filesystem implementation notes
>> 1.3 Notes on cache occupancy monitoring and control
>> ...
>>
>> Note how the "RESCTRL filesystem implementation notes" is inserted
>> as a subsection of resource and monitoring groups. Since the text
>> describes all files in resctrl (not just resource groups) I expect
>> that it would not be buried as a subsection of resource groups.
> 
> we can make a separate sub chapter for this. Something like this.
> 
> 1. User Interface for Resource Control feature
> 1.1 Info directory
> 1.2 Resource alloc and monitor groups
> 1.2.1 Resource allocation rules
> 1.2.2 Resource monitoring rules
> 1.3 RESCTRL filesystem implementation notes
> 1.3.1 info directory structure
> 1.3.2 base directory structure
> 1.3 Notes on cache occupancy monitoring and control

ok. Since this is the first addition of any implementation notes
it does set a precedent. In this case it implies that implementation
notes would accompany the feature being described as opposed to all
implementation notes being placed together. I am not aware of
customs in this regard but having consistent text, like
"implementation notes" that a developer can search for is useful.

>>
>> This addition also ignores existing customs. Nowhere in the
>> entire document will you find "RESCTRL" (well, except for the
>> config option).
> 
> Sure. We can change this to "resctrl"

Thank you.

> 
>>  
>>> Thanks
>>>
>>> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
>>> index 178ab1d8f747..e990272e9a4f 100644
>>> --- a/Documentation/arch/x86/resctrl.rst
>>> +++ b/Documentation/arch/x86/resctrl.rst
>>> @@ -396,6 +396,67 @@ Resource monitoring rules
>>>  3) Otherwise RDT events for the task will be reported in the root level
>>>     "mon_data" group.
>>>
>>> +RESCTRL filesystem implementation notes
>>> +---------------------------------------
>>> +RESCTRL filesystem has two main components.
>>
>> This documentation was extracted as-is from the header file where it
>> was next to the flags being documented. With this move all that context
>> is lost so it may be helpful to summarize what is documented here.
> 
> I can move the notes here with some modifications.
> 
>>
>>> +
>>> +a. info
>>> +b. base
>>> +
>>> +::
>>
>> If I understand correctly, instead of adjusting to the target format you
>> hardcode all the text as pre-formatted? That may be ok. I cannot speak to
>> whether this matches expectations of the proposal to move the documentation
>> here.
> 
> I am not sure either.
> 

It seems that since this you decided to stop using pre-formatted text for
some of the text but not all?

>>
>>> +
>>> +       /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 uses flags for files, not for directories. Directories
>>> +                     are created based on the resource type. Added the directories
>>> +                     below for better understanding. The RFTYPE flags are defined
>>> +                     in arch/x86/kernel/cpu/resctrl/internal.h.
>>> +
>>> +"info directory structure"::
>>> +
>>
>> This unexpected. In your previous response you noted that you did not know
>> how to make it look like a header. I expected that you would read the manual
>> to answer your own question but instead you just placed the title in quotes? I do
>> not see how placing text in quotes create impression that it is a header.
>> There is syntax to indicate section headers.
> 
> Actually, i have kind of understood all the formatting details. Still
> learning though. Attached the patch.

In the version I responded to you could have underlined the text with "~"
to turn it into a subsection of appropriate level.
For reference: https://docutils.sourceforge.io/docs/user/rst/quickstart.html

It is inconvenient to work with an attached patch. Please send patches
inline.

> Subject: [PATCH] x86/resctrl: Add comments on RFTYPE flags hierarchy
> 
> 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.
> 

Commit message needs to be reworked considering that it is now
documentation and not just comments added to a header file.

> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  Documentation/arch/x86/resctrl.rst | 73 ++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
> index a6279df64a9d..c89a926934fe 100644
> --- a/Documentation/arch/x86/resctrl.rst
> +++ b/Documentation/arch/x86/resctrl.rst
> @@ -407,6 +407,79 @@ Resource monitoring rules
>  3) Otherwise RDT events for the task will be reported in the root level
>     "mon_data" group.
>  
> +resctrl filesystem implementation notes
> +=======================================
> +
> +This section lists the files and directories under resctrl filesystem to
> +understand filesystem hierarchy. resctrl module uses RFTYPE flags defined
> +in arch/x86/kernel/cpu/resctrl/internal.h to create files. Directories are
> +created based on the resource type.

First sentence can be dropped. It is not clear how it relates to what this
section describes. Also, there is no resctrl module,
so "resctrl module" -> "resctrl". Code moves around, especially nowadays in
resctrl. Noting the flags are enough for a developer to find them, no
need to name the header file.

So just: 
	resctrl uses RFTYPE flags to create files. Directories are created
	based on the resource type.


> +
> +resctrl filesystem has two main components.

"components." -> "components:" ?

> +
> +1) info
> +2) base
> +
> +::
> +

The "::" above means that the text below is pre-formatted. Thus, all that
was needed was to copy the text from what you had in v11 to the new document.
I do not understand how so many issues could have slipped in while doing so 
(see below).

> +	/sys/fs/resctrl/
> +	|
> +	|--> info (Top level directory named "info". Contains files that
> +	|         provide details on control and monitoring resources")

monitoring resources" -> monitoring resources. ?

Also note that the alignment is different between the two sections. Can
the text be aligned to the parenthesis as before to match what is done
below?

> +	|
> +	--> 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)
> +


This is different from what you had before and it now appears as though base is
at a different level from info. 

Also, sentence needs to end with a period.


The text above is pre-formatted ...

> +info directory structure
> +------------------------
> +

... but the text below is no longer pre-formatted ? Why is this inconsistent?
I assumed from previous exchange that you switched to pre-formatted to not have
to deal with inconsistent spacing when using the same text in a reStructuredText
doc ... but now you are back to using the same text with the inconsistent spacing.

> +	--> RFTYPE_INFO
> +	    Directory: info
> +		--> RFTYPE_TOP (Files in top level of info directory)
> +		    File: 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)
> +			    Directory: L3_MON
> +			        Files: max_threshold_occupancy,
> +			               mbm_total_bytes_config,
> +			               mbm_local_bytes_config
> +
> +		--> RFTYPE_CTRL (Files for all control resources)
> +		    Directories: L2, L3, MB, SMBA, L2CODE, L2DATA, L3CODE, L3DATA
> +		           File: num_closids
> +
> +			--> RFTYPE_RES_CACHE (Files for cache control resources)
> +			    Directories: L2, L3, L2CODE, L2DATA, L3CODE, L3DATA
> +			          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
> +--------------------------
> +

I am not familiar enough with the syntax to know if the underlining needs to
be precise.

As above, the text below is no longer pre-formatted? This brings some
unexpected views when I try to look at it in the generated html ...

> +	--> RFTYPE_BASE (Files common for both MON and CTRL groups)
> +	    Files: cpus, cpus_list, tasks
> +
> +		--> RFTYPE_MON (Files for MON group)
> +
> +			--> RFTYPE_DEBUG (Files to help resctrl debugging)
> +			    File: mon_hw_id
> +
> +		--> RFTYPE_CTRL (Files for CTRL group)
> +		    Files: mode, schemata, size
> +

For some reason the above snippet appears different when attempting a html
view. The "-->" uses a different character and the "Files" line is next to
the flag line, not below. Could you please run "make htmldocs" and confirm
that the generated file looks sane?

> +			-> RFTYPE_DEBUG (Files to help resctrl debugging)
> +			   File: ctrl_hw_id
> +
>  
>  Notes on cache occupancy monitoring and control
>  ===============================================
> -- 

Reinette