Re: [PATCH v4 2/5] arm_mpam: resctrl: Pre-allocate assignable monitors

[Fenghua Yu <fenghuay@nvidia.com>]

Hi, Ben,

On 5/20/26 14:24, Ben Horgan wrote:
> MPAM is able to emulate ABMC, i.e. mbm_event mode, by making memory
> bandwidth monitors assignable. Rather than supporting the 'default'
> mbm_assign_mode always use 'mbm_event' mode even if there are sufficient
> memory bandwidth monitors. The per monitor event configuration is only
> provided by resctrl when in 'mbm_event' mode and so only allowing
> 'mbm_event' mode will make it easier to support per-monitor event
> configuration for MPAM. For the moment, the only event supported is
> mbm_total_event with no bandwidth type configuration. The 'mbm_assign_mode'
> file will still show 'default' when there is no support for memory
> bandwidth monitoring.
> 
> The monitors need to be allocated from the driver, and mapped to whichever
> control/monitor group resctrl wants to use them with.
> 
> Add a second array to hold the monitor values indexed by resctrl's cntr_id.
> 
> When CDP is in use, two monitors are needed so the available number of
> counters halves. Platforms with one monitor will have zero monitors when
> CDP is in use.
> 
> Co-developed-by: James Morse <james.morse@arm.com>
> Signed-off-by: James Morse <james.morse@arm.com>
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>

Reviewed-by: Fenghua Yu <fenghuay@nvidia.com>

Please check the following nit.

> ---
> Changes since rfc v1:
> abmc enabled even if enough counters
> Helpers from dropped free running commits
> carry on with zero counters if using cdp
> set config bits
> use kmalloc_objs
> drop tags for rework
> Configure mbm_cntr_configurable, mbm_cntr_assign_fixed
> 
> Changes since rfc v2:
> Don't set mon->assigned_counters to an error pointer
> Fix mpam_resctrl_teardown_mon()
> Remove free running check
> Separate cleanup allocations, e.g. __free(), from the rest
> Restrict scope on err in mpam_resctrl_monitor_init()
> 
> Changes since v3:
> Correct NULL check in mpam_resctrl_teardown_mon() (Shaopeng)
> variable allocation ordering in mpam_resctrl_pick_domain_id() (Shaopeng)
> Move mon.* assignments from mpam_resctrl_monitor_sync_abmc_vals()
> to mpam_resctrl_monitor_init_abmc() counters (Sashiko)
> use kvmalloc_obj() for allocations that may be big on some
> platforms (Sashiko)
> ---
>   drivers/resctrl/mpam_internal.h |   6 +-
>   drivers/resctrl/mpam_resctrl.c  | 139 +++++++++++++++++++++++++++++++-
>   2 files changed, 141 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h
> index 1914aefdcba9..7a166b395b5a 100644
> --- a/drivers/resctrl/mpam_internal.h
> +++ b/drivers/resctrl/mpam_internal.h
> @@ -411,7 +411,11 @@ struct mpam_resctrl_res {
>   struct mpam_resctrl_mon {
>   	struct mpam_class	*class;
>   
> -	/* per-class data that resctrl needs will live here */
> +	/* Array of allocated MBWU monitors, indexed by (closid, rmid). */
> +	int			*mbwu_idx_to_mon;
> +
> +	/* Array of assigned MBWU monitors, indexed by idx argument. */

Nit:

"idx argument" is unclear and confusing. The idx is actually cntr_id 
coming from resctrl.

Is it better s/index by idx argument/indexed by resctrl's cntr_id/?

You mentioned "indexed by resctrl's cntr_id" in the commit message 
already. I think it's clearer than simple "idx argument".

> +	int			*assigned_counters;
>   };
>  
[SNIP]

Thanks.

-Fenghua