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

[Dave Martin <Dave.Martin@arm.com>]

Hi there,

On Tue, Feb 18, 2025 at 09:39:43AM -0600, Moger, Babu wrote:
> Hi All,
> 
> On 2/18/25 06:30, Dave Martin wrote:
> > On Mon, Feb 17, 2025 at 10:45:29AM -0600, Moger, Babu wrote:
> >> Hi All,

[...]

> >> One thing I am not clear why MPAM implementation plans to create separate
> >> files(dynamically) in /sys/fs/resctrl/info/L3_MON/ directory to read the
> >> events. We already have files in each group to read the events.
> >>
> >> # ls -l /sys/fs/resctrl/mon_data/mon_L3_00/
> >> total 0
> >> -r--r--r--. 1 root root 0 Feb 17 08:16 llc_occupancy
> >> -r--r--r--. 1 root root 0 Feb 17 08:16 mbm_local_bytes
> >> -r--r--r--. 1 root root 0 Feb 17 08:16 mbm_total_bytes
> > 
> > 
> > To be clear, we have no current plan to do this from the Arm side.
> > 
> > My sketch was just a thought experiment to test whether we would have
> > difficulties _if_ a decision were made to extend the interface in that
> > direction.
> > 
> > But it looks OK to me: the interface proposed in this series seems to
> > leave enough possibilities for extension open that we could do
> > something like what I described later in if we decide to.
> > 
> > 
> > Overall, the interface proposed in this series seems a reasonable way
> > to support ABMC systems while keeping the consumer-side interface
> > (i.e., reading the mbm_total_bytes files etc.) as similar to the
> > classic / Intel RDT situation as possible.
> > 
> > MPAM can fit in with this approach, as demonstrated by James' past
> > branches porting the MPAM driver on top of previous versions of the
> > ABMC series.
> 
> Thanks Dave.
> > 
> > As I understand it, he's almost done with porting onto this v11,
> > with no significant issues.
> > 
> Good to know. Thanks
> 
> I am working on v12 of ABMC with few changes from Reinette's earlier
> review comments.
> 
> Most of the changes are related to commit message update and user
> documentation update.
> 
> Introduced couple of new functions resctrl_reset_rmid_all() and
> mbm_cntr_free_all() to organize the code better based on the comment.
> https://lore.kernel.org/lkml/b60b4f72-6245-46db-a126-428fb13b6310@intel.com/
> 
> 
> On top of that I have few comments from from Dave.
> 
> 1.  Change "mbm_cntr_assign" to "mbm_counter_assign".
> 
> This will require me to search and replace lot of places. There are
> variables, names like num_mbm_cntrs, mbm_cntr_assignable,
> resctrl_arch_mbm_cntr_assign_enabled, resctrl_arch_mbm_cntr_assign_set,
> mbm_cntr_assign_enabled, resctrl_num_mbm_cntrs_show, mbm_cntr_cfg and list
> goes on.
> 
>  This is mostly cosmetic and not much value add. Will drop this change if
> Dave has no objections.

There is no need to change the names of kernel symbols -- this was just
about the interface presented to userspace.

So, if you rename only the affect file names in resctrlfs (I think
there weren't any others) then I'm happy with that.

But if you prefer to avoid this inconsistency, the file name can stay
as-is.  It's not a huge deal.


> 2. Change /sys/fs/resctrl/info/L3_MON/num_mbm_cntrs to display per-domain
> supported counters instead of a single value.

Ack; thanks (we could always add it back in later without an ABI break,
if people feel strongly about it and it looks feasible).


> 3. Use the actual events instead of flags based on the below comment.
> 
> https://lore.kernel.org/lkml/a07fca4c-c8fa-41a6-b126-59815b9a58f9@intel.com/
> 
>  Something like this.
>  # echo '//0={mbm_total_bytes}{mbm_local_bytes};1={mbm_local_bytes}'
> >/sys/fs/resctrl/info/L3_MON/mbm_assign_control
> 
>  Are we ready to go with this approach? I am still not clear on this.

[...]

> -- 
> Thanks
> Babu Moger

On this point, I'll defer to discussions elsewhere on the thread.


I have a few other minor comments pending to post, but it looks like
there may be a more serious issue with how the mbm_assign_control file
is handled in the kernel -- I'll try to post comments on that today.

Cheers
---Dave