Re: [PATCH v8 07/25] x86/resctrl: Introduce the interface to display monitor mode

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

Hi Babu,

On 10/16/24 8:57 AM, Moger, Babu wrote:
> On 10/15/24 22:12, Reinette Chatre wrote:
>> On 10/9/24 10:39 AM, Babu Moger wrote:

>>> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
>>> index 30586728a4cd..e4a7d6e815f6 100644
>>> --- a/Documentation/arch/x86/resctrl.rst
>>> +++ b/Documentation/arch/x86/resctrl.rst
>>> @@ -257,6 +257,40 @@ with the following files:
>>>  	    # cat /sys/fs/resctrl/info/L3_MON/mbm_local_bytes_config
>>>  	    0=0x30;1=0x30;3=0x15;4=0x15
>>>  
>>> +"mbm_assign_mode":
>>> +	Reports the list of monitoring modes supported. The enclosed brackets
>>> +	indicate which mode is enabled.
>>> +	::
>>> +
>>> +	  cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode
>>> +	  [mbm_cntr_assign]
>>> +	  default
>>> +
>>> +	"mbm_cntr_assign":
>>> +
>>> +	In mbm_cntr_assign mode user-space is able to specify which control
>>> +	or monitor groups in resctrl should have a counter assigned using the
>>
>> Counters cannot be assigned to control groups. How about replacing all instances
>> of "control and monitor groups" with "CTRL_MON and MON groups", similarly
>> "control or monitor groups" with "CTRL_MON or MON groups".
> 
> Ok.
> 
>>
>>> +	'mbm_assign_control' file. The number of counters available is described
>>
>> Looking at the rest of the doc it seems that the custom is actually to place
>> filenames in double quotes, like "mbm_assign_control".
> 
> Sure.
> 
>>
>>> +	in the 'num_mbm_cntrs' file. Changing the mode may cause all counters on
>>> +	a resource to reset.
>>> +
>>> +	The mode is useful on platforms which support more control and monitor
>>> +	groups than hardware counters, meaning 'unassigned' control or monitor
>>> +	groups will report 'Unavailable' or count the traffic in an unpredictable
>>> +	way.
>>
>> Note two more instances of "control groups" above.
>>
>> Please note that the above description implies that counter assignment is per-group. For
>> example, "specify which control	or monitor groups in resctrl should have a counter
>> assigned" and "useful on platforms which support more control and monitor groups
>> than hardware counters". This needs to be reworked to reflect that counters
>> are assigned to events.
> 
> How about this?
> 
> The mode is useful on platforms which support more CTRL_MON and MON groups
> than the hardware counters, meaning 'unassigned' events on CTRL_MON or MON
> groups will report 'Unavailable' or count the traffic in an unpredictable
> way.

This rewrites the second paragraph of the section about "mbm_cntr_assign". It is
not clear to me how this section will end up looking since the first paragraph still
seems to refer to counters being assigned to groups ("specify which control or monitor
groups in resctrl should have a counter assigned") while the later addition
to this section by "x86/resctrl: Introduce the interface to switch between monitor
modes" starts by specifying how counters are assigned to the MBM events ("The MBM
events (mbm_total_bytes and/or mbm_local_bytes) associated counters").

>>> +
>>> +	AMD Platforms with ABMC (Assignable Bandwidth Monitoring Counters) feature
>>> +	enable this mode by default so that counters remain assigned even when the
>>> +	corresponding RMID is not in use by any processor.
>>
>> I assume this should remain RMID since this specifically talks about an x86 system?
> 
> This was a suggestion from James. Let me know if you want me to change.

I can proceed to assume this is a paragraph intended to be x86 specific. No need to change.

Reinette