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

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

Hi Shaopeng,

Thanks for the quick review.

On 5/12/26 08:16, Shaopeng Tan (Fujitsu) wrote:
> Hello Ben,
> 
> 
>> 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>
>> ---
>> 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()
>> ---
>>  drivers/resctrl/mpam_internal.h |   6 +-
>>  drivers/resctrl/mpam_resctrl.c  | 138 +++++++++++++++++++++++++++++++-
>>  2 files changed, 140 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. */
>> +       int                     *assigned_counters;
>>  };
>>  
>>  static inline int mpam_alloc_csu_mon(struct mpam_class *class)
>> diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c
>> index f70fa65d39e4..cba295621f56 100644
>> --- a/drivers/resctrl/mpam_resctrl.c
>> +++ b/drivers/resctrl/mpam_resctrl.c
>> @@ -75,6 +75,8 @@ static DECLARE_WAIT_QUEUE_HEAD(wait_cacheinfo_ready);
>>   */
>>  static bool resctrl_enabled;
>>  
>> +static unsigned int l3_num_allocated_mbwu = ~0;
>> +
>>  bool resctrl_arch_alloc_capable(void)
>>  {
>>          struct mpam_resctrl_res *res;
>> @@ -140,7 +142,7 @@ int resctrl_arch_cntr_read(struct rdt_resource *r, struct rdt_l3_mon_domain *d,
>>  
>>  bool resctrl_arch_mbm_cntr_assign_enabled(struct rdt_resource *r)
>>  {
>> -       return false;
>> +       return (r == &mpam_resctrl_controls[RDT_RESOURCE_L3].resctrl_res);
>>  }
>>  
>>  int resctrl_arch_mbm_cntr_assign_set(struct rdt_resource *r, bool enable)
>> @@ -185,6 +187,22 @@ static void resctrl_reset_task_closids(void)
>>          read_unlock(&tasklist_lock);
>>  }
>>  
>> +static void mpam_resctrl_monitor_sync_abmc_vals(struct rdt_resource *l3)
>> +{
>> +       l3->mon.num_mbm_cntrs = l3_num_allocated_mbwu;
>> +       if (cdp_enabled)
>> +               l3->mon.num_mbm_cntrs /= 2;
>> +
>> +       /*
>> +        * Continue as normal even if there are zero counters to avoid giving
>> +        * resctrl mixed messages.
>> +        */
>> +       l3->mon.mbm_cntr_assignable = true;
>> +       l3->mon.mbm_assign_on_mkdir = true;
>> +       l3->mon.mbm_cntr_configurable = false;
>> +       l3->mon.mbm_cntr_assign_fixed = true;
>> +}
>> +
>>  int resctrl_arch_set_cdp_enabled(enum resctrl_res_level rid, bool enable)
>>  {
>>          u32 partid_i = RESCTRL_RESERVED_CLOSID, partid_d = RESCTRL_RESERVED_CLOSID;
>> @@ -244,6 +262,7 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level rid, bool enable)
>>          WRITE_ONCE(arm64_mpam_global_default, mpam_get_regval(current));
>>  
>>          resctrl_reset_task_closids();
>> +       mpam_resctrl_monitor_sync_abmc_vals(l3);
>>  
>>          for_each_possible_cpu(cpu)
>>                  mpam_set_cpu_defaults(cpu, partid_d, partid_i, 0, 0);
>> @@ -613,6 +632,9 @@ static bool class_has_usable_mbwu(struct mpam_class *class)
>>          if (!mpam_has_feature(mpam_feat_msmon_mbwu, cprops))
>>                  return false;
>>  
>> +       if (!cprops->num_mbwu_mon)
>> +               return false;
>> +
>>          return true;
>>  }
>>  
>> @@ -935,6 +957,52 @@ static void mpam_resctrl_pick_mba(void)
>>          }
>>  }
>>  
>> +static void __free_mbwu_mon(struct mpam_class *class, int *array,
>> +                           u16 num_mbwu_mon)
>> +{
>> +       for (int i = 0; i < num_mbwu_mon; i++) {
>> +               if (array[i] < 0)
>> +                       continue;
>> +
>> +               mpam_free_mbwu_mon(class, array[i]);
>> +               array[i] = ~0;
>> +       }
>> +}
>> +
>> +static int __alloc_mbwu_mon(struct mpam_class *class, int *array,
>> +                           u16 num_mbwu_mon)
>> +{
>> +       for (int i = 0; i < num_mbwu_mon; i++) {
>> +               int mbwu_mon = mpam_alloc_mbwu_mon(class);
>> +
>> +               if (mbwu_mon < 0) {
>> +                       __free_mbwu_mon(class, array, num_mbwu_mon);
>> +                       return mbwu_mon;
>> +               }
>> +               array[i] = mbwu_mon;
>> +       }
>> +
>> +       l3_num_allocated_mbwu = min(l3_num_allocated_mbwu, num_mbwu_mon);
>> +
>> +       return 0;
>> +}
>> +
>> +static int *__alloc_mbwu_array(struct mpam_class *class, u16 num_mbwu_mon)
>> +{
>> +       int err;
>> +
>> +       int *array __free(kfree) = kmalloc_objs(*array, num_mbwu_mon);
>> +       if (!array)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       memset(array, -1, num_mbwu_mon * sizeof(*array));
>> +
>> +       err = __alloc_mbwu_mon(class, array, num_mbwu_mon);
>> +       if (err)
>> +               return ERR_PTR(err);
>> +       return_ptr(array);
>> +}
>> +
>>  static void counter_update_class(enum resctrl_event_id evt_id,
>>                                   struct mpam_class *class)
>>  {
>> @@ -1089,6 +1157,38 @@ static int mpam_resctrl_pick_domain_id(int cpu, struct mpam_component *comp)
>>          return comp->comp_id;
>>  }
>>  
>> +/*
>> + * This must run after all event counters have been picked so that any free
>> + * running counters have already been allocated.
>> + */
>> +static int mpam_resctrl_monitor_init_abmc(struct mpam_resctrl_mon *mon)
>> +{
>> +       struct mpam_resctrl_res *res = &mpam_resctrl_controls[RDT_RESOURCE_L3];
>> +       struct rdt_resource *l3 = &res->resctrl_res;
>> +       struct mpam_class *class = mon->class;
>> +       u16 num_mbwu_mon;
>> +       size_t num_rmid = resctrl_arch_system_num_rmid_idx();
>> +       int *cntrs;
> 
> Wouldn't a reverse tree format be better?

Do you mean just moving the num_rmid line up? If so, yes, that is a bit neater.

	struct mpam_resctrl_res *res = &mpam_resctrl_controls[RDT_RESOURCE_L3];
	size_t num_rmid = resctrl_arch_system_num_rmid_idx();
	struct rdt_resource *l3 = &res->resctrl_res;
	struct mpam_class *class = mon->class;
	u16 num_mbwu_mon;
	int *cntrs;

> 
>> +       int *rmid_array __free(kfree) = kmalloc_objs(*rmid_array, num_rmid);
>> +       if (!rmid_array) {
>> +               pr_debug("Failed to allocate RMID array\n");
>> +               return -ENOMEM;
>> +       }
>> +       memset(rmid_array, -1, num_rmid * sizeof(*rmid_array));
>> +
>> +       num_mbwu_mon = class->props.num_mbwu_mon;
>> +       cntrs = __alloc_mbwu_array(mon->class, num_mbwu_mon);
>> +       if (IS_ERR(cntrs))
>> +               return PTR_ERR(cntrs);
>> +       mon->assigned_counters = cntrs;
>> +       mon->mbwu_idx_to_mon = no_free_ptr(rmid_array);
>> +
>> +       mpam_resctrl_monitor_sync_abmc_vals(l3);
>> +
>> +       return 0;
>> +}
>> +
>>  static int mpam_resctrl_monitor_init(struct mpam_resctrl_mon *mon,
>>                                       enum resctrl_event_id type)
>>  {
>> @@ -1133,8 +1233,21 @@ static int mpam_resctrl_monitor_init(struct mpam_resctrl_mon *mon,
>>           */
>>          l3->mon.num_rmid = resctrl_arch_system_num_rmid_idx();
>>  
>> -       if (resctrl_enable_mon_event(type, false, 0, NULL))
>> -               l3->mon_capable = true;
>> +       if (type == QOS_L3_MBM_TOTAL_EVENT_ID) {
>> +               int err;
>> +
>> +               err = mpam_resctrl_monitor_init_abmc(mon);
>> +               if (err)
>> +                       return err;
>> +
>> +               static_assert(MAX_EVT_CONFIG_BITS == 0x7f);
>> +               l3->mon.mbm_cfg_mask = MAX_EVT_CONFIG_BITS;
>> +       }
>> +
>> +       if (!resctrl_enable_mon_event(type, false, 0, NULL))
>> +               return -EINVAL;
>> +
>> +       l3->mon_capable = true;
>>  
>>          return 0;
>>  }
>> @@ -1697,6 +1810,23 @@ void mpam_resctrl_exit(void)
>>          resctrl_exit();
>>  }
>>  
>> +static void mpam_resctrl_teardown_mon(struct mpam_resctrl_mon *mon, struct mpam_class *class)
>> +{
>> +       u32 num_mbwu_mon = l3_num_allocated_mbwu;
>> +
>> +       if (mon->mbwu_idx_to_mon)
>> +               return;
> 
>  Isn't it 'if (mon->mbwu_idx_to_mon == NULL)' ?

Oops... indeed.

Thanks,

Ben

>  
>  
>  Best regards,
>  Shaopeng TAN
> 
>> +       if (mon->assigned_counters) {
>> +               __free_mbwu_mon(class, mon->assigned_counters, num_mbwu_mon);
>> +               kfree(mon->assigned_counters);
>> +               mon->assigned_counters = NULL;
>> +       }
>> +
>> +       kfree(mon->mbwu_idx_to_mon);
>> +       mon->mbwu_idx_to_mon = NULL;
>> +}
>> +
>>  /*
>>   * The driver is detaching an MSC from this class, if resctrl was using it,
>>   * pull on resctrl_exit().
>> @@ -1719,6 +1849,8 @@ void mpam_resctrl_teardown_class(struct mpam_class *class)
>>          for_each_mpam_resctrl_mon(mon, eventid) {
>>                  if (mon->class == class) {
>>                          mon->class = NULL;
>> +
>> +                       mpam_resctrl_teardown_mon(mon, class);
>>                          break;
>>                  }
>>          }
>> --
>> 2.43.0