Message-ID: <8babbd2a-50ae-4a18-8e48-5421539ef0e6@amd.com>
Date: Mon, 17 Mar 2025 18:00:52 -0500
From: "Moger, Babu" <bmoger@....com>
To: Peter Newman <peternewman@...gle.com>,
 Reinette Chatre <reinette.chatre@...el.com>
Cc: babu.moger@....com, "Luck, Tony" <tony.luck@...el.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 Peter,

On 3/17/2025 11:27 AM, Peter Newman wrote:
> Hi Reinette,
> 
> On Thu, Mar 13, 2025 at 10:22 PM Reinette Chatre
> <reinette.chatre@...el.com> 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.
>>
>> 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.

There is no opt-in mode. ABMC will be enabled by default if supported.
Users will have option to go back to legacy mode.

The default configurations will be used for total(0x7f equivalent to 
enable all) and local(0x15 equivalent to all local events).

Same thing will show up at
a. info/L3_MON/counter_configs/mbm_total_bytes/event_filter
b. info/L3_MON/counter_configs/mbm_local_bytes/event_filter

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

Powered by blists - more mailing lists