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

[Dave Martin]

Hi Reinette,

On Tue, Feb 18, 2025 at 01:29:09PM -0800, Reinette Chatre wrote:
> Hi Babu,
> 
> On 2/18/25 11:32 AM, Moger, Babu wrote:
> > Hi Reinette,
> > 
> > On 2/18/25 12:14, Reinette Chatre wrote:
> >> Hi Babu,
> >>
> >> On 2/18/25 7:39 AM, Moger, Babu wrote:
> >>  
> >>> 3. Use the actual events instead of flags based on the below comment.
> >>>
> >>> https://lore.kernel.org/lkml/a07fca4c-c8fa-41a6-b126-59815b9a58f9@xxxxxxxxx/
> >>>
> >>>  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.
> >>>
> >>>  Reinette, What do you think?
> >>
> >> I was actually expecting some push back or at least discussion on this interface
> >> because the braces seem difficult to parse when compared to, for example, using
> > 
> > I am yet to work on it. Will work on it after confirmation.
> > 
> > Here is the output from a system with 12 domains. I created one "test" group.
> > 
> > Output is definitely harder to parse for human eyes.
> > 
> > #cat info/L3_MON/mbm_assign_control
> > test//0={mbm_total_bytes}{mbm_local_bytes};1={mbm_total_bytes}{mbm_local_bytes};2={mbm_total_bytes}{mbm_local_bytes};3={mbm_total_bytes}{mbm_local_bytes};4={mbm_total_bytes}{mbm_local_bytes};5={mbm_total_bytes}{mbm_local_bytes};6={mbm_total_bytes}{mbm_local_bytes};7={mbm_total_bytes}{mbm_local_bytes};8={mbm_total_bytes}{mbm_local_bytes};9={mbm_total_bytes}{mbm_local_bytes};10={mbm_total_bytes}{mbm_local_bytes};11={mbm_total_bytes}{mbm_local_bytes}
> > //0={mbm_total_bytes}{mbm_local_bytes};1={mbm_total_bytes}{mbm_local_bytes};2={mbm_total_bytes}{mbm_local_bytes};3={mbm_total_bytes}{mbm_local_bytes};4={mbm_total_bytes}{mbm_local_bytes};5={mbm_total_bytes}{mbm_local_bytes};6={mbm_total_bytes}{mbm_local_bytes};7={mbm_total_bytes}{mbm_local_bytes};8={mbm_total_bytes}{mbm_local_bytes};9={mbm_total_bytes}{mbm_local_bytes};10={mbm_total_bytes}{mbm_local_bytes};11={mbm_total_bytes}{mbm_local_bytes}
> > 
> > It is harder to parse in code also. We should consider only if there is a
> > value-add with this format.
> 
> Please see my comments in [2] for some motivations.
> 
> > 
> > Otherwise I prefer our current flag format.
> > 
> > # cat info/L3_MON/mbm_assign_control
> > test//0=tl;1=tl;2=tl;3=tl;4=tl;5=tl;6=tl;7=tl;8=tl;9=tl;10=tl;11=tl
> > //0=tl;1=tl;2=tl;3=tl;4=tl;5=tl;6=tl;7=tl;8=tl;9=tl;10=tl;11=tl
> 
> We could possibly consider some middle ground where flags are separated by
> commas and when the amount of used flags reach 26 the interface can use
> "two letter flags" or "longer names" or "the actual event name" or ....
> 
> > 
> > 
> >> commas to separate the events of a domain. Peter [1] has some reservations about
> > 
> > Yes. I would like to hear from Peter.
> > 
> 
> Reinette

Ack; see also my reply to Peter on the other subthread.

I think the single-letter names provide a much less cumbersome
interface.

>From the Arm side, I'd be happy to see just "t" and "l" for now, with
their current fixed mappings to event names, provided that we are
confident that we can add flexibility later without breaking the ABI.

In case this has got lost in the noise, I still think that the v11
proposal for the ABMC interface looks fine as a first step -- I just
wanted to kick the tires re extensibility.

Cheers
---Dave