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

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

Hi Koba,

On 5/26/26 03:50, Koba Ko wrote:
> 
>>
>> +/*
>> + * 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];
>> +       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(kvfree) = kvmalloc_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);
> 
> hi Ben,
> One thing double-checking here.
> The allocation path uses class->props.num_mbwu_mon,
> but teardown frees using l3_num_allocated_mbwu,
> which is the global minimum exposed counter counter.
> If classes can have different num_mbwu_mon values, a class with more
> monitors than the global minimum may leak the tail of assigned_counters during
> teardown.
> 
> koba

Thanks for bringing this up. The l3_num_allocated_mbwu is a hangover from when
this code supported both mbm_total_bytes and mbm_local_bytes with a separate
class for each. As we've now decided that mbm_total_bytes is the only bandwidth
counter it makes sense to support we will always have at most single class with
allocated bandwidth counters. Hence, I don't think there is a leak but
l3_num_allocated_mbwu adds unneeeded complication so I'll have a go at removing it.

Thanks,

Ben

> 
>> +       if (IS_ERR(cntrs))
>> +               return PTR_ERR(cntrs);
>> +       mon->assigned_counters = cntrs;
>> +       mon->mbwu_idx_to_mon = no_free_ptr(rmid_array);
>> +
>> +       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;
>> +
>> +       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 +1234,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 +1811,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;
>> +
>> +       if (mon->assigned_counters) {
>> +               __free_mbwu_mon(class, mon->assigned_counters, num_mbwu_mon);
>> +               kvfree(mon->assigned_counters);
>> +               mon->assigned_counters = NULL;
>> +       }
>> +
>> +       kvfree(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 +1850,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
>>