Re: [PATCH v13 21/27] x86/resctrl: Introduce mbm_assign_on_mkdir to configure assignments

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

Hi Babu,

On 5/15/25 3:52 PM, Babu Moger wrote:
> The mbm_cntr_assign mode provides an option to the user to assign a
> counter to an RMID, event pair and monitor the bandwidth as long as
> the counter is assigned.
> 
> Introduce a configuration option to automatically assign counter IDs

"assign counter IDs" -> "assign counter IDs to <what?>"

> when a resctrl group is created, provided the counters are available.
> By default, this option is enabled at boot.
> 
> Suggested-by: Peter Newman <peternewman@google.com>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> v13: Added Suggested-by tag.
>      Resolved conflicts caused by the recent FS/ARCH code restructure.
>      The rdtgroup.c/monitor.c file has now been split between the FS and ARCH directories.
> 
> v12: New patch. Added after the discussion on the list.
>      https://lore.kernel.org/lkml/CALPaoCh8siZKjL_3yvOYGL4cF_n_38KpUFgHVGbQ86nD+Q2_SA@mail.gmail.com/
> ---
>  Documentation/filesystems/resctrl.rst | 10 ++++++
>  fs/resctrl/monitor.c                  |  2 ++
>  fs/resctrl/rdtgroup.c                 | 44 +++++++++++++++++++++++++--
>  include/linux/resctrl.h               |  2 ++
>  4 files changed, 56 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
> index 9923276826db..356f1f918a86 100644
> --- a/Documentation/filesystems/resctrl.rst
> +++ b/Documentation/filesystems/resctrl.rst
> @@ -348,6 +348,16 @@ with the following files:
>  	  # cat /sys/fs/resctrl/info/L3_MON/counter_configs/mbm_total_bytes/event_filter
>  	   local_reads, local_non_temporal_writes
>  
> +"mbm_assign_on_mkdir":
> +	Automatically assign the monitoring counters on resctrl group creation

assign the monitoring counters to what?

> +	if the counters are available. It is enabled by default on boot and users
> +	can disable by writing to the interface.
> +	::
> +
> +	  # echo 0 > /sys/fs/resctrl/info/L3_MON/mbm_assign_on_mkdir
> +	  # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_on_mkdir
> +	  0

Please be explicit in docs what possible values are and what they mean.

> +
>  "max_threshold_occupancy":
>  		Read/write file provides the largest value (in
>  		bytes) at which a previously used LLC_occupancy
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index 1f72249a5c93..5f6c4b662f3b 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -933,6 +933,8 @@ int resctrl_mon_resource_init(void)
>  		resctrl_file_fflags_init("available_mbm_cntrs",
>  					 RFTYPE_MON_INFO | RFTYPE_RES_CACHE);
>  		resctrl_file_fflags_init("event_filter", RFTYPE_ASSIGN_CONFIG);
> +		resctrl_file_fflags_init("mbm_assign_on_mkdir", RFTYPE_MON_INFO |
> +					 RFTYPE_RES_CACHE);
>  	}
>  
>  	return 0;
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index 8c498b41be5d..0093b323d858 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -2035,8 +2035,8 @@ static ssize_t event_filter_write(struct kernfs_open_file *of, char *buf,
>  	}
>  
>  	ret = resctrl_process_configs(buf, &evt_cfg);
> -	if (!ret && mevt->evt_val != evt_cfg) {
> -		mevt->evt_val = evt_cfg;
> +	if (!ret && mevt->evt_cfg != evt_cfg) {
> +		mevt->evt_cfg = evt_cfg;
>  		resctrl_update_assign(r, mevt->evtid, evt_cfg);
>  	}
>  

Needs to be squashed.

> @@ -2047,6 +2047,39 @@ static ssize_t event_filter_write(struct kernfs_open_file *of, char *buf,
>  	return ret ?: nbytes;
>  }
>  
> +static int resctrl_mbm_assign_on_mkdir_show(struct kernfs_open_file *of,
> +					    struct seq_file *s, void *v)
> +{
> +	struct rdt_resource *r = rdt_kn_parent_priv(of->kn);
> +
> +	seq_printf(s, "%u\n", r->mon.mbm_assign_on_mkdir);
> +
> +	return 0;
> +}
> +
> +static ssize_t resctrl_mbm_assign_on_mkdir_write(struct kernfs_open_file *of,
> +						 char *buf, size_t nbytes, loff_t off)
> +{
> +	struct rdt_resource *r = rdt_kn_parent_priv(of->kn);
> +	bool value;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &value);
> +	if (ret)
> +		return ret;
> +
> +	cpus_read_lock();

not traversing the domain list so hotplug lock not needed.

> +	mutex_lock(&rdtgroup_mutex);

rdtgroup_mutex seems only needed because the message buffer is cleared below, and this is why it
is not required in the show()?

> +	rdt_last_cmd_clear();
> +
> +	r->mon.mbm_assign_on_mkdir = value;
> +
> +	mutex_unlock(&rdtgroup_mutex);
> +	cpus_read_unlock();
> +
> +	return ret ?: nbytes;
> +}
> +
>  /* rdtgroup information files for one cache resource. */
>  static struct rftype res_common_files[] = {
>  	{
> @@ -2056,6 +2089,13 @@ static struct rftype res_common_files[] = {
>  		.seq_show	= rdt_last_cmd_status_show,
>  		.fflags		= RFTYPE_TOP_INFO,
>  	},
> +	{
> +		.name		= "mbm_assign_on_mkdir",
> +		.mode		= 0644,
> +		.kf_ops		= &rdtgroup_kf_single_ops,
> +		.seq_show	= resctrl_mbm_assign_on_mkdir_show,
> +		.write		= resctrl_mbm_assign_on_mkdir_write,
> +	},
>  	{
>  		.name		= "num_closids",
>  		.mode		= 0444,
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index cd24d1577e0a..d6435abdde7b 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -278,6 +278,7 @@ enum resctrl_schema_fmt {
>   *			monitoring events can be configured.
>   * @num_mbm_cntrs:	Number of assignable monitoring counters
>   * @mbm_cntr_assignable:Is system capable of supporting monitor assignment?
> + * @mbm_assign_on_mkdir:Auto enable monitor assignment on mkdir?

How is "monitor assignment" different from "counter assignment"?

>   * @evt_list:		List of monitoring events
>   */
>  struct resctrl_mon {
> @@ -285,6 +286,7 @@ struct resctrl_mon {
>  	unsigned int		mbm_cfg_mask;
>  	int			num_mbm_cntrs;
>  	bool			mbm_cntr_assignable;
> +	bool			mbm_assign_on_mkdir;
>  	struct list_head	evt_list;
>  };
>  

Reinette