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

[Ben Horgan <ben.horgan@arm.com>]

Hi Fenghua,

On 7/3/26 06:30, Fenghua Yu wrote:
> 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".

Sure, updated.

Thanks,

Ben

> 
>> +    int            *assigned_counters;
>>   };
>>  
> [SNIP]
> 
> Thanks.
> 
> -Fenghua