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

["Luck, Tony" <tony.luck@intel.com>]

On Thu, Mar 13, 2025 at 03:13:32PM -0500, Moger, Babu wrote:
> 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@mail.gmail.com/):
> >>>
> >>>
> >>>  # 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@mail.gmail.com/
> > 
> > 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@mail.gmail.com/
> 
> 
> 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

Once the "mkdir" support described below is implemented users will not
need to redefine these legacy event file names. That makes me happy.

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

Maybe the one file could look like:

# cat mon_L3_assignments
mbm_total_bytes: exclusive
mbm_local_bytes: shared
mbm_read_only: unassigned

with new lines appearing when mkdir creates new events, and the obvious
write semantics:

# echo "mbm_total_bytes: unassigned" > mon_L3_assignments

to make updates.

> 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

Is there a matching "rmdir" to make this go away again?

> -- 
> Thanks
> Babu Moger