Re: [PATCH v12 03/26] x86/cpufeatures: Add support for Assignable Bandwidth Monitoring Counters (ABMC)

[Moger, Babu]

Hi Reinette,

On 4/15/25 11:09, Reinette Chatre wrote:
> Hi Babu,
> 
> On 4/14/25 10:48 AM, Moger, Babu wrote:
> 
>> Here is my proposal to handle this case. This can be separate patch.
>>
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index d10cf1e5b914..772f2f77faee 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1370,7 +1370,7 @@ static int rdt_mon_features_show(struct
>> kernfs_open_file *of,
>>
>>         list_for_each_entry(mevt, &r->mon.evt_list, list) {
>>                 seq_printf(seq, "%s\n", mevt->name);
>> -               if (mevt->configurable)
>> +               if (mevt->configurable &&
>> !resctrl_arch_mbm_cntr_assign_enabled(r))
>>                         seq_printf(seq, "%s_config\n", mevt->name);
>>         }
>>
>> @@ -1846,6 +1846,11 @@ static int mbm_config_show(struct seq_file *s,
>> struct rdt_resource *r, u32 evtid
>>         cpus_read_lock();
>>         mutex_lock(&rdtgroup_mutex);
>>
>> +       if (resctrl_arch_mbm_cntr_assign_enabled(r)) {
>> +               rdt_last_cmd_puts("Event configuration(BMEC) not supported
>> with mbm_cntr_assign mode\n");
>> +               return -EINVAL;
>> +       }
>> +
>>         list_for_each_entry(dom, &r->mon_domains, hdr.list) {
>>                 if (sep)
>>                         seq_puts(s, ";");
>> @@ -1865,21 +1870,24 @@ static int mbm_config_show(struct seq_file *s,
>> struct rdt_resource *r, u32 evtid
>>  static int mbm_total_bytes_config_show(struct kernfs_open_file *of,
>>                                        struct seq_file *seq, void *v)
>>  {
>> +       int ret;
>>         struct rdt_resource *r = of->kn->parent->priv;
>>
>> -       mbm_config_show(seq, r, QOS_L3_MBM_TOTAL_EVENT_ID);
>> +       ret = mbm_config_show(seq, r, QOS_L3_MBM_TOTAL_EVENT_ID);
>>
>> -       return 0;
>> +       return ret;
>>  }
>>
>>  static int mbm_local_bytes_config_show(struct kernfs_open_file *of,
>>                                        struct seq_file *seq, void *v)
>>  {
>> +       int ret;
>> +
>>         struct rdt_resource *r = of->kn->parent->priv;
>>
>> -       mbm_config_show(seq, r, QOS_L3_MBM_LOCAL_EVENT_ID);
>> +       ret = mbm_config_show(seq, r, QOS_L3_MBM_LOCAL_EVENT_ID);
>>
>> -       return 0;
>> +       return ret;
>>  }
>>
>>  static void mbm_config_write_domain(struct rdt_resource *r,
>> @@ -1932,6 +1940,11 @@ static int mon_config_write(struct rdt_resource *r,
>> char *tok, u32 evtid)
>>         /* Walking r->domains, ensure it can't race with cpuhp */
>>         lockdep_assert_cpus_held();
>>
>> +       if (resctrl_arch_mbm_cntr_assign_enabled(r)) {
>> +               rdt_last_cmd_puts("Event configuration(BMEC) not supported
>> with mbm_cntr_assign mode\n");
>> +               return -EINVAL;
>> +       }
>> +
>>  next:
>>         if (!tok || tok[0] == '\0')
>>                 return 0;
>>
> 
> Instead of chasing every call that may involve BMEC I think it will be simpler to
> disable BMEC support during initialization when ABMC is detected. Specifically,
> on systems that support both BMEC and ABMC rdt_cpu_has(X86_FEATURE_BMEC) returns
> false. 

There is one problem with this approach. Users have the option to switch
between the assignment modes. System will boot with ABMC by default if
supported. But, users can switch to 'default' mode after the boot. By
disabling the BMEC completely, it will not be possible to do that.

> 
> I would also like to consider enhancing mevt->configurable to handle all different
> ways in which events can be configured. For example, making mevt->configurable an
> enum that captures how event can be configured instead of keeping mevt->configurable
> a boolean for BMEC support and handling ABMC completely separately. I hope this
> may become clearer when using struct mon_evt for ABMC also.

Sure. I can try that.


-- 
Thanks
Babu Moger