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

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

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.

...

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

...

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

Reinette