Re: [PATCH v2 3/3] x86/resctrl: Keep mbm_assign_mode in default mode at boot

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

Hi Babu,

On 9/4/26 11:06 AM, Babu Moger wrote:
> ABMC ("mbm_event" mode) allows explicit assignment of hardware MBM counters
> to RMID/event pairs. It is intended for deployments that need to manage
> counter assignment on platforms where the number of monitoring groups
> exceeds the available hardware counters. Because hardware MBM counters are
> scarce, mbm_event mode is best suited for snapshot-and-rotation workflows
> that monitor a subset of groups at a time.
> 
> Commit 0f1576e43adc ("x86/resctrl: Configure mbm_event mode if supported")
> enabled ABMC automatically during initialization. This causes problems on

Se "Fixes:" tag notes in Documentation/process/maintainer-tip.rst

> systems with limited MBM counters and breaks existing userspace that
> assumes the historical default mode, including the pqos tool from
> intel-cmt-cat [1].

There is no record of this breaking pqos. *you* created [1] *after* you
submitted v1. What a strategy! I mentioned a couple of times that this is
misleading. Since you insist on proclaiming "we cannot break pqos!" as
motivation for this change you have to also disclose the consequence of
this change on pqos followed by motivation why that is acceptable. Specifically:

	https://lore.kernel.org/lkml/77f77d02-fae7-401d-9bb5-c62b244d23cd@intel.com/

> 
> For example, pqos mounts resctrl and creates 16 or more monitoring groups,
> using two counters per group (mbm_local_bytes and mbm_total_bytes). On
> platforms that provide 32 MBM counters per domain, this consumes the entire
> counter pool. Additional groups cannot be assigned counters and pqos
> reports zero bandwidth for them.
> 
> Leave mbm_assign_mode in "default" mode during initialization. Default mode
> can support more monitoring groups (up to 64) than mbm_event mode, which is

"up to 64" - so it may be fewer than 64? What is guidance to users about
how many monitoring groups in "default" mode are "safe"?

> typically limited to 16 groups because of hardware counter availability.
> Common deployments with a modest number of monitoring groups continue to
> receive accurate bandwidth measurements.

Please be specific and do not hide the consequences in a note at the end of changelog.
For example,
	Deployments with 64 or fewer monitoring groups will receive accurate
	bandwidth measurements. The hardware supports 4096 monitoring groups.
	Deployments with 65 to 4096 monitoring groups may (without user-visible
	indication) receive misleading values or "Unavailable".

	Users that need stable measurements across 65 or more monitoring
	groups should use mbm_event mode and rotate assignments as needed.

Although, the earlier text is "up to 64" so above attempt at guidance may not
be correct and there is no knowing how many monitoring groups are guaranteed
to receive accurate counts?

> 
> Users that require ABMC functionality can enable it explicitly:
> 
>   echo mbm_event > /sys/fs/resctrl/info/L3_MON/mbm_assign_mode
> 
> Note that default mode has a long-standing limitation when the number of
> monitoring groups exceeds the available counter pool. After hardware
> counter reallocation, reads may return "Unavailable" or misleading values.
> Users that need stable measurements across a large number of monitoring
> groups should use mbm_event mode and rotate assignments as needed.
> 
> Update Documentation/filesystems/resctrl.rst to reflect the default boot
> behavior, document the limitations of default mode, and adjust
> mbm_assign_mode examples accordingly.
> 

If the plan is to send this to stable then it needs a "Fixes:" tag.

> Signed-off-by: Babu Moger <babu.moger@amd.com>
> Link: https://github.com/intel/intel-cmt-cat/issues/311 # [1]

"Link:" -> "Closes:" (and then move it above SoB)?

> ---
> v2:
>   Added documentation describing the known issue with the default mode.
>   Will add cc to stable once we have all the things in order.
>   Let me know if I missed anything.
> 
> v1:
>   https://lore.kernel.org/lkml/8cb66e18e32e4087a9712c1e68ee6da614efe244.1784322818.git.babu.moger@amd.com/
> ---
>  Documentation/filesystems/resctrl.rst | 79 +++++++++++++++++----------
>  arch/x86/kernel/cpu/resctrl/monitor.c |  1 -
>  2 files changed, 49 insertions(+), 31 deletions(-)
> 
> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
> index 79feeb1dc296..a43ed3c89a93 100644
> --- a/Documentation/filesystems/resctrl.rst
> +++ b/Documentation/filesystems/resctrl.rst
> @@ -355,8 +355,8 @@ with the following files:
>  	::
>  
>  	  # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode
> -	  [mbm_event]
> -	  default
> +	  [default]
> +	  mbm_event
>  
>  	"mbm_event":
>  
> @@ -377,20 +377,30 @@ with the following files:
>  	will return 'Unassigned' when read until the user assigns one using
>  	"mbm_L3_assignments".
>  
> -	The mode is beneficial for AMD platforms that support more CTRL_MON
> -	and MON groups than available hardware counters. By default, this
> -	feature is enabled on AMD platforms with the ABMC (Assignable Bandwidth
> -	Monitoring Counters) capability, ensuring counters remain assigned even
> -	when the corresponding RMID is not actively used by any processor.
> +	The mode is beneficial for AMD platforms that support more CTRL_MON and MON
> +	groups than available hardware counters. The mbm_event mode ensures counters
> +	remain assigned even when the corresponding RMID is not actively monitored.
>  
>  	"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_event mode, if supported, to prevent reset of MBM
> -	events between reads resulting from hardware re-allocating counters. This can
> -	result in misleading values or display "Unavailable" if no counter is assigned
> -	to the event.
> +	event within every CTRL_MON and MON group. This mode is enabled by default.
> +
> +	Default mode has a long-standing limitation on AMD platforms that support

Please be specific and drop unnecessary words. For example, "long-standing" can be dropped.

> +	more CTRL_MON and MON groups than hardware counters. Hardware dynamically
> +	shares a smaller pool of counters among RMIDs. The size of that pool is
> +	not enumerated to software (unlike "num_mbm_cntrs" in mbm_event mode), and

I do not think access to "num_mbm_cntrs" depends on mbm_event mode being enabled
so "in mbm_event mode" can just be dropped?

> +	"num_rmids" may be much larger. On current AMD platforms this pool can

"current AMD platforms" does not age well in documentation. Can "current" just be dropped?

> +	provide more counters than mbm_event mode (for example 64, versus 32 ABMC

"ABMC" -> "mbm_event mode"?

There seems to be another distinction that just "more counters": what can be counted
by such counter. Specifically, a single counter from from the "pool of 64" seems to count
*all* events associated with an RMID, while a single counter from the "pool of 32 mbm_event
mode counters" can only count a single event associated with an RMID? This documentation
uses the term "counter" interchangeably and is difficult to follow.

> +	counters), so more groups can be monitored accurately than with mbm_event.

"mbm_event" -> "mbm_event mode"?

> +	Typical usage with fewer groups keeps a counter attached and readings

Drop "Typical usage" and just be specific about the different scenarios. It will be easier
for user to determine how their usage matches to specific scenarios than to try and
determine if their usage is "typical".

Also related to above text, since resctrl documentation usually refers to counters being
assigned to event/group pairs this is not clear about what the counter is attached to.

> +	remain accurate. Creating more groups than that pool (for example 64 or

"64" -> "65"?

> +	more) can cause hardware to re-allocate counters
> +	between reads. Bandwidth values may then be misleading, or reads may return

"between reads" - what reads are referred to here?

> +	"Unavailable" if no counter is allocated to the event. There is no

hmmm ... now it is refering to counter being allocated to event, but this seems to
be referring to the counters assigned to RMID which would mean it counts all the events?

> +	user-visible indication when this begins. Users who need stable readings
> +	for many groups should switch to mbm_event mode, if supported, and assign

"many groups" -> "65 or more"?

Since this is not enumerated this may be the best guidance that can be provided ... although
since the changelog mentions "up to 64" there really seems no way for users to know how
many monitor groups are "safe"?

> +	counters to the groups of interest (rotating assignments as needed).
>  
>  	* To enable "mbm_event" counter assignment mode:
>  	  ::
> @@ -474,8 +484,8 @@ with the following files:
>  
>  	Determines if a counter will automatically be assigned to an RMID, MBM event
>  	pair when its associated monitor group is created via mkdir. Enabled by default
> -	on boot, also when switched from "default" mode to "mbm_event" counter assignment
> -	mode. Users can disable this capability by writing to the interface.
> +	when switched to "mbm_event" counter assignment mode. Users can disable this

Why is this change necessary? It seems to drop the text that mbm_assign_on_mkdir is
enabled on boot ... but it is still enabled on boot, no?

> +	capability by writing to the interface.
>  
>  	"0":
>  		Auto assignment is disabled.
> @@ -1791,32 +1801,41 @@ a. Check if MBM counter assignment mode is supported.
>  
>    # mount -t resctrl resctrl /sys/fs/resctrl/
>  
> +  # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode
> +  [default]
> +  mbm_event
> +
> +The "mbm_event" and "default" modes are supported. The "default" mode
> +is enabled by default.
> +
> +b. Enable "mbm_event" counter assignment mode.
> +::
> +
> +  # echo "mbm_event" > /sys/fs/resctrl/info/L3_MON/mbm_assign_mode
>    # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode
>    [mbm_event]
>    default
>  
> -The "mbm_event" mode is detected and enabled.
> -
> -b. Check how many assignable counters are supported.
> +c. Check how many assignable counters are supported.

<snip>

So many hunks follow and all they do is relabel the steps. This is a lot of churn for
a fix.

What if "step a" instead just drops the 
	# mount -t resctrl resctrl /sys/fs/resctrl/
step that implies "mbm_event" is the default?

If so, all these hunks could just be replaced with, for example:

diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
index e4b66af55ffb..1c3c27444dd3 100644
--- a/Documentation/filesystems/resctrl.rst
+++ b/Documentation/filesystems/resctrl.rst
@@ -1783,11 +1783,9 @@ View the llc occupancy snapshot::
 Examples on working with mbm_assign_mode
 ========================================
 
-a. Check if MBM counter assignment mode is supported.
+a. Check if MBM counter assignment mode is supported and enabled.
 ::
 
-  # mount -t resctrl resctrl /sys/fs/resctrl/
-
   # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode
   [mbm_event]
   default


> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 3838e0a13d36..8a0d6086518b 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -471,7 +471,6 @@ int __init rdt_get_l3_mon_config(struct rdt_resource *r)
>  		r->mon.mbm_cntr_configurable = true;
>  		cpuid_count(0x80000020, 5, &eax, &ebx, &ecx, &edx);
>  		r->mon.num_mbm_cntrs = (ebx & GENMASK(15, 0)) + 1;
> -		hw_res->mbm_cntr_assign_enabled = true;
>  	}
>  
>  	r->mon_capable = true;

Reinette