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

[Peter Newman]

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

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.

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.

-Peter