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

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

Hi Reinette,

On 8/23/2024 3:29 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/21/24 6:31 PM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 8/16/24 16:28, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 8/6/24 3:00 PM, Babu Moger wrote:
>>>>
>>>> Feature adds following interface files:
>>>>
>>>> /sys/fs/resctrl/info/L3_MON/mbm_mode: Reports the list of assignable
>>>> monitoring features supported. The enclosed brackets indicate which
>>>> feature is enabled.
>>>
>>> I've been considering this file as a generic file where all future "MBM
>>> modes"
>>> can be captured, while this series treats it as specific to "assignable
>>> monitoring
>>> features" (btw, should this be "assignable monitoring modes" to match 
>>> the
>>> name?).
>>> Looking closer at this implementation it does make things easier that
>>> "mbm_mode" is
>>> specific to "assignable monitoring features" but when doing so I 
>>> think it
>>> should have
>>> a less generic name to avoid the obstacles we have with the existing
>>> "mon_features".
>>> Apologies that this goes back to be close to what you had earlier ... 
>>> maybe
>>> "mbm_assign_mode"?
>>
>> Lets see:
>> #cat /sys/fs/resctrl/info/L3_MON/mbm_mode
>> [mbm_cntr_assign]  <- This already says 'assign'. Isn't that enough?
> 
> It will be enough if "mbm_mode" is intended to be used for all current
> and future MBM modes/features but this series instead dedicates this file
> to just "assignable monitoring counters" feature. Doing so prevents us
> from, in the future, expanding this file to, for example, contain
> a new entry representing a new feature.
> 
>>
>> default            <-  Default mode is not related assignable features.
> 
> If not assignable features, what is it related to? "default" being the
> absence of assignable features still seems related to me.
> 
>>
>> I would think mbm_mode is fine. Let me know.
> 
> If this work is reworded that it is intended to support any MBM mode then
> it is fine, if this work remains to dedicate this file just to assignable
> features then I think its name should be changed.

Ok. Will change it to "mbm_assign_mode".

> 
> ...
> 
>>>>
>>>>     Flags can be one of the following:
>>>>
>>>>          t  MBM total event.
>>>>          l  MBM local event.
>>>>          tl Both total and local MBM events.
>>>>          _  None of the MBM events. Only works with '=' opcode.
>>>
>>> Please take care with the implementation that seems to support a 
>>> variety of
>>> combinations. If I understand correctly the implementation support flags
>>> like,
>>> for example, "tttt", "llll", "ltlt" ... those may not be an issue but 
>>> of most
>>> concern is, for example, a pattern like "_lt" that (unexpectedly) 
>>> appears to
>>> result in set of total and local.
>>
>> Yes. Should we not allow flag combinations with "_"?
>> I am not very sure about how to go about this.
>>
> 
> This topic seems to have moved to patch #22.

Yes. got it.
> 
> ...
> 
>>>>     # echo "legacy" > /sys/fs/resctrl/info/L3_MON/mbm_mode
>>>>     # cat /sys/fs/resctrl/info/L3_MON/mbm_mode
>>>>     mbm_cntr_assign
>>>>     [legacy]
>>>>
>>>> k. Unmount the resctrl
>>>>     #umount /sys/fs/resctrl/
>>>> ---
>>>> v6:
>>>>    We still need to finalize few interface details on mbm_mode and
>>>> mbm_control
>>>>    in case of ABMC and Soft-ABMC. We can continue the discussion with
>>>> this series.
>>>
>>> Could you please list the details that need to be finalized?
>>
>> 1. mbm_mode display
>>     # cat /sys/fs/resctrl/info/L3_MON/mbm_mode
>>       mbm_cntr_assign
>>       [legacy]
>>
>>      "mbm_cntr_assign"
>>       Are we sticking with ""mbm_cntr_assign" for ABMC?
>>       What should we name for soft-ABMC?
>>
>> 2. Also we had some concerns about Individual event assignment(ABMC)
>>     and group assignment(soft-ABMC)?
>>     Are the flags "t" and 'l' good for both these modes?
> 
> If I remember correctly the previous discussion ended with the need for
> "modes" that indicate to user space what to expect when interacting with 
> the
> MBM flags in the "mbm_control" file. The term used by ABMC should 
> reflect that
> each MBM flag/event can be set independently, while the term used by 
> soft-ABMC
> reflects that setting one flag/event makes the same change to the other
> MBM flag/event.
> 

Will add text in the resctrl.rst make it clear about it.
thanks

-- 
- Babu Moger