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

["Moger, Babu" <bmoger@amd.com>]

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