Re: [PATCH v12 08/26] x86/resctrl: Introduce the interface to display monitor mode

[Moger, Babu]

Hi Reinette,

On 4/11/25 15:56, Reinette Chatre wrote:
> Hi Babu,
> 
> On 4/3/25 5:18 PM, Babu Moger wrote:
>> Introduce the interface file "mbm_assign_mode" to list monitor modes
> 
> Using "resctrl file" instead of "interface file" should help to make it
> clear what this patch does.

ok. Sure.

> 
>> supported.
>>
>> The "mbm_cntr_assign" mode provides the option to assign a counter to
>> an RMID, event pair and monitor the bandwidth as long as it is assigned.
>>
>> On AMD systems "mbm_cntr_assign" mode is backed by the ABMC (Assignable
>> Bandwidth Monitoring Counters) hardware feature and is enabled by default.
>>
>> 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
>>
>> Added an IS_ENABLED(CONFIG_RESCTRL_ASSIGN_FIXED) check to handle Arm64
> 
> (needs imperative)

Sure.

> 
>> platforms. On x86, CONFIG_RESCTRL_ASSIGN_FIXED is not defined, whereas on
>> Arm64, it is. As a result, for MPAM, the file would be either:
> 
> CONFIG_RESCTRL_ASSIGN_FIXED does not yet exist anywhere so this motivation needs
> to provide stronger support for why it is used before it exists. There is a precedent
> here with RESCTRL_RMID_DEPENDS_ON_CLOSID already used while it does not yet
> appear in a Kconfig file. I would propose that this is motivated by noting
> how it is already understood how Arm supports assignable counters this was recommended
> by James to prepare for that work. Since this is user interface this
> work is done early to ensure user interface is compatible with that upcoming
> support. Also set folks at ease that IS_ENABLED() works as expected with a
> non-existing config.

How about this?

Add IS_ENABLED(CONFIG_RESCTRL_ASSIGN_FIXED) check to support Arm64.

On x86, CONFIG_RESCTRL_ASSIGN_FIXED is not defined. On Arm64, it will be
defined when the "mbm_cntr_assign" mode is supported.

Add an IS_ENABLED(CONFIG_RESCTRL_ASSIGN_FIXED) check early to ensure the
user interface remains compatible with upcoming Arm64 support.
IS_ENABLED() safely evaluates to 0 when the configuration is not defined.

As a result, for MPAM, the file would be either:
[default]
or
[mbm_cntr_assign]


> 
> 
>> [default]
>> or
>> [mbm_cntr_assign]
>>
>> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
>> ---
>> v12: Minor text update in change log and user documentation.
>>      Added the check CONFIG_RESCTRL_ASSIGN_FIXED to take care of arm platforms.
>>      This will be defined only in arm and not in x86.
>>
>> v11: Renamed rdtgroup_mbm_assign_mode_show() to resctrl_mbm_assign_mode_show().
>>      Removed few texts in resctrl.rst about AMD specific information.
>>      Updated few texts.
>>
>> v10: Added few more text to user documentation clarify on the default mode.
>>
>> v9: Updated user documentation based on comments.
>>
>> v8: Commit message update.
>>
>> 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.
>>
>> ???END
>> ---
>>  Documentation/arch/x86/resctrl.rst     | 27 +++++++++++++++++++
>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 37 ++++++++++++++++++++++++++
>>  2 files changed, 64 insertions(+)
>>
>> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
>> index fb90f08e564e..bb96b44019fe 100644
>> --- a/Documentation/arch/x86/resctrl.rst
>> +++ b/Documentation/arch/x86/resctrl.rst
>> @@ -257,6 +257,33 @@ 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, a monitoring event can only accumulate data
>> +	while it is backed by a hardware counter. The user-space is able to
>> +	specify which of the events in CTRL_MON or MON groups should have a
>> +	counter assigned using the "mbm_assign_control" file. The number of
> 
> "mbm_assign_control" no longer exist.

The user-space is able to specify which of the events in CTRL_MON or MON
groups should have a counter assigned using the "mbm_L3_assignments"
interface file in each resctrl group.

> 
>> +	counters available is described in the "num_mbm_cntrs" file. Changing
>> +	the mode may cause all counters on the resource to reset.
>> +
>> +	"default":
>> +
>> +	In default mode, resctrl assumes there is a hardware counter for each
>> +	event within every CTRL_MON and MON group. On AMD platforms, it is
>> +	recommended to use the mbm_cntr_assign mode, if supported, to prevent
>> +	the hardware from resetting counters between reads. This can result in
> 
> "from resetting counters" -> "from re-allocating counters"?

How about?

"from resetting MBM events between reads"

> 
>> +	misleading values or display "Unavailable" if no counter is assigned
>> +	to the event.
>> +
>>  "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 17de38e26f94..626be6becca7 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -882,6 +882,36 @@ static int rdtgroup_rmid_show(struct kernfs_open_file *of,
>>  	return ret;
>>  }
>>  
>> +static int resctrl_mbm_assign_mode_show(struct kernfs_open_file *of,
>> +					struct seq_file *s, void *v)
>> +{
>> +	struct rdt_resource *r = of->kn->parent->priv;
>> +	bool enabled;
>> +
>> +	mutex_lock(&rdtgroup_mutex);
>> +	enabled = resctrl_arch_mbm_cntr_assign_enabled(r);
>> +
>> +	if (r->mon.mbm_cntr_assignable) {
>> +		if (enabled)
>> +			seq_puts(s, "[mbm_cntr_assign]\n");
>> +		else
>> +			seq_puts(s, "[default]\n");
>> +
>> +		if (!IS_ENABLED(CONFIG_RESCTRL_ASSIGN_FIXED)) {
>> +			if (enabled)
>> +				seq_puts(s, "default\n");
>> +			else
>> +				seq_puts(s, "mbm_cntr_assign\n");
>> +		}
>> +	} else {
>> +		seq_puts(s, "[default]\n");
>> +	}
>> +
>> +	mutex_unlock(&rdtgroup_mutex);
>> +
>> +	return 0;
>> +}
>> +
>>  #ifdef CONFIG_PROC_CPU_RESCTRL
>>  
>>  /*
>> @@ -1908,6 +1938,13 @@ static struct rftype res_common_files[] = {
>>  		.seq_show	= mbm_local_bytes_config_show,
>>  		.write		= mbm_local_bytes_config_write,
>>  	},
>> +	{
>> +		.name		= "mbm_assign_mode",
>> +		.mode		= 0444,
>> +		.kf_ops		= &rdtgroup_kf_single_ops,
>> +		.seq_show	= resctrl_mbm_assign_mode_show,
>> +		.fflags		= RFTYPE_MON_INFO,
> 
> Needs a RFTYPE_RES_CACHE?

I am not very sure about this.  This flag is added to the files in info/L3.

"mbm_assign_mode" goes in info/L3_MON/

The files in L3_MON does not have these flags set (for example
mon_features, num_rmids).

> 
>> +	},
>>  	{
>>  		.name		= "cpus",
>>  		.mode		= 0644,
> 
> Reinette
> 

-- 
Thanks
Babu Moger