Re: [PATCH v2 2/3] fs/resctrl: Assign counters to existing groups when enabling mbm_event

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

Hi Babu,

On 9/4/26 11:06 AM, Babu Moger wrote:
> resctrl_mbm_assign_mode_write() frees all counters and sets

The changelog is easier to read if it documents what the code does
instead of documenting the function names. In its current form the reader needs
to stop at the first word of this changelog, go to the source code,
figure out when resctrl_mbm_assign_mode_write() is called, and then be able
to return to changelog to further try and understand the change.

Consider an alternative like:
	When the user enables counter assignment mode by writing "mbm_event"
	to /sys/fs/resctrl/info/L3_MON/mbm_assign_mode resctrl resets all
	monitoring state and sets ...

> mbm_assign_on_mkdir for subsequent mkdir, but does not assign counters to
> groups that already exist, including the default group created at mount.
> Those events then read "Unassigned" until the user assigns counters by

"Those events"? No mention of "events" before this.

> hand.
> 
> Enable mbm_assign_on_mkdir and assign counters to existing CTRL_MON and MON
> groups with resctrl_assign_cntrs_allrdtgrp() so the switch matches mkdir

no need to mention the function name, this can be seen from the patch. Just mention
what the change achieves. Looks like "with resctrl_assign_cntrs_allrdtgrp()" can
just be dropped.

> auto-assignment. Groups left without a counter still read "Unassigned".

"Groups" -> "An event ..."?
"still read" -> "reads"?

This implies that counters are assigned to all groups but that may not be possible.
I am not sure what would be best text here. How about something like:

	Enable mbm_assign_on_mkdir and assign counters, while there are some
	available, to existing ...

> 
> Update Documentation/filesystems/resctrl.rst to describe this.
> 
> Fixes: 8004ea01cf63 ("fs/resctrl: Introduce the interface to switch between monitor modes")
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> v2: New patch.
>     This patch addresses the Sashiko comment about documentation issue where
>     counters are not assigned automatically when mode is switched to mbm_event.
>     https://sashiko.dev/#/patchset/8cb66e18e32e4087a9712c1e68ee6da614efe244.1784322818.git.babu.moger%40amd.com

Sounds like this is needed:

	Reported-by: Sashiko <sashiko-bot@kernel.org>
	Closes: https://sashiko.dev/#/patchset/8cb66e18e32e4087a9712c1e68ee6da614efe244.1784322818.git.babu.moger%40amd.com

Is this a stable candidate?

>     In fact, it exposed a real issue. When switching to mbm_event mode, existing
>     monitoring groups should be assigned counters whenever counters are available.
>     This provides a smooth transition between modes and aligns the behavior with
>     the existing auto-assignment mechanism.
> ---
>  Documentation/filesystems/resctrl.rst |  7 +++++--
>  fs/resctrl/monitor.c                  | 30 ++++++++++++++++++++++++---
>  2 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
> index e4b66af55ffb..79feeb1dc296 100644
> --- a/Documentation/filesystems/resctrl.rst
> +++ b/Documentation/filesystems/resctrl.rst
> @@ -371,8 +371,11 @@ with the following files:
>  	of counters available is described in the "num_mbm_cntrs" file. Changing the
>  	mode may cause all counters on the resource to reset.
>  
> -	Moving to mbm_event counter assignment mode requires users to assign the counters
> -	to the events. Otherwise, the MBM event counters will return 'Unassigned' when read.
> +	Moving to mbm_event counter assignment mode enables "mbm_assign_on_mkdir" and
> +	assigns counters to the events of all existing groups, including the default

"all existing groups" -> "all existing monitoring groups"

> +	group, for as long as counters remain available. Events left without a counter

"for as long as" implies duration. Perhaps "while counters remain available"?

> +	will return 'Unassigned' when read until the user assigns one using
> +	"mbm_L3_assignments".

hmmm ... guiding users to read each event and use the return value to learn whether
a counter is assigned or not seems inefficient. How about replacing last sentence with
something similar to the "mbm_assign_on_mkdir" doc:

	Consult "mbm_L3_assignments" after switching to "mbm_event" mode for
	counter assignment states of all monitoring groups.

>  
>  	The mode is beneficial for AMD platforms that support more CTRL_MON
>  	and MON groups than available hardware counters. By default, this
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index 73413cb128ea..61463741b91b 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -1326,6 +1326,27 @@ void rdtgroup_assign_cntrs(struct rdtgroup *rdtgrp)
>  					   &mon_event_all[QOS_L3_MBM_LOCAL_EVENT_ID]);
>  }
>  
> +/*
> + * resctrl_assign_cntrs_allrdtgrp() - Assign counters to the MBM events of every
> + *				      existing group. Called when "mbm_event" mode
> + *				      is enabled.

Please do not include caller information in function comments. This does not age well
and this patch clearly demonstrates this: rdtgroup_assign_cntrs()'s comments
read "Called when a new group is created.", after this patch those comments are no
longer accurate and thus also needs to change as part of this patch.

> + *
> + * Groups created while in "default" mode have no counter assigned, including the
> + * default group created when resctrl is mounted. Assign counters to them so that
> + * enabling the mode leaves the same assignments that mkdir would have made.

Above comment belongs in caller.

> + */
> +static void resctrl_assign_cntrs_allrdtgrp(void)
> +{
> +	struct rdtgroup *prgrp, *crgrp;
> +
> +	list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
> +		rdtgroup_assign_cntrs(prgrp);
> +
> +		list_for_each_entry(crgrp, &prgrp->mon.crdtgrp_list, mon.crdtgrp_list)
> +			rdtgroup_assign_cntrs(crgrp);
> +	}
> +}

I think sashiko's feedback about needing to test for pseudo-locked groups need not
be followed since there is no overlap between systems supporting assigned counters
and those that support pseudo-locking.

> +
>  /*
>   * rdtgroup_free_unassign_cntr() - Unassign and reset the counter ID configuration
>   * for the event pointed to by @mevt within the domain @d and resctrl group @rdtgrp.
> @@ -1599,9 +1620,6 @@ ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf,
>  									   (READS_TO_LOCAL_MEM |
>  									    READS_TO_LOCAL_S_MEM |
>  									    NON_TEMP_WRITE_TO_LOCAL_MEM);
> -		/* Enable auto assignment when switching to "mbm_event" mode */
> -		if (enable)
> -			r->mon.mbm_assign_on_mkdir = true;
>  		/*
>  		 * Reset all the non-achitectural RMID state and assignable counters.
>  		 */
> @@ -1609,6 +1627,12 @@ ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf,
>  			mbm_cntr_free_all(r, d);
>  			resctrl_reset_rmid_all(r, d);
>  		}
> +

Comment within the block can be dropped and instead a new comment can be placed here.
Something like:
	/*
	 * Counters were freed above, so both new groups (via mkdir) and the
	 * groups that already exist need assignments.
	 */

> +		if (enable) {
> +			/* Enable auto assignment when switching to "mbm_event" mode */
> +			r->mon.mbm_assign_on_mkdir = true;
> +			resctrl_assign_cntrs_allrdtgrp();
> +		}
>  	}
>  
>  out_unlock:

Reinette