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

["Moger, Babu" <bmoger@amd.com>]

Hi Reinette,

On 6/11/2025 1:29 PM, Reinette Chatre wrote:
> 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.

ok. Sure. It will be

# cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode.
   [mbm_event] <- for ABMC
    mbm_group  <- for soft-ABMC


> 
>>
>> 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.

Ok. That is fine. Lets keep it as is then.

> 
>> 3. resctrl_arch_mbm_cntr_assign_enabled
>>   -> >resctrl_arch_mbm_assign_enabled
>>
> 
> This is directly connected to choice for (2)

Ok.

> 
>> 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.

Ok.

> 
>>
>> 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.

Ok. Sure. That is fine. Lets keep the internals to implementation 
specific for now.

Thanks
Babu

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