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

["Moger, Babu" <babu.moger@amd.com>]

Hi Reinette,

On 9/19/24 11:28, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/4/24 3:21 PM, Babu Moger wrote:
>> Introduce the interface file "mbm_assign_mode" to list monitor modes
>> supported.
>>
>> The "mbm_cntr_assign" mode provides the option to assign a hardware
>> counter to an RMID and monitor the bandwidth as long as it is assigned.
>>
>> On AMD systems "mbm_cntr_assign" is backed by the ABMC (Assignable
>> Bandwidth Monitoring Counters) hardware feature. "mbm_cntr_assign" mode
>> is enabled by default when supported.
> 
> As I understand this series changed this behavior to let the architecture
> dictate whether "mbm_cntr_assign" is enabled by default.

Yes. Correct. Will change the test to mention that.

> 
>>
>> The "default" mode is the existing monitoring mode that works without the
>> explicit counter assignment, instead relying on dynamic counter assignment
>> by hardware that may result in hardware not dedicating a counter resulting
>> in monitoring data reads returning "Unavailable".
>>
>> Provide an interface to display the monitor mode on the system.
>> $cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode
>> [mbm_cntr_assign]
>> default
>>
>> Switching the mbm_assign_mode will reset all the MBM counters of all
>> resctrl groups.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v7: Updated the descriptions/commit log in resctrl.rst to generic text.
>>     Thanks to James and Reinette.
>>     Rename mbm_mode to mbm_assign_mode.
>>     Introduced mutex lock in rdtgroup_mbm_mode_show().
>>
>> v6: Added documentation for mbm_cntr_assign and legacy mode.
>>     Moved mbm_mode fflags initialization to static initialization.
>>
>> v5: Changed interface name to mbm_mode.
>>     It will be always available even if ABMC feature is not supported.
>>     Added description in resctrl.rst about ABMC mode.
>>     Fixed display abmc and legacy consistantly.
>>
>> v4: Fixed the checks for legacy and abmc mode. Default it ABMC.
>>
>> v3: New patch to display ABMC capability.
>> ---
>>  Documentation/arch/x86/resctrl.rst     | 33 ++++++++++++++++++++++++++
>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 31 ++++++++++++++++++++++++
>>  2 files changed, 64 insertions(+)
>>
>> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
>> index 30586728a4cd..a7b17ad8acb9 100644
>> --- a/Documentation/arch/x86/resctrl.rst
>> +++ b/Documentation/arch/x86/resctrl.rst
>> @@ -257,6 +257,39 @@ 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 feature is enabled.
> 
> "which feature is enabled" -> "which mode is enabled"?

Sure.

> 
>> +	::
>> +
>> +	  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 hardware counter assigned
> 
> This documentation should ideally also be appropriate for when the "soft-ABMC"
> support lands. Considering that, should all the "hardware counter" instances perhaps be
> changed to just be "counter"?

Sure.

> 
>> +	using the 'mbm_control' file. The number of hardware counters available
>> +	is described in the 'num_mbm_cntrs' file. Changing to this mode will
>> +	cause all counters on a resource to reset.
> 
> Should resctrl commit to this? Resetting of the counters as implemented here
> does seem to be an architecture specific action so this text could be
> made more generic by stating "may cause all counters on a resource to reset".

Ok. Sure.

> 
>> +
>> +	The feature is needed on platforms which support more control and monitor
> 
> "The feature" -> "The mode"?

Sure.
> 
>> +	groups than hardware counters, meaning 'unassigned' control or monitor
>> +	groups will report 'Unavailable' or not count all the traffic in an
>> +	unpredictable way.
> 
> "or not count all the traffic in an unpredictable way" is a bit hard to parse ... how
> about "or count traffic in an unpredictable way"?

ok. Sure.

> 
> 
>> +
>> +	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.
>> +
>> +	"default":
>> +
>> +	By default resctrl assumes each control and monitor group has a hardware counter.
>> +	Hardware without this property will still allow more control or monitor groups
>> +	than 'num_mbm_cntrs' to be created. Reading the mbm files may report 'Unavailable'
> Please be specific what is meant with "the mbm files"

Sure. Will change it to mbm_total_bytes and mbm_local_bytes.

> 
>> +	if there is no hardware resource assigned.
> 
> "no hardware resource" -> "no counter"?

Sure.

> 
>> +
>>  "max_threshold_occupancy":
>>  		Read/write file provides the largest value (in
>>  		bytes) at which a previously used LLC_occupancy
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 0178555bf3f6..dbc8c5e63213 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -845,6 +845,30 @@ static int rdtgroup_rmid_show(struct kernfs_open_file *of,
>>  	return ret;
>>  }
>>  
> 
> Reinette
> 

-- 
Thanks
Babu Moger