Re: [PATCH v12 08/26] x86/resctrl: Introduce the interface to display monitor mode

Moger, Babu
2025-04-16T14:05:53Z

Hi Reinette,

On 4/15/2025 11:22 AM, Reinette Chatre wrote:
> Hi Babu,
>
> On 4/14/25 12:52 PM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 4/11/25 15:56, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 4/3/25 5:18 PM, Babu Moger wrote:
>
>>>> platforms. On x86, CONFIG_RESCTRL_ASSIGN_FIXED is not defined, whereas on
>>>> Arm64, it is. As a result, for MPAM, the file would be either:
>>>
>>> CONFIG_RESCTRL_ASSIGN_FIXED does not yet exist anywhere so this motivation needs
>>> to provide stronger support for why it is used before it exists. There is a precedent
>>> here with RESCTRL_RMID_DEPENDS_ON_CLOSID already used while it does not yet
>>> appear in a Kconfig file. I would propose that this is motivated by noting
>>> how it is already understood how Arm supports assignable counters this was recommended
>>> by James to prepare for that work. Since this is user interface this
>>> work is done early to ensure user interface is compatible with that upcoming
>>> support. Also set folks at ease that IS_ENABLED() works as expected with a
>>> non-existing config.
>>
>> How about this?
>>
>> Add IS_ENABLED(CONFIG_RESCTRL_ASSIGN_FIXED) check to support Arm64.
>>
>> On x86, CONFIG_RESCTRL_ASSIGN_FIXED is not defined. On Arm64, it will be
>> defined when the "mbm_cntr_assign" mode is supported.
>>
>> Add an IS_ENABLED(CONFIG_RESCTRL_ASSIGN_FIXED) check early to ensure the
>> user interface remains compatible with upcoming Arm64 support.
>> IS_ENABLED() safely evaluates to 0 when the configuration is not defined.
>>
>> As a result, for MPAM, the file would be either:
>> [default]
>> or
>> [mbm_cntr_assign]
>>
>
> Sounds good to me.
>

Thanks

>>
>>>
>>>
>>>> [default]
>>>> or
>>>> [mbm_cntr_assign]
>>>>
>>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>>>> ---
>>>> v12: Minor text update in change log and user documentation.
>>>> Added the check CONFIG_RESCTRL_ASSIGN_FIXED to take care of arm platforms.
>>>> This will be defined only in arm and not in x86.
>>>>
>>>> v11: Renamed rdtgroup_mbm_assign_mode_show() to resctrl_mbm_assign_mode_show().
>>>> Removed few texts in resctrl.rst about AMD specific information.
>>>> Updated few texts.
>>>>
>>>> v10: Added few more text to user documentation clarify on the default mode.
>>>>
>>>> v9: Updated user documentation based on comments.
>>>>
>>>> v8: Commit message update.
>>>>
>>>> v7: Updated the descriptions/commit log in resctrl.rst to generic text.
>>>> Thanks to James and Reinette.
>>>> Rename mbm_mode to mbm_assign_mode.
>>>> Introduced mutex lock in rdtgroup_mbm_mode_show().
>>>>
>>>> v6: Added documentation for mbm_cntr_assign and legacy mode.
>>>> Moved mbm_mode fflags initialization to static initialization.
>>>>
>>>> v5: Changed interface name to mbm_mode.
>>>> It will be always available even if ABMC feature is not supported.
>>>> Added description in resctrl.rst about ABMC mode.
>>>> Fixed display abmc and legacy consistantly.
>>>>
>>>> v4: Fixed the checks for legacy and abmc mode. Default it ABMC.
>>>>
>>>> v3: New patch to display ABMC capability.
>>>>
>>>> ???END
>>>> ---
>>>> Documentation/arch/x86/resctrl.rst | 27 +++++++++++++++++++
>>>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 37 ++++++++++++++++++++++++++
>>>> 2 files changed, 64 insertions(+)
>>>>
>>>> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
>>>> index fb90f08e564e..bb96b44019fe 100644
>>>> --- a/Documentation/arch/x86/resctrl.rst
>>>> +++ b/Documentation/arch/x86/resctrl.rst
>>>> @@ -257,6 +257,33 @@ with the following files:
>>>> # cat /sys/fs/resctrl/info/L3_MON/mbm_local_bytes_config
>>>> 0=0x30;1=0x30;3=0x15;4=0x15
>>>>
>>>> +"mbm_assign_mode":
>>>> + Reports the list of monitoring modes supported. The enclosed brackets
>>>> + indicate which mode is enabled.
>>>> + ::
>>>> +
>>>> + # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode
>>>> + [mbm_cntr_assign]
>>>> + default
>>>> +
>>>> + "mbm_cntr_assign":
>>>> +
>>>> + In mbm_cntr_assign mode, a monitoring event can only accumulate data
>>>> + while it is backed by a hardware counter. The user-space is able to
>>>> + specify which of the events in CTRL_MON or MON groups should have a
>>>> + counter assigned using the "mbm_assign_control" file. The number of
>>>
>>> "mbm_assign_control" no longer exist.
>>
>> The user-space is able to specify which of the events in CTRL_MON or MON
>> groups should have a counter assigned using the "mbm_L3_assignments"
>> interface file in each resctrl group.
>
> I think it can be assumed the reader represents the user space. If doing so
> this can be simplified like:
>
> Use "mbm_L3_assignments" found in each CTRL_MON and MON group to
> specify which of the events should have a counter assigned.
>

Sure.

>>
>>>
>>>> + counters available is described in the "num_mbm_cntrs" file. Changing
>>>> + the mode may cause all counters on the resource to reset.
>>>> +
>>>> + "default":
>>>> +
>>>> + In default mode, resctrl assumes there is a hardware counter for each
>>>> + event within every CTRL_MON and MON group. On AMD platforms, it is
>>>> + recommended to use the mbm_cntr_assign mode, if supported, to prevent
>>>> + the hardware from resetting counters between reads. This can result in
>>>
>>> "from resetting counters" -> "from re-allocating counters"?
>>
>> How about?
>>
>> "from resetting MBM events between reads"
>
> With more detail, how about:
>
> ", to prevent reset of MBM events between reads resulting from hardware re-allocating counters"?

Yes.

>
>>>> /*
>>>> @@ -1908,6 +1938,13 @@ static struct rftype res_common_files[] = {
>>>> .seq_show = mbm_local_bytes_config_show,
>>>> .write = mbm_local_bytes_config_write,
>>>> },
>>>> + {
>>>> + .name = "mbm_assign_mode",
>>>> + .mode = 0444,
>>>> + .kf_ops = &rdtgroup_kf_single_ops,
>>>> + .seq_show = resctrl_mbm_assign_mode_show,
>>>> + .fflags = RFTYPE_MON_INFO,
>>>
>>> Needs a RFTYPE_RES_CACHE?
>>
>> I am not very sure about this. This flag is added to the files in info/L3.
>>
>> "mbm_assign_mode" goes in info/L3_MON/
>>
>> The files in L3_MON does not have these flags set (for example
>> mon_features, num_rmids).
>>
>
> My assumption is that mon_features and num_rmids are generic monitoring
> files that should be supported by all resources that support monitoring. When
> resctrl starts to handle resource specific information then it should be
> clear what type or resource it applies to. I understand that this may not
> seem obvious since resctrl only supports monitoring on L3 resource.
>
> Another view, consider existing code in resctrl_mon_resource_init() where
> the MBM configuration files are made specific to RFTYPE_RES_CACHE. I see
> mbm_assign_mode to be very similar to these.
>

Ok. Sure. Will do.

thanks
Babu