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

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

Hi Babu,

On 5/24/24 5:23 AM, Babu Moger wrote:
> The ABMC feature provides an option to the user to assign a hardware
> counter to an RMID and monitor the bandwidth as long as it is assigned.
> ABMC mode is enabled by default when supported. System can be one mode
> at a time (Legacy monitor mode or ABMC mode).
> 
> Provide an interface to display the monitor mode on the system.
>      $cat /sys/fs/resctrl/info/L3_MON/mbm_assign
>      [abmc]
>      legacy

Output is different from cover letter and what this patch implements.

> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> v4: Fixed the checks for legacy and abmc mode. Default it ABMC.
> 
> v3: New patch to display ABMC capability.
> ---
>   Documentation/arch/x86/resctrl.rst     | 10 ++++++++++
>   arch/x86/kernel/cpu/resctrl/monitor.c  |  1 +
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 23 +++++++++++++++++++++++
>   3 files changed, 34 insertions(+)
> 
> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
> index 7ab8172ef208..ab3cde61a124 100644
> --- a/Documentation/arch/x86/resctrl.rst
> +++ b/Documentation/arch/x86/resctrl.rst
> @@ -261,6 +261,16 @@ with the following files:
>   	Available when ABMC feature is supported. The number of ABMC counters
>   	available for configuration.
>   
> +"mbm_assign":

This name is not ideal but I am having trouble finding a better one ... I have
seen you use "monitor mode" a couple of times (even in shortlog), so maybe that
could be the start of a more generic name? "mbm_mode"?

> +	Available when ABMC feature is supported. Reports the list of assignable

Why not always make this file available? At least it will display that
legacy mode is supported and it gives user space an always present file to query to
determine support.

> +	monitoring features supported. The enclosed brackets indicate which
> +	feature is enabled.
> +	::
> +
> +	  cat /sys/fs/resctrl/info/L3_MON/mbm_assign
> +	  [abmc]
> +	  mbm_legacy
> +
>   "max_threshold_occupancy":
>   		Read/write file provides the largest value (in
>   		bytes) at which a previously used LLC_occupancy
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index e75a6146068b..b1d002e5e93d 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -1071,6 +1071,7 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>   
>   		if (rdt_cpu_has(X86_FEATURE_ABMC)) {
>   			r->abmc_capable = true;
> +			resctrl_file_fflags_init("mbm_assign", RFTYPE_MON_INFO);
>   			/*
>   			 * Query CPUID_Fn80000020_EBX_x05 for number of
>   			 * ABMC counters
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 9148d1234ede..3071bbb7a15e 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -856,6 +856,23 @@ static int rdtgroup_num_cntrs_show(struct kernfs_open_file *of,
>   	return 0;
>   }
>   
> +static int rdtgroup_mbm_assign_show(struct kernfs_open_file *of,
> +				    struct seq_file *s, void *v)
> +{
> +	struct rdt_resource *r = of->kn->parent->priv;
> +	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);

Please use arch helper to just get abmc state instead of fs code
digging into arch structures.

> +
> +	if (hw_res->abmc_enabled) {
> +		seq_puts(s, "[abmc]\n");
> +		seq_puts(s, "mbm_legacy\n");
> +	} else {
> +		seq_puts(s, "abmc\n");
> +		seq_puts(s, "[mbm_legacy]\n");
> +	}
> +
> +	return 0;
> +}
> +
>   #ifdef CONFIG_PROC_CPU_RESCTRL
>   
>   /*
> @@ -1913,6 +1930,12 @@ static struct rftype res_common_files[] = {
>   		.seq_show	= mbm_local_bytes_config_show,
>   		.write		= mbm_local_bytes_config_write,
>   	},
> +	{
> +		.name		= "mbm_assign",
> +		.mode		= 0444,
> +		.kf_ops		= &rdtgroup_kf_single_ops,
> +		.seq_show	= rdtgroup_mbm_assign_show,
> +	},
>   	{
>   		.name		= "cpus",
>   		.mode		= 0644,

Reinette