Message-ID: <9551d350-a665-462a-9f5a-d8666ae73249@amd.com>
Date: Wed, 11 Jun 2025 16:21:57 -0500
From: "Moger, Babu" <bmoger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>, babu.moger@....com,
 Peter Newman <peternewman@...gle.com>
Cc: corbet@....net, tony.luck@...el.com, tglx@...utronix.de,
 mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
 james.morse@....com, dave.martin@....com, fenghuay@...dia.com,
 x86@...nel.org, hpa@...or.com, paulmck@...nel.org,
 akpm@...ux-foundation.org, thuth@...hat.com, rostedt@...dmis.org,
 ardb@...nel.org, gregkh@...uxfoundation.org, daniel.sneddon@...ux.intel.com,
 jpoimboe@...nel.org, alexandre.chartre@...cle.com,
 pawan.kumar.gupta@...ux.intel.com, thomas.lendacky@....com,
 perry.yuan@....com, seanjc@...gle.com, kai.huang@...el.com,
 xiaoyao.li@...el.com, kan.liang@...ux.intel.com, xin3.li@...el.com,
 ebiggers@...gle.com, xin@...or.com, sohil.mehta@...el.com,
 andrew.cooper3@...rix.com, mario.limonciello@....com,
 linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
 maciej.wieczor-retman@...el.com, eranian@...gle.com, Xiaojian.Du@....com,
 gautham.shenoy@....com
Subject: Re: [PATCH v13 00/27] x86/resctrl : Support AMD Assignable Bandwidth
 Monitoring Counters (ABMC)

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

Powered by blists - more mailing lists