Message-ID: <7f54f9aa-1102-49ed-b830-6facb6f48366@intel.com>
Date: Thu, 13 Mar 2025 14:21:52 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: <babu.moger@....com>, "Moger, Babu" <bmoger@....com>, "Luck, Tony"
	<tony.luck@...el.com>
CC: Peter Newman <peternewman@...gle.com>, Dave Martin <Dave.Martin@....com>,
	<corbet@....net>, <tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>,
	<dave.hansen@...ux.intel.com>, <x86@...nel.org>, <hpa@...or.com>,
	<paulmck@...nel.org>, <akpm@...ux-foundation.org>, <thuth@...hat.com>,
	<rostedt@...dmis.org>, <xiongwei.song@...driver.com>,
	<pawan.kumar.gupta@...ux.intel.com>, <daniel.sneddon@...ux.intel.com>,
	<jpoimboe@...nel.org>, <perry.yuan@....com>, <sandipan.das@....com>,
	<kai.huang@...el.com>, <xiaoyao.li@...el.com>, <seanjc@...gle.com>,
	<xin3.li@...el.com>, <andrew.cooper3@...rix.com>, <ebiggers@...gle.com>,
	<mario.limonciello@....com>, <james.morse@....com>,
	<tan.shaopeng@...itsu.com>, <linux-doc@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <maciej.wieczor-retman@...el.com>,
	<eranian@...gle.com>
Subject: Re: [PATCH v11 00/23] x86/resctrl : Support AMD Assignable Bandwidth
 Monitoring Counters (ABMC)

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@mail.gmail.com/

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.

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

As alternative, we can try to work the previous mbm_assign_control syntax in here (use + and -).

For example:

# cat info/L3_MON/counter_configs/mbm_local_bytes/event_filter
LclNTWr
# echo "+LclFill,-LclNTWr,+LclSlowFill" > info/L3_MON/counter_configs/mbm_local_bytes/event_filter
# cat info/L3_MON/counter_configs/mbm_local_bytes/event_filter
LclFill,LclSlowFill

With something like above resctrl just deals with file writes as before.


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

Could you please elaborate what your concern is? You mention that it is
excessive but it is not clear to me what issues may arise by
having three files instead of one.

I do think, and Peter also mentioned [1] this, that it may be useful,
to "put a group/resource-scoped assign_* file higher in the hierarchy
to make it easier for users who want to configure all domains the
same for a group."

Placing *additional* files higher in hierarchy (used to manage counters in all
domains) may be more useful that trying to provide the shared/exclusive/unassign
in one file per domain.

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

Got it!

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

Related to comment from Tony [2] about rmdir, please also consider that
original mbm_local_bytes/mbm_total_bytes could also be removed because at this
point they should not appear different from other counter configurations ... apart
from being pre-populated for backward compatibility.

Thank you.

Reinette


[1] https://lore.kernel.org/lkml/CALPaoCiii0vXOF06mfV=kVLBzhfNo0SFqt4kQGwGSGVUqvr2Dg@mail.gmail.com/
[2] https://lore.kernel.org/lkml/Z9NB0wd8ZewLjNAd@agluck-desk3/

Powered by blists - more mailing lists