Re: [PATCH v13 00/27] x86/resctrl : Support AMD Assignable Bandwidth Monitoring Counters (ABMC)

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Babu,

On 6/10/25 4:19 PM, Moger, Babu wrote:
> Hi Reinette,
> 
> On 5/22/2025 11:33 AM, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 5/22/25 8:44 AM, Moger, Babu wrote:
>>> On 5/21/25 18:03, Reinette Chatre wrote:
>>
>> ...
>>
>>>> This is why I proposed in [3] that the name of the mode reflects how user can interact
>>>> with the system. Instead of one "mbm_cntr_assign" mode there can be "mbm_cntr_event_assign"
>>>> that is used for ABMC and "mbm_cntr_group_assign" that is used for soft-ABMC. The mode should
>>>> make it clear what the system is capable of wrt counter assignments.
>>>
>>> Yes, that makes sense. Perhaps we can also simplify it further:
>>>
>>> # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode:
>>> [mbm_cntr_evt_assign] <- for ABMC
>>>   mbm_cntr_grp_assign  <- for soft-ABMC
>>
>> Looks good to me. Thank you.
> 
> I am actually ready with v14 series. I have good feeling that we are getting closer to making these changes final.
> 
> So, Looking back again, it might make more sense to rename few user visible interfaces.
> 
> 1. # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode.
>    [mbm_assign_event] <- for ABMC
>     mbm_assign_group  <- for soft-ABMC
> 
>    This looks much more cleaner.  It matches with "mbm_assign_mode"

ah, I see, by dropping "cntr" it reduces confusion where ABMC assigns counters
and soft-ABMC assigned RMID. This looks good.

Taking this further, the "assign" term in "mbm_assign_event" and "mbm_assign_group" may also
be redundant considering that the filename, "mbm_assign_mode", already has "assign" in its name.

> 
> Similarly, we can rename few functions and variable names to make little more readable.
> 
> 2. mbm_cntr_assignable -> mbm_assignable
> 

I have no insight into how the soft-ABMC implementation will look and thus if it will
build on this property. If soft-ABMC uses the property then making it more generic may
help, but if it does not then it may make the code harder to read. Since this is all
internal I'd vote for keeping it mbm_cntr_assignable since the current implementation
directly associates it with hardware counters. I do not know if there will be a scenario
where a system may support *both* event and group assignable counters. The idea did
briefly come up[1]. If that may be possible then resctrl would need to distinguish them.
Also, interesting to note that the example used in (1) above notes a system that
supports both event and group assignment.

> 3. resctrl_arch_mbm_cntr_assign_enabled
>  -> >resctrl_arch_mbm_assign_enabled
> 

This is directly connected to choice for (2)

> 4. mbm_cntr_assign_enabled -> mbm_assign_enabled

hmmm ... here mbm_cntr_assign_enabled is even more directly associated with hardware
support for counter assignment. It is not clear what the benefit is to make it generic.

> 
> 5. resctrl_arch_mbm_cntr_assign_set_one ->
> 
>    resctrl_arch_mbm_assign_set_one.

Same as (4)

> 
> 6. There will few more functions. I will look into that if you agree with approach.
> 
> 7. No need to change few of these below. These are related to actual counters.
>    num_mbm_cntrs
>    available_mbm_cntrs
> 
> What do you think?

It sounds to me as though you are aiming to make the ABMC implementation more
generic in preparation for soft-ABMC support. If you have insight into the soft-ABMC
implementation then please share the details for this to be taken into account.
Until then I think it will be simpler for the implementation to be specific to
the feature being enabled here. When soft-ABMC enabling arrives the needed changes
can be made. Since this is about internals of resctrl (not the user interface) we
are not as pressured to "get it right" while not having all information required
to make these choices.

Reinette
 
[1] https://lore.kernel.org/lkml/CALPaoCj438UfH3QA_VnGo-pj2a_48sJufUWjBKT3MQatcMJ_Uw@mail.gmail.com/