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

[Reinette Chatre]

Hi Babu and Peter,

On 3/17/25 4:00 PM, Moger, Babu wrote:
> Hi Peter,
> 
> On 3/17/2025 11:27 AM, Peter Newman wrote:
>> Hi Reinette,
>>
>> On Thu, Mar 13, 2025 at 10:22 PM Reinette Chatre
>> <reinette.chatre@xxxxxxxxx> wrote:
>>>
>>> Hi Babu,
>>>
>>> On 3/13/25 1:13 PM, Moger, Babu wrote:
>>>> On 3/13/25 11:08, Reinette Chatre wrote:
>>>>> On 3/12/25 11:14 AM, Moger, Babu wrote:
>>>>>> On 3/12/25 12:14, Reinette Chatre wrote:
>>>>>>> On 3/12/25 9:03 AM, Moger, Babu wrote:
>>>>>>>> On 3/12/25 10:07, Reinette Chatre wrote:
>>>
>>>
>>>> Here are the steps. Just copying steps from Peters proposal.
>>>> https://lore.kernel.org/lkml/CALPaoCiii0vXOF06mfV=kVLBzhfNo0SFqt4kQGwGSGVUqvr2Dg@xxxxxxxxxxxxxx/
>>>
>>> Thank you very much for detailing the steps. It is starting the fall into place
>>> for me.
>>>
>>>>
>>>>
>>>> 1. Mount the resctrl
>>>>     mount -t resctrl resctrl /sys/fs/resctrl
>>>
>>> I assume that on ABMC system the plan remains to have ABMC enabled by default, which
>>> will continue to depend on BMEC.
>>>
>>> How would the k existing BMEC implementation be impacted in this case?
>>>
>>> Without any changes to BMEC support the mbm_total_bytes_config and mbm_local_bytes_config
>>> files will remain and user space may continue to use them to change the event
>>> configurations with confusing expecations/results on an ABMC system.
>>>
>>> One possibility may be that a user may see below on ABMC system even if BMEC is supported:
>>> # cat /sys/fs/resctrl/info/L3_MON/mon_features
>>> llc_occupancy
>>> mbm_total_bytes
>>> mbm_local_bytes
>>>
>>> With the above a user cannot be expected to want to interact with mbm_total_bytes_config
>>> and mbm_local_bytes_config, which may be the simplest to do.
>>
>> How about making mbm_local_bytes and mbm_total_bytes always be
>> configured using mbm_{local,total}_bytes_config and only allowing the
>> full ABMC configurability on user-defined configurations. This could
>> resolve the issue of backwards compatibility with the BMEC files and
>> remove the need for the user opting-in to ABMC mode.

hmmm, yes, backward compatibility is a big issue with an earlier suggestion
from me. Users with scripts/tools using mbm_{local,total}_bytes_config
would expect that to continue to work on systems that support BMEC.
resctrl could continue to use mbm_{local,total}_bytes_config
even though the inconsistent interface is not ideal

> 
> There is no opt-in mode. ABMC will be enabled by default if supported.
> Users will have option to go back to legacy mode.

I assume there will still be the opt-in for automatic counter assignment
on monitor group creation (mkdir)?

> 
> The default configurations will be used for total(0x7f equivalent to enable all) and local(0x15 equivalent to all local events).
> 
> Same thing will show up at
> a. info/L3_MON/counter_configs/mbm_total_bytes/event_filter
> b. info/L3_MON/counter_configs/mbm_local_bytes/event_filter

These files could possibly be read-only but the moment user space uses
mbm_{local,total}_bytes_config to change the configurations between domains
this will be invalid. In this case the file could also perhaps
read "Configured using <path to>mbm_{local,total}_bytes_config". It is
not clear to me what would be most intuitive to user space.

> 
>>
>> It will be less clean implementation-wise, since there will be two
>> classes of event configuration to deal with, but I think it seems
>> logical from the user's side.
>>
>>>
>>> To follow that, we should also consider how "mon_features" will change with this
>>> implementation.
>>>
>>>>
>>>> 2. When ABMC is supported two default configurations will be created.
>>>>
>>>>    a. info/L3_MON/counter_configs/mbm_total_bytes/event_filter
>>>>    b. info/L3_MON/counter_configs/mbm_local_bytes/event_filter
>>>>
>>>>    These files will be populated with default total and local events
>>>>    # cat info/L3_MON/counter_configs/mbm_total_bytes/event_filter
>>>>      VictimBW
>>>>      RmtSlowFill
>>>>      RmtNTWr
>>>>      RmtFill
>>>>      LclFill
>>>>      LclNTWr
>>>>      LclSlowFill
>>>
>>> Looks good. Here we could perhaps start nitpicking about naming and line separation.
>>> I think it may be easier if the fields are separated by comma, but more on that
>>> below ...
>>>
>>>>
>>>>    # cat info/L3_MON/counter_configs/mbm_local_bytes/event_filter
>>>>     LclFill,
>>>>     LclNTWr
>>>>     LclSlowFill
>>>>
>>>> 3. Users will have options to update the event configuration.
>>>>     echo LclFill > info/L3_MON/counter_configs/mbm_local_bytes/event_filter
>>>
>>> We need to be clear on how user space interacts with this file. For example,
>>> can user space "append" configurations? Specifically, if the file has
>>> contents like your earlier example:
>>> # cat info/L3_MON/counter_configs/mbm_local_bytes/event_filter
>>>   LclFill
>>>   LclNTWr
>>>   LclSlowFill
>>>
>>> Should above be created with (note "append" needed for second and third):
>>> echo LclFill > info/L3_MON/counter_configs/mbm_local_bytes/event_filter
>>> echo LclNTWr >> info/L3_MON/counter_configs/mbm_local_bytes/event_filter
>>> echo LclSlowFill >> info/L3_MON/counter_configs/mbm_local_bytes/event_filter
>>>
>>> Is it possible to set multiple configurations in one write like below?
>>> echo "LclFill,LclNTWr,LclSlowFill" > info/L3_MON/counter_configs/mbm_local_bytes/event_filter
>>>
>>> (note above where it may be easier for user space to use comma (or some other field separator)
>>> when providing multiple configurations at a time, with this, to match, having output in
>>> commas may be easier since it makes user interface copy&paste easier)
>>>
>>> If file has content like:
>>> # cat info/L3_MON/counter_configs/mbm_local_bytes/event_filter
>>>   LclNTWr
>>>   LclSlowFill
>>>
>>> What is impact of the following:
>>> echo LclFill > info/L3_MON/counter_configs/mbm_local_bytes/event_filter
>>>
>>> Is it (append):
>>> # cat info/L3_MON/counter_configs/mbm_local_bytes/event_filter
>>>   LclFill
>>>   LclNTWr
>>>   LclSlowFill
>>>
>>> or (overwrite):
>>> # cat info/L3_MON/counter_configs/mbm_local_bytes/event_filter
>>>   LclFill
>>>
>>> I do think the interface will be more intuitive it if follows regular file
>>> operations wrt "append" and such. I have not looked into how kernfs supports
>>> "append".
>>
>> I expect specifying counter_configs to be a rare or one-time
>> operation, so I think ease-of-use is the only concern. I think
>> multiple, appending writes is the most straightforward to implement
>> and invoke (for a shell user), but I think commas are easy enough to
>> support as well, even though it would look better when reading back to
>> see the entries on separate lines.

When the counter configuration consist out of multiple events then it may
be convenient to just write it all in one go and having a shell user use
newline as field separator does not seem convenient. Appending writes sound
good no matter the field separator.
Reading back we may have to consider both what looks good to user space and
what is easy to parse by a script.

>>
>> I believe you can inspect the file descriptor's flags from the
>> kernfs_open_file reference: of->file->f_flags & O_APPEND
>>
>> I haven't tried this, though.

Thanks for looking this up.

Reinette