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

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Babu,

On 3/20/25 11:12 AM, Moger, Babu wrote:
> Hi Reinette,
> 
> On 3/19/25 13:36, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 3/14/25 9:18 AM, Moger, Babu wrote:
>>> On 3/13/2025 4:21 PM, Reinette Chatre wrote:
>>>> 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.
>>
>> I see. The previous dependency was thus just something enforced by OS to support the
>> chosen implementation? 
> 
> Yes. That is correct. We went that route mainly not to change the
> rmid_read operation.
> 
> With ABMC, we need to set Extended EVTID and ABMC bit in QM_EVTSEL
> register while reading the cntr_id events. Will add those patches in next
> version to make it clear.

Thank you.

> 
>> Looks like the two features share some registers.
>>
>>>
>>>> How would the existing BMEC implementation be impacted in this case?
>>>
>>> BMEC will only work with pre-ABMC(or default) mode.
>>
>> ok. Does this mean that if a user boots kernel with "rdt=!bmec" then ABMC will keep working?
> 
> Yes. That is correct.

Just to confirm and bring the two email threads together ... it sounds like the
expectation is that existing users of BMEC are expected to use mon_features to
know if mbm_{total,local}_bytes_config are supported. If system supports ABMC
then BMEC will not be available and thus mon_features will not contain
mbm_{total,local}_bytes_config. Existing users that rely on
mbm_{total,local}_bytes_config will experience failures and are expected
to switch to ABMC?


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


...

>>>>
>>>> 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/
>>
>> I am hesitant to respond to that message considering the corporate preamble that
>> sneaked in so I'll just add some thoughts here:
> 
> Yea. I noticed it later. Will take care next time.
> 
>>
>> Having the file higher in hierarchy does seem more useful. It may be useful to reduce
>> amount of parsing to get to needed information. Compare with below two examples that can
>> be used to convey the same information:
>>
>> # cat /sys/fs/resctrl/test/mon_L3_assignments
>> mbm_total_bytes: 0=unassigned; 1=unassigned
>> mbm_local_bytes: 0=unassigned; 1=unassigned
>>
>> #cat /sys/fs/resctrl/test/mon_L3_assignments
>> 0=_; 1=_
>>
>> We need to take care that it is always clear what "0" or "1" means ...
>> Tony has been mentioning a lot of interesting things about scope
>> changes. I assume the "L3" in mon_L3_assignments will dictate the scope?
> 
> I didnt think about the scope here. I was thinking of changing it to
> "mbm_assignments".

ah, I see, not a general "monitoring" file but specific to MBM. This still
may encounter difficulty if AMD does something like SNC where MBM could
be done per numa node. Perhaps we could constrain this even more with a
"mbm_L3_assignments". If anything ever shows up that need to do MBM
counter assignment at some other scope then at least we have the option
to create another file "mbm_?_assignments".

> 
>>
>> With a syntax like above the needed information can be presented in one line.
>> For example,
>>
>> #cat /sys/fs/resctrl/test/mon_L3_assignments
>> 0=mbm_total_bytes; 1=mbm_local_bytes
>>
>> The caveat is that is only for assigned counters, not shared, so this needs
>> to change.
>>
>> To support shared assignment ...  I wonder if it will be useful to users to
>> get the information on which other monitor groups the counter is shared _with_?
>>
>> Some examples:
>>
>> a) Just indicate whether a counter is shared or dedicated. (Introduce flags).
>> #cat /sys/fs/resctrl/test/mon_L3_assignments
>> 0=mbm_total_bytes:s; 1=mbm_local_bytes:d
>>
>> b) Indicate which groups a counter is shared with:
>> #cat /sys/fs/resctrl/testA/mon_L3_assignments
>> 0=mbm_total_bytes:s(testB); 1=mbm_local_bytes:d(not needed but perhaps empty for consistent interface?)
>> #cat /sys/fs/resctrl/testB/mon_L3_assignments
>> 0=mbm_total_bytes:s(testA); 1=mbm_local_bytes:d(?)
> 
> This format does not tell what is going on with all other domains. We need
> to display all the domains. I think that is important because users need
> to know what to expect reading the events on specific domains.
> 
> I think we need to convey all the following information to the user.
> 
> 1. Event Configuation: What is event configuration applied here?
> 
> 2. Domains: To which all the domains the configaration is applied?
>    This is useful in multi-domain configuration.
>    We also need to know if which domains are assigned or unassigned.
> 
> 3. Type of assignment: Exclusive(e or d) or Shared(s) or Unassigned(_)
> 
> 4. Finally: Where to read the events?
>  This is important when we add "mkdir" support in the future,
> mon_data/mon_l3_*/config_name will be created.
> 
> 
> With that in mind this might be helpful.
> 
> # cat /sys/fs/resctrl/test/mon_L3_assignments
> mbm_total_bytes: 0=e; 1=_
> mbm_local_bytes: 0=_; 1=s
> 
> This format tells the user all the information.
> mbm_total_bytes and mbm_local_bytes configurations are applied and
> configuration are coming from counter_configs.
> 
> User can read the events in
> mon_data/mon_L3_*/mbm_total_bytes
> mon_data/mon_L3_*/mbm_local_bytes
> 
> mbm_total_bytes is assigned on domain 0 and not on domain 1.
> Reading the mbm_total_bytes on domain 1 will report "unassigned".
> 
> mbm_local_bytes is not assigned on domain 0 and assigned on domain 1.
> Reading the mbm_local_bytes on domain 0 will report "unassigned".

Thank you very much for spelling it out. Much appreciated. This looks good to me.
Please include your list of requirements for interface in the cover-letter and/or
patch that introduces the interface.

> 
> I dont have much information on shared assignment at this point. Dont know
> if we can display shared group.

The proposed interface accommodates shared counters. The expectation is that
users can keep track themselves and if not, then the information can be
obtained with a read of every group's counter assignment. The issue here is
that this may worst case need a large number of file operations if expectation
is that it will still be possible to create num RMID monitoring groups.

Using files inside monitor group for this information may actually not be ideal.
If this information is needed then we could perhaps add a new file. For
example:
/sys/fs/resctrl/info/L3_MON/counter_configs/mbm_total_bytes/<file reporting which monitor groups share this counter configuration in different domains>
Of course, I do not know if this will be required and this seems manageable as
a later enhancement if needed.

> 
>>
>> ... (b) may just be overkill and we should instead follow Tony's
>> guideline (see https://lore.kernel.org/lkml/Z9CiwLrhuTODruCj@agluck-desk3/ )
>> that users should be able to keep track themselves.
>>
>> Reinette
>>
> 

Reinette