Re: [PATCH v10 20/24] x86/resctrl: Introduce the interface to switch between monitor modes

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

Hi Babu,

On 12/12/24 12:15 PM, Babu Moger wrote:
> Introduce interface to switch between mbm_cntr_assign and default modes.
> 

This changelog needs context.

> $ cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode
> [mbm_cntr_assign]
> default
> 
> To enable the "mbm_cntr_assign" mode:
> $ echo "mbm_cntr_assign" > /sys/fs/resctrl/info/L3_MON/mbm_assign_mode
> 
> To enable the default monitoring mode:
> $ echo "default" > /sys/fs/resctrl/info/L3_MON/mbm_assign_mode
> 
> MBM event counters will reset when mbm_assign_mode is changed.

I think it will help to elaborate on this.

I understand this as two parts. As stated, the hardware counters
are reset since that is what ABMC does. In this patch
there is a mbm_cntr_reset() but that does not actually reset the counters as
the above implies.
Instead, the counters are automatically reset as part of changing the mode. 
resctrl triggers reset of architectural and non-architectural
state of the events because of the hardware counter reset.

The changelog can really do more to explain what this patch does.

> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---

> ---
>  Documentation/arch/x86/resctrl.rst     | 15 ++++++++
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 50 +++++++++++++++++++++++++-
>  2 files changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
> index 3ec14c314606..d3a8a34cf629 100644
> --- a/Documentation/arch/x86/resctrl.rst
> +++ b/Documentation/arch/x86/resctrl.rst
> @@ -290,6 +290,21 @@ with the following files:
>  	"mbm_total_bytes" or "mbm_local_bytes" will report 'Unavailable' if
>  	there is no counter associated with that event.
>  
> +	* To enable "mbm_cntr_assign" mode:
> +	  ::
> +
> +	    # echo "mbm_cntr_assign" > /sys/fs/resctrl/info/L3_MON/mbm_assign_mode
> +
> +	* To enable default monitoring mode:
> +	  ::
> +
> +	    # echo "default" > /sys/fs/resctrl/info/L3_MON/mbm_assign_mode
> +
> +	The MBM events (mbm_total_bytes and/or mbm_local_bytes) associated with
> +	counters may reset when "mbm_assign_mode" is changed. Moving to

After looking at the final documentation it seems more appropriate to move this to
the top of the "mbm_assign_mode" section. The top already shows how to read from the
file using cat so it seems like a good match to document write to the file in the
same area.

> +	mbm_cntr_assign mode require users to assign the counters to the events.
> +	Otherwise, the MBM event counters will return "Unassigned" when read.

This portion can move to the mode it applies to.

> +
>  "num_mbm_cntrs":
>  	The number of monitoring counters available for assignment when the
>  	architecture supports mbm_cntr_assign mode.
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 8d00b1689a80..eea534cce3d0 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -939,6 +939,53 @@ static void mbm_cntr_reset(struct rdt_resource *r)
>  	}
>  }
>  
> +static ssize_t rdtgroup_mbm_assign_mode_write(struct kernfs_open_file *of,
> +					      char *buf, size_t nbytes, loff_t off)

rdtgroup_ namespace is not appropriate

> +{
> +	struct rdt_resource *r = of->kn->parent->priv;
> +	int ret = 0;
> +	bool enable;
> +
> +	/* Valid input requires a trailing newline */
> +	if (nbytes == 0 || buf[nbytes - 1] != '\n')
> +		return -EINVAL;
> +
> +	buf[nbytes - 1] = '\0';
> +
> +	cpus_read_lock();
> +	mutex_lock(&rdtgroup_mutex);
> +
> +	rdt_last_cmd_clear();
> +
> +	if (!strcmp(buf, "default")) {
> +		enable = 0;
> +	} else if (!strcmp(buf, "mbm_cntr_assign")) {
> +		if (r->mon.mbm_cntr_assignable) {
> +			enable = 1;
> +		} else {
> +			ret = -EINVAL;
> +			rdt_last_cmd_puts("mbm_cntr_assign mode is not supported\n");
> +			goto write_exit;
> +		}
> +	} else {
> +		ret = -EINVAL;
> +		rdt_last_cmd_puts("Unsupported assign mode\n");
> +		goto write_exit;
> +	}
> +
> +	if (enable != resctrl_arch_mbm_cntr_assign_enabled(r)) {
> +		ret = resctrl_arch_mbm_cntr_assign_set(r, enable);
> +		if (!ret)
> +			mbm_cntr_reset(r);
> +	}
> +
> +write_exit:
> +	mutex_unlock(&rdtgroup_mutex);
> +	cpus_read_unlock();
> +
> +	return ret ?: nbytes;
> +}
> +
>  #ifdef CONFIG_PROC_CPU_RESCTRL
>  
>  /*
> @@ -2222,9 +2269,10 @@ static struct rftype res_common_files[] = {
>  	},
>  	{
>  		.name		= "mbm_assign_mode",
> -		.mode		= 0444,
> +		.mode		= 0644,
>  		.kf_ops		= &rdtgroup_kf_single_ops,
>  		.seq_show	= rdtgroup_mbm_assign_mode_show,
> +		.write		= rdtgroup_mbm_assign_mode_write,
>  		.fflags		= RFTYPE_MON_INFO,
>  	},
>  	{

Reinette