Re: [PATCH v8 21/25] x86/resctrl: Introduce the interface to switch between monitor modes

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

Hi Babu,

On 10/9/24 10:39 AM, Babu Moger wrote:
> Introduce interface to switch between mbm_cntr_assign and default modes.
> 
> $ 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.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---

...

> ---
>  Documentation/arch/x86/resctrl.rst     | 15 ++++++
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 75 +++++++++++++++++++++++++-
>  2 files changed, 89 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
> index 99ee9c87952b..d9574078f735 100644
> --- a/Documentation/arch/x86/resctrl.rst
> +++ b/Documentation/arch/x86/resctrl.rst
> @@ -291,6 +291,21 @@ with the following files:
>  	that case reading the mbm_total_bytes and mbm_local_bytes may report
>  	'Unavailable' if there is no counter associated with that group.
>  
> +	* To enable "mbm_cntr_assign" mode:
> +	  ::
> +
> +	    # echo  "mbm_cntr_assign" > /sys/fs/resctrl/info/L3_MON/mbm_assign_mode

extra spaces

> +
> +	* To enable default monitoring mode:
> +	  ::
> +
> +	    # echo  "default" > /sys/fs/resctrl/info/L3_MON/mbm_assign_mode

extra spaces

> +
> +	The MBM events (mbm_total_bytes and/or mbm_local_bytes) associated counters

I did ask you not to copy the text verbatim
https://lore.kernel.org/all/b38c93bf-4650-45d1-9aca-8b4c4d425886@intel.com/

> +	may reset when the mode is changed. Moving to mbm_cntr_assign mode will
> +	require users to assign the counters to the events. Otherwise, the MBM

"will require" -> "require"

> +	event counters will return "Unassigned" when read.
> +
>  "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 cb2c60c0319e..88eda3cf5c82 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -888,6 +888,78 @@ static int rdtgroup_mbm_assign_mode_show(struct kernfs_open_file *of,
>  	return 0;
>  }
>  
> +static void mbm_cntr_reset(struct rdt_resource *r)
> +{
> +	struct rdtgroup *prgrp, *crgrp;
> +	struct rdt_mon_domain *dom;
> +
> +	/*
> +	 * Hardware counters will reset after switching the monitor mode.
> +	 * Reset the architectural state so that reading of hardware
> +	 * counter is not considered as an overflow in the next update.
> +	 * Also reset the domain counter bitmap.
> +	 */
> +	list_for_each_entry(dom, &r->mon_domains, hdr.list) {
> +		bitmap_zero(dom->mbm_cntr_map, r->mon.num_mbm_cntrs);
> +		resctrl_arch_reset_rmid_all(r, dom);
> +	}
> +
> +	/* Reset global MBM counter map */
> +	bitmap_fill(r->mon.mbm_cntr_free_map, r->mon.num_mbm_cntrs);
> +
> +	/* Reset the cntr_id's for all the monitor groups */
> +	list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
> +		prgrp->mon.cntr_id[0] = MON_CNTR_UNSET;
> +		prgrp->mon.cntr_id[1] = MON_CNTR_UNSET;
> +		list_for_each_entry(crgrp, &prgrp->mon.crdtgrp_list,
> +				    mon.crdtgrp_list) {
> +			crgrp->mon.cntr_id[0] = MON_CNTR_UNSET;
> +			crgrp->mon.cntr_id[1] = MON_CNTR_UNSET;
> +		}

Please use MBM_EVENT_ARRAY_INDEX

> +	}
> +}
> +
> +static ssize_t rdtgroup_mbm_assign_mode_write(struct kernfs_open_file *of,
> +					      char *buf, size_t nbytes, loff_t off)
> +{
> +	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")) {
> +		enable = 1;
> +	} else {
> +		ret = -EINVAL;
> +		rdt_last_cmd_puts("Unsupported assign mode\n");
> +		goto write_exit;
> +	}

Please keep two things in mind:
* this file is always accessible, whether platform supports assignable
  counters or not.
* this is resctrl fs code.

So, considering above, how should user interpret the "Unsupported assign mode"?
Shouldn't it also return this error if a user attempts to enable
"mbm_cntr_assign" on a platform that does not support this mode?

> +
> +	if (enable != resctrl_arch_mbm_cntr_assign_enabled(r)) {

resctrl_arch_mbm_cntr_assign_enabled() returns true if mbm_cntr_assign
mode is enabled, but when it returns false it could mean different things:
platform supports mbm_cntr_assign mode, but it is disabled, or platform
does not support mbm_cntr_assign mode.

resctrl fs should not rely on all archs to duplicate the all the checking done
in resctrl_arch_mbm_cntr_assign_set(). It should never ask arch to enable a mode
that it knows the platform is not capable of.

> +		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;
> +}
> +
>  static int rdtgroup_num_mbm_cntrs_show(struct kernfs_open_file *of,
>  				       struct seq_file *s, void *v)
>  {
> @@ -2115,9 +2187,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