[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <c840cb69-41fe-49a8-a7a8-d75f68e1d84c@amd.com>
Date: Fri, 14 Mar 2025 11:18:33 -0500
From: "Moger, Babu" <bmoger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>, babu.moger@....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 Reinette,
On 3/13/2025 4:21 PM, Reinette Chatre 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@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.
Yes. ABMC will be enabled by default. ABMC will use the configurations
from info/L3_MON/counter_configs. ABMC will not depend on BMEC.
> How would the existing BMEC implementation be impacted in this case?
BMEC will only work with pre-ABMC(or default) mode.
>
> 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.
yes.
>
> To follow that, we should also consider how "mon_features" will change with this
> implementation.
May be
# cat /sys/fs/resctrl/info/L3_MON/mon_features
llc_occupancy
mbm_total_bytes
mbm_local_bytes
counter_configs/mbm_total_bytes/event_filter
counter_configs/mbm_local_bytes/event_filter
>
>>
>> 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
Yes. We should support that.
>
> (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".
Just searching quickly, I have not seen any append operations on kernfs.
> 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.
Or without complicating much we can just support basic operations.
# cat info/L3_MON/counter_configs/mbm_local_bytes/event_filter
LclFill, LclNTWr, LclSlowFill
# echo "LclFill, LclNTWr, LclSlowFill, VictimBW" >
info/L3_MON/counter_configs/mbm_local_bytes/event_filter
# cat info/L3_MON/counter_configs/mbm_local_bytes/event_filter
LclFill, LclNTWr, LclSlowFill, VictimBW
# echo "LclFill, LclNTWr" >
info/L3_MON/counter_configs/mbm_local_bytes/event_filter
# cat info/L3_MON/counter_configs/mbm_local_bytes/event_filter
LclFill, LclNTWr
>
>
>>
>> 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.
All these 3 properties are mutually exclusive. Only one can true at a
time. Example:
#cat assign_exclusive
0
#cat assign_shared
0
#cat uassigned
1
Three operations to find out the assign state.
Instead of that
#cat mon_l3_assignments
unassigned
>
> 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.
Yea. To make it better we can add "mon_l3_assignments" in groups main
directory. We can do all the operation in just one file.
https://lore.kernel.org/lkml/efb5293f-b0ef-4c94-bf10-9ca7ebb3b53f@amd.com/
>
>>
>> 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.
Sure.
>
> Thank you.
>
> Reinette
>
>
> [1] https://lore.kernel.org/lkml/CALPaoCiii0vXOF06mfV=kVLBzhfNo0SFqt4kQGwGSGVUqvr2Dg@mail.gmail.com/
> [2] https://lore.kernel.org/lkml/Z9NB0wd8ZewLjNAd@agluck-desk3/
>
Thanks
Babu
Powered by blists - more mailing lists