Re: [PATCH v14 25/32] fs/resctrl: Provide interface to update the event configurations

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

Hi Babu,

On 6/13/25 2:05 PM, Babu Moger wrote:
> When assignable counters are supported the users can modify the event

"the users" -> "the user" or "users"?

> configuration by writing to the 'event_filter' interface file. The event

nit: "interface file" -> "resctrl file"

> configurations for mbm_event mode are located in
> /sys/fs/resctrl/info/L3_MON/event_configs/.
> 
> Update the assignments of all groups when the event configuration is

(just to help be specific) "all groups" -> "all CTRL_MON and MON resource groups"

> modified.
> 
> Example:
> $ mount -t resctrl resctrl /sys/fs/resctrl
> 
> $ cd /sys/fs/resctrl/
> 
> $ cat info/L3_MON/event_configs/mbm_local_bytes/event_filter
>   local_reads,local_non_temporal_writes,local_reads_slow_memory
> 
> $ echo "local_reads,local_non_temporal_writes" >
>   info/L3_MON/event_configs/mbm_total_bytes/event_filter
> 
> $ cat info/L3_MON/event_configs/mbm_total_bytes/event_filter
>   local_reads,local_non_temporal_writes
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---

...

> ---
>  Documentation/filesystems/resctrl.rst |  12 +++
>  fs/resctrl/rdtgroup.c                 | 120 +++++++++++++++++++++++++-
>  2 files changed, 131 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
> index b1db1a53db2a..2cd6107ca452 100644
> --- a/Documentation/filesystems/resctrl.rst
> +++ b/Documentation/filesystems/resctrl.rst
> @@ -342,6 +342,18 @@ with the following files:
>  	  # cat /sys/fs/resctrl/info/L3_MON/event_configs/mbm_local_bytes/event_filter
>  	  local_reads, local_non_temporal_writes, local_reads_slow_memory
>  
> +	Modify the event configuration by writing to the "event_filter" file within the
> +	configuration directory. The read/write event_filter file contains the configuration

(to help be specific)
"within the configuration directory" -> "within the "event_configs" directory"

Note that "event_filter" is not consistently in quotes.

> +	of the event that reflects which memory transactions are counted by it.
> +
> +	For example::
> +
> +	  # echo "local_reads, local_non_temporal_writes" >
> +	    /sys/fs/resctrl/info/L3_MON/counter_configs/mbm_total_bytes/event_filter

counter_configs -> event_configs

> +
> +	  # cat /sys/fs/resctrl/info/L3_MON/counter_configs/mbm_total_bytes/event_filter

counter_configs -> event_configs

> +	   local_reads, local_non_temporal_writes

Please let example match what code does wrt spacing.

> +
>  "max_threshold_occupancy":
>  		Read/write file provides the largest value (in
>  		bytes) at which a previously used LLC_occupancy
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index e2fa5e10c2dd..fdea608e0796 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -1928,6 +1928,123 @@ static int event_filter_show(struct kernfs_open_file *of, struct seq_file *seq,
>  	return 0;
>  }
>  
> +/**
> + * rdtgroup_assign_cntr - Update the counter assignments for the event in

Could this please be renamed to rdtgroup_update_cntr()? Actually, how about
rdtgroup_update_cntr_event() to pair with a rdtgroup_assign_cntr_event()?

After staring at this code it becomes confusing when the term "assign" is used
for both allocating and just updating. 

Compare for example: rdtgroup_assign_cntrs() with this function ... the
only difference is "cntr" vs "cntrs" in the name but instead of both functions
doing the same just on single vs multiple counters as the name implies they do
significantly different things. I find this very confusing.

> + *			  a group.
> + * @r:		Resource to which update needs to be done.
> + * @rdtgrp:	Resctrl group.
> + * @mevt:	MBM monitor event.
> + */
> +static int rdtgroup_assign_cntr(struct rdt_resource *r, struct rdtgroup *rdtgrp,
> +				struct mon_evt *mevt)
> +{
> +	struct rdt_mon_domain *d;
> +	int cntr_id;
> +
> +	list_for_each_entry(d, &r->mon_domains, hdr.list) {
> +		cntr_id = mbm_cntr_get(r, d, rdtgrp, mevt->evtid);
> +		if (cntr_id >= 0 && d->cntr_cfg[cntr_id].evt_cfg != mevt->evt_cfg) {
> +			d->cntr_cfg[cntr_id].evt_cfg = mevt->evt_cfg;

I referred to this snippet in earlier comment 
https://lore.kernel.org/lkml/887bad33-7f4a-4b6d-95a7-fdfe0451f42b@intel.com/

> +			resctrl_arch_config_cntr(r, d, mevt->evtid, rdtgrp->mon.rmid,
> +						 rdtgrp->closid, cntr_id, true);
> +		}
> +	}
> +
> +	return 0;

Looks like this can be a void function.

> +}
> +
> +/**
> + * resctrl_assign_cntr_allrdtgrp - Update the counter assignments for the event
> + *				   for all the groups.
> + * @r:		Resource to which update needs to be done.
> + * @mevt	MBM Monitor event.
> + */
> +static void resctrl_assign_cntr_allrdtgrp(struct rdt_resource *r, struct mon_evt *mevt)

resctrl_assign_cntr_allrdtgrp() -> resctrl_update_cntr_allrdtgrp()/resctrl_update_cntr_event_allrdtgrp()

> +{
> +	struct rdtgroup *prgrp, *crgrp;
> +
> +	/*
> +	 * Check all the groups where the event tyoe is assigned and update

I am not sure what is meant with "Check" here. Maybe "Find"?

tyoe -> type?

> +	 * the assignment
> +	 */
> +	list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
> +		rdtgroup_assign_cntr(r, prgrp, mevt);
> +
> +		list_for_each_entry(crgrp, &prgrp->mon.crdtgrp_list, mon.crdtgrp_list)
> +			rdtgroup_assign_cntr(r, crgrp, mevt);
> +	}
> +}
> +
> +static int resctrl_process_configs(char *tok, u32 *val)
> +{
> +	char *evt_str;
> +	u32 temp_val;
> +	bool found;
> +	int i;
> +
> +next_config:
> +	if (!tok || tok[0] == '\0')
> +		return 0;
> +
> +	/* Start processing the strings for each memory transaction type */
> +	evt_str = strim(strsep(&tok, ","));
> +	found = false;
> +	for (i = 0; i < NUM_MBM_EVT_VALUES; i++) {
> +		if (!strcmp(mbm_config_values[i].name, evt_str)) {
> +			temp_val = mbm_config_values[i].val;
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	if (!found) {
> +		rdt_last_cmd_printf("Invalid memory transaction type %s\n", evt_str);
> +		return -EINVAL;
> +	}
> +
> +	*val |= temp_val;

This still returns a partially initialized value on failure. Please only set
provided parameter on success.

> +
> +	goto next_config;
> +}
> +
> +static ssize_t event_filter_write(struct kernfs_open_file *of, char *buf,
> +				  size_t nbytes, loff_t off)
> +{
> +	struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
> +	struct mon_evt *mevt = rdt_kn_parent_priv(of->kn);

With mon_evt::rid available it should not be necessary to hardcode the resource?
Do any of these new functions need a struct rdt_resource parameter in addition
to struct mon_evt?

> +	u32 evt_cfg = 0;
> +	int ret = 0;
> +
> +	/* 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 (!resctrl_arch_mbm_cntr_assign_enabled(r)) {
> +		rdt_last_cmd_puts("mbm_cntr_assign mode is not enabled\n");

Needs update to new names.

> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +	ret = resctrl_process_configs(buf, &evt_cfg);
> +	if (!ret && mevt->evt_cfg != evt_cfg) {
> +		mevt->evt_cfg = evt_cfg;
> +		resctrl_assign_cntr_allrdtgrp(r, mevt);

Could only event_filter_write() be in fs/resctrl/rdtgroup.c with the rest
of the functions introduced here located with rest of monitoring code
in fs/resctrl/monitor.c?

> +	}
> +
> +out_unlock:
> +	mutex_unlock(&rdtgroup_mutex);
> +	cpus_read_unlock();
> +
> +	return ret ?: nbytes;
> +}
> +
>  /* rdtgroup information files for one cache resource. */
>  static struct rftype res_common_files[] = {
>  	{
> @@ -2054,9 +2171,10 @@ static struct rftype res_common_files[] = {
>  	},
>  	{
>  		.name		= "event_filter",
> -		.mode		= 0444,
> +		.mode		= 0644,
>  		.kf_ops		= &rdtgroup_kf_single_ops,
>  		.seq_show	= event_filter_show,
> +		.write		= event_filter_write,
>  	},
>  	{
>  		.name		= "mbm_assign_mode",

Reinette