Re: [PATCH v7 08/24] x86/resctrl: Introduce interface to display number of monitoring counters

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

Hi Babu,

On 9/4/24 3:21 PM, Babu Moger wrote:
> The mbm_assign_cntr mode provides an option to the user to assign a
> hardware counter to an RMID, event pair and monitor the bandwidth as

Could you please be consistent in this series in how you refer to
an RMID, event pair ? For example later it becomes RMID-event pair.


> long as the counter is assigned. Number of assignments depend on number
> of monitoring counters available.
> 
> Provide the interface to display the number of monitoring counters
> supported.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> v7: Minor commit log text changes.
> 
> v6: No changes.
> 
> v5: Changed the display name from num_cntrs to num_mbm_cntrs.
>     Updated the commit message.
>     Moved the patch after mbm_mode is introduced.
> 
> v4: Changed the counter name to num_cntrs. And few text changes.
> 
> v3: Changed the field name to mbm_assign_cntrs.
> 
> v2: Changed the field name to mbm_assignable_counters from abmc_counte
> ---
>  Documentation/arch/x86/resctrl.rst     |  3 +++
>  arch/x86/kernel/cpu/resctrl/monitor.c  |  1 +
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 16 ++++++++++++++++
>  3 files changed, 20 insertions(+)
> 
> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
> index a7b17ad8acb9..3e9302971faf 100644
> --- a/Documentation/arch/x86/resctrl.rst
> +++ b/Documentation/arch/x86/resctrl.rst
> @@ -290,6 +290,9 @@ with the following files:
>  	than 'num_mbm_cntrs' to be created. Reading the mbm files may report 'Unavailable'
>  	if there is no hardware resource assigned.
>  
> +"num_mbm_cntrs":
> +	The number of monitoring counters available for assignment.
> +

I think it will be helpful if the changelog and the above doc notes when this file can
be expected to be visible since its visibility is not connected to visibility of
"mbm_assign_mode" that refers to it. There also seems to be a conflict here where
"mbm_assign_mode" documentation contains section about "default" that refers to
"num_mbm_cntrs", but "num_mbm_cntrs" may not be visible if "default" is the only mode.

Reinette