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

[Moger, Babu]

Hi Reinette,

On 3/13/25 11:08, Reinette Chatre wrote:
> Hi Babu,
> 
> On 3/12/25 11:14 AM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 3/12/25 12:14, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 3/12/25 9:03 AM, Moger, Babu wrote:
>>>> Hi Reinette,
>>>>
>>>> On 3/12/25 10:07, Reinette Chatre wrote:
>>>>> Hi Babu,
>>>>>
..

>>>>>> We can add the mkdir support later. That way we can provide basic ABMC
>>>>>> support without too much code complexity with mkdir support.
>>>>>
>>>>> This is not clear to me how you envision the "first phase". Is it what you
>>>>> proposed above, for example:
>>>>>       #echo "LclFill, LclNTWr, RmtFill" >
>>>>>          /sys/fs/resctrl/info/L3_MON/counter_configs/mbm_local_bytes
>>>>>
>>>>> In above the counter configuration name is a file. 
>>>>
>>>> Yes. That is correct.
>>>>
>>>> There will be two configuration files by default when resctrl is mounted
>>>> when ABMC is enabled.
>>>> /sys/fs/resctrl/info/L3_MON/counter_configs/mbm_total_bytes
>>>> /sys/fs/resctrl/info/L3_MON/counter_configs/mbm_local_bytes
>>>>
>>>>>
>>>>> How could mkdir support be added to this later if there are already files present?
>>>>
>>>> We already have these directories when resctrl is mounted.
>>>> /sys/fs/resctrl/test/mon_data/mon_L3_00/mbm_total_bytes
>>>> /sys/fs/resctrl/test/mon_data/mon_L3_00/mbm_local_bytes
>>>> /sys/fs/resctrl/test/mon_data/mon_L3_01/mbm_total_bytes
>>>> /sys/fs/resctrl/test/mon_data/mon_L3_01/mbm_local_bytes
>>>>
>>>> We dont need "mkdir" support for default  configurations.
>>>
>>> I was referring to the "mkdir" support for additional configurations that
>>> I understood you are thinking about adding later. For example,
>>> (copied from Peter's message
>>> https://lore.kernel.org/lkml/CALPaoCiii0vXOF06mfV=kVLBzhfNo0SFqt4kQGwGSGVUqvr2Dg@xxxxxxxxxxxxxx/):
>>>
>>>
>>>  # mkdir info/L3_MON/counter_configs/mbm_local_bytes
>>>  # 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
>>>  # cat info/L3_MON/counter_configs/mbm_local_bytes/event_filter
>>> LclFill
>>> LclNTWr
>>> LclSlowFill
>>>
>>> Any "later" work needs to be backward compatible with the first phase.
>>
>> Actually, we dont need extra file "event_filter".
>> This was discussed here.
>> https://lore.kernel.org/lkml/CALPaoChLL8p49eANYgQ0dJiFs7G=223fGae+LJyx3DwEhNeR8A@xxxxxxxxxxxxxx/
> 
> I undestand from that exchange that it is possible to read/write from
> an *existing* kernfs file but it is not obvious to me how that file is
> planned to be created.

My bad.. I misspoke here. We need "event_filter" file under each
configuration.


> 
> My understanding of the motivation behind support for "mkdir" is to enable
> user space to create custom counter configurations.
> 

That is correct.

> I understand that ABMC support aims to start with existing mbm_total_bytes/mbm_local_bytes
> configurations but I believe the consensus is that custom configurations need
> to be supported in the future.
> If resctrl starts with support where counter configuration as
> managed with a *file*, for example:
> /sys/fs/resctrl/info/L3_MON/counter_configs/mbm_total_bytes
> how will user space create future custom configurations?
> As I understand that is only possible with mkdir.
> 
>>
>> # echo LclFill > info/L3_MON/counter_configs/mbm_local_bytes
>> # echo LclNTWr > info/L3_MON/counter_configs/mbm_local_bytes
>> # echo LclSlowFill > info/L3_MON/counter_configs/mbm_local_bytes
>> # cat info/L3_MON/counter_configs/mbm_local_bytes
>>  LclFill
>>  LclNTWr
>>  LclSlowFill
>>
>> In the future, we can add mkdir support.
>>
>> # mkdir info/L3_MON/counter_configs/mbm_read_only
> 
> This is exactly my concern. resctrl should not start with a user space where
> a counter configuration is a file (mbm_local_bytes/mbm_total_bytes) and then
> switch user space interface to have counter configuration be done with
> directories.
> 
>> # echo LclFill > info/L3_MON/counter_configs/mbm_read_only
>> # cat info/L3_MON/counter_configs/mbm_read_only
>>   LclFill
> 
> ... wait ... user space writes to the directory?
> 

My bad. This is wrong. Let me rewrite the steps below.

> 
> 
>>
>> #echo mbm_read_only > test/mon_data/mon_L3_00/assign_exclusive
>>
>> Which would result in the creation of test/mon_data/mon_L3_*/mbm_read_only
>>
>> So, there is not breakage of backword compatibility.
> 
> The way I understand it I am seeing many incompatibilities. Perhaps I am missing
> something. Could you please provide detailed steps of how first phase and
> second phase would look?

No. You didn't miss anything. I misspoke on few steps.

Here are the steps. Just copying steps from Peters proposal.
https://lore.kernel.org/lkml/CALPaoCiii0vXOF06mfV=kVLBzhfNo0SFqt4kQGwGSGVUqvr2Dg@xxxxxxxxxxxxxx/


1. Mount the resctrl
   mount -t resctrl resctrl /sys/fs/resctrl

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

  # 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

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.

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

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

-- 
Thanks
Babu Moger