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

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

Hi Reinette,

On 3/13/2025 4:21 PM, Reinette Chatre 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@mail.gmail.com/
> 
> 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.

Yes. ABMC will be enabled by default. ABMC will use the configurations 
from info/L3_MON/counter_configs. ABMC will not depend on BMEC.

> How would the existing BMEC implementation be impacted in this case?

BMEC will only work with pre-ABMC(or default) mode.


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

yes.

> 
> To follow that, we should also consider how "mon_features" will change with this
> implementation.

May be

# cat /sys/fs/resctrl/info/L3_MON/mon_features
  llc_occupancy
  mbm_total_bytes
  mbm_local_bytes
  counter_configs/mbm_total_bytes/event_filter
  counter_configs/mbm_local_bytes/event_filter

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

Yes. We should support that.

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

Just searching quickly, I have not seen any append operations on kernfs.


> As alternative, we can try to work the previous mbm_assign_control syntax in here (use + and -).
> 
> For example:
> 
> # cat info/L3_MON/counter_configs/mbm_local_bytes/event_filter
> LclNTWr
> # echo "+LclFill,-LclNTWr,+LclSlowFill" > info/L3_MON/counter_configs/mbm_local_bytes/event_filter
> # cat info/L3_MON/counter_configs/mbm_local_bytes/event_filter
> LclFill,LclSlowFill
> 
> With something like above resctrl just deals with file writes as before.

Or without complicating much we can just support basic operations.

# cat info/L3_MON/counter_configs/mbm_local_bytes/event_filter
   LclFill, LclNTWr, LclSlowFill

# echo "LclFill, LclNTWr, LclSlowFill, VictimBW" > 
info/L3_MON/counter_configs/mbm_local_bytes/event_filter

# cat info/L3_MON/counter_configs/mbm_local_bytes/event_filter
   LclFill, LclNTWr, LclSlowFill, VictimBW

# echo "LclFill, LclNTWr" > 
info/L3_MON/counter_configs/mbm_local_bytes/event_filter

# cat info/L3_MON/counter_configs/mbm_local_bytes/event_filter
   LclFill, LclNTWr

> 
> 
>>
>> 4. As usual the events can be read from the mon_data directories.
>>     #mkdir /sys/fs/resctrl/test
>>     #cd   /sys/fs/resctr/test
>>     #cat  test/mon_data/mon_data/mon_L3_00/mbm_tota_bytes
>>     101010
>>     #cat   test/mon_data/mon_data/mon_L3_00/mbm_local_bytes
>>     32323
>>
>> 5. There will be 3 files created in each group's mon_data directory when
>> ABMC is supported.
>>
>>     a. test/mon_data/mon_L3_00/assign_exclusive
>>     b. test/mon_data/mon_L3_00/assign_shared
>>     c. test/mon_data/mon_L3_00/unassign
>>
>>
>> 6. Events can be assigned/unassigned by these commands
>>
>>   # echo mbm_total_bytes > test/mon_data/mon_L3_00/assign_exclusive
>>   # echo mbm_local_bytes > test/mon_data/mon_L3_01/assign_exclusive
>>   # echo mbm_local_bytes > test/mon_data/mon_L3_01/unassign
>>
>>
>> Note:
>> I feel 3 files are excessive here. We can probably achieve everything in
>> just one file.
> 
> Could you please elaborate what your concern is? You mention that it is
> excessive but it is not clear to me what issues may arise by
> having three files instead of one.

All these 3 properties are mutually exclusive. Only one can true at a 
time. Example:
#cat assign_exclusive
0
#cat assign_shared
0
#cat uassigned
1

Three operations to find out the assign state.

Instead of that
#cat mon_l3_assignments
unassigned


> 
> I do think, and Peter also mentioned [1] this, that it may be useful,
> to "put a group/resource-scoped assign_* file higher in the hierarchy
> to make it easier for users who want to configure all domains the
> same for a group."
> 
> Placing *additional* files higher in hierarchy (used to manage counters in all
> domains) may be more useful that trying to provide the shared/exclusive/unassign
> in one file per domain.

Yea. To make it better we can add "mon_l3_assignments" in groups main 
directory. We can do all the operation in just one file.

https://lore.kernel.org/lkml/efb5293f-b0ef-4c94-bf10-9ca7ebb3b53f@amd.com/


> 
>>
>> Not sure about mbm_assign_control interface as there are concerns with
>> group listing holding the lock for long.
>>
>> -----------------------------------------------------------------------
>> Second phase, we can add support for "mkdir"
>>
>> 1. mkdir info/L3_MON/counter_configs/mbm_read_only
>>
>> 2. mkdir option will create "event_filter" file.
>>     info/L3_MON/counter_configs/mbm_read_only/event_filter
>>
> 
> Got it!
> 
>> 3. Users can modify event configuration.
>>     echo LclFill > info/L3_MON/counter_configs/mbm_read_only/event_filter
>>
>> 4. Users can assign the events
>>
>>    echo mbm_read_only > test/mon_data/mon_L3_00/assign_exclusive
>>
>> 5. Events can be read in
>>
>>     test/mon_data/mon_data/mon_L3_00/mbm_read_only
>>
> 
> Related to comment from Tony [2] about rmdir, please also consider that
> original mbm_local_bytes/mbm_total_bytes could also be removed because at this
> point they should not appear different from other counter configurations ... apart
> from being pre-populated for backward compatibility.

Sure.

> 
> Thank you.
> 
> Reinette
> 
> 
> [1] https://lore.kernel.org/lkml/CALPaoCiii0vXOF06mfV=kVLBzhfNo0SFqt4kQGwGSGVUqvr2Dg@mail.gmail.com/
> [2] https://lore.kernel.org/lkml/Z9NB0wd8ZewLjNAd@agluck-desk3/
> 

Thanks
Babu