Re: [PATCH v13 19/27] x86/resctrl: Add event configuration directory under info/L3_MON/

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

Hi Babu,

On 5/15/25 3:52 PM, Babu Moger wrote:
> Create the configuration directory and files for mbm_cntr_assign mode.
> These configurations will be used to assign MBM events in mbm_cntr_assign
> mode, with two default configurations created upon mounting.

This just jumps in with what the patch does. Requirements for proper changelog
should be familiar by now. The changelog *always* starts with a context.

Sample:

"When assignable counters are supported the
/sys/fs/resctrl/info/L3_MON/event_configs directory contains a sub-directory
for each MBM event that can be assigned to a counter. The MBM event
sub-directory contains a file named "event_filter" that is used to
view and modify which memory transactions the MBM event is configured with.

Create the /sys/fs/resctrl/info/L3_MON/event_configs directory on resctrl
mount and pre-populate it with directories for the two existing MBM events:
mbm_total_bytes and mbm_local_bytes. Create the "event_filter" file within
each MBM event directory with the needed *show() that displays the memory
transactions with which the MBM event is configured."

> 
> Example:
> $ cd /sys/fs/resctrl/
> $ cat info/L3_MON/counter_configs/mbm_total_bytes/event_filter
>   local_reads, remote_reads, local_non_temporal_writes,
>   remote_non_temporal_writes, local_reads_slow_memory,
>   remote_reads_slow_memory, dirty_victim_writes_all
> 
> $ cat info/L3_MON/counter_configs/mbm_local_bytes/event_filter
>   local_reads, local_non_temporal_writes, local_reads_slow_memory
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> v13: Updated user doc (resctrl.rst).
>      Changed the name of the function resctrl_mkdir_info_configs to
>      resctrl_mkdir_counter_configs().
>      Replaced seq_puts() with seq_putc() where applicable.
>      Removed RFTYPE_MON_CONFIG definition. Not required.
>      Changed the name of the flag RFTYPE_CONFIG to RFTYPE_ASSIGN_CONFIG.
>      Reinette suggested RFTYPE_MBM_EVENT_CONFIG but RFTYPE_ASSIGN_CONFIG
>      seemed shorter and pricise.
>      The configuration is created using evt_list.
>      Resolved conflicts caused by the recent FS/ARCH code restructure.
>      The monitor.c/rdtgroup.c files have been split between the FS and ARCH directories.
> 
> v12: New patch to hold the MBM event configurations for mbm_cntr_assign mode.
> ---
>  Documentation/filesystems/resctrl.rst | 30 ++++++++++
>  fs/resctrl/internal.h                 |  2 +
>  fs/resctrl/monitor.c                  |  1 +
>  fs/resctrl/rdtgroup.c                 | 80 +++++++++++++++++++++++++++
>  4 files changed, 113 insertions(+)
> 
> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
> index 5cf2d742f04c..4eb9f007ba3d 100644
> --- a/Documentation/filesystems/resctrl.rst
> +++ b/Documentation/filesystems/resctrl.rst
> @@ -306,6 +306,36 @@ with the following files:
>  	  # cat /sys/fs/resctrl/info/L3_MON/available_mbm_cntrs
>  	  0=30;1=30
>  
> +"counter_configs":
> +	When the "mbm_cntr_assign" mode is supported, a dedicated directory is created
> +	under the "L3_MON" directory to store configuration files.

? it does not contain files but directories for each event, no?

It will help if the text is specific. For example,
	"event_configs":
	Directory that exists when mbm_cntr_evt_assign is supported. Contains sub-directory
	for each MBM event that can be assigned to a counter. Each MBM event
	sub-directory ...

> +
> +	These files contain the list of configurable events. There are two default

So confusing ... terminology is all over the place. Which files are even talked about here?
"configurable events" ... are these the memory transactions or MBM events? 

> +	configurations: mbm_local_bytes and mbm_total_bytes.

"two default configurations"? These are not "configurations" but "events", no?

> +
> +	Following types of events are supported:

events -> memory transactions?

I am unable to parse the above.


> +
> +	==== ========================= ============================================================
> +	Bits Name   		         Description
> +	==== ========================= ============================================================
> +	6    dirty_victim_writes_all     Dirty Victims from the QOS domain to all types of memory
> +	5    remote_reads_slow_memory    Reads to slow memory in the non-local NUMA domain
> +	4    local_reads_slow_memory     Reads to slow memory in the local NUMA domain
> +	3    remote_non_temporal_writes  Non-temporal writes to non-local NUMA domain
> +	2    local_non_temporal_writes   Non-temporal writes to local NUMA domain
> +	1    remote_reads                Reads to memory in the non-local NUMA domain
> +	0    local_reads                 Reads to memory in the local NUMA domain
> +	==== ========================= ==========================================================

Why does user need to know the bit position used to represent the memory transaction?

> +
> +	For example::
> +
> +	  # cat /sys/fs/resctrl/info/L3_MON/counter_configs/mbm_total_bytes/event_filter
> +	  local_reads, remote_reads, local_non_temporal_writes, remote_non_temporal_writes,
> +	  local_reads_slow_memory, remote_reads_slow_memory, dirty_victim_writes_all
> +
> +	  # cat /sys/fs/resctrl/info/L3_MON/counter_configs/mbm_local_bytes/event_filter
> +	  local_reads, local_non_temporal_writes, local_reads_slow_memory
> +
>  "max_threshold_occupancy":
>  		Read/write file provides the largest value (in
>  		bytes) at which a previously used LLC_occupancy
> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
> index 019d00bf5adf..446cc9cc61df 100644
> --- a/fs/resctrl/internal.h
> +++ b/fs/resctrl/internal.h
> @@ -238,6 +238,8 @@ struct mbm_evt_value {
>  
>  #define RFTYPE_DEBUG			BIT(10)
>  
> +#define RFTYPE_ASSIGN_CONFIG		BIT(11)
> +
>  #define RFTYPE_CTRL_INFO		(RFTYPE_INFO | RFTYPE_CTRL)
>  
>  #define RFTYPE_MON_INFO			(RFTYPE_INFO | RFTYPE_MON)
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index 72f3dfb5b903..1f72249a5c93 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -932,6 +932,7 @@ int resctrl_mon_resource_init(void)
>  					 RFTYPE_MON_INFO | RFTYPE_RES_CACHE);
>  		resctrl_file_fflags_init("available_mbm_cntrs",
>  					 RFTYPE_MON_INFO | RFTYPE_RES_CACHE);
> +		resctrl_file_fflags_init("event_filter", RFTYPE_ASSIGN_CONFIG);
>  	}
>  
>  	return 0;
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index b109e91096b0..cf84e3a382ac 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -1911,6 +1911,25 @@ static int resctrl_available_mbm_cntrs_show(struct kernfs_open_file *of,
>  	return ret;
>  }
>  
> +static int event_filter_show(struct kernfs_open_file *of, struct seq_file *seq, void *v)
> +{
> +	struct mon_evt *mevt = rdt_kn_parent_priv(of->kn);
> +	bool sep = false;
> +	int i;
> +
> +	for (i = 0; i < NUM_MBM_EVT_VALUES; i++) {
> +		if (mevt->evt_cfg & mbm_evt_values[i].evt_val) {

Still no idea where mevt->evt_cfg comes from. Patch ordering issue?

> +			if (sep)
> +				seq_putc(seq, ',');
> +			seq_printf(seq, "%s", mbm_evt_values[i].evt_name);
> +			sep = true;
> +		}
> +	}
> +	seq_putc(seq, '\n');
> +
> +	return 0;
> +}
> +
>  /* rdtgroup information files for one cache resource. */
>  static struct rftype res_common_files[] = {
>  	{
> @@ -2035,6 +2054,12 @@ static struct rftype res_common_files[] = {
>  		.seq_show	= mbm_local_bytes_config_show,
>  		.write		= mbm_local_bytes_config_write,
>  	},
> +	{
> +		.name		= "event_filter",
> +		.mode		= 0444,
> +		.kf_ops		= &rdtgroup_kf_single_ops,
> +		.seq_show	= event_filter_show,
> +	},
>  	{
>  		.name		= "mbm_assign_mode",
>  		.mode		= 0444,
> @@ -2317,6 +2342,55 @@ static int rdtgroup_mkdir_info_resdir(void *priv, char *name,
>  	return ret;
>  }
>  
> +static int resctrl_mkdir_counter_configs(struct rdt_resource *r, char *name)
> +{
> +	struct kernfs_node *l3_mon_kn, *kn_subdir, *kn_subdir2;
> +	struct mon_evt *mevt;
> +	int ret;
> +
> +	l3_mon_kn = kernfs_find_and_get(kn_info, name);
> +	if (!l3_mon_kn)
> +		return -ENOENT;
> +
> +	kn_subdir = kernfs_create_dir(l3_mon_kn, "counter_configs", l3_mon_kn->mode, NULL);
> +	if (IS_ERR(kn_subdir)) {
> +		kernfs_put(l3_mon_kn);
> +		return PTR_ERR(kn_subdir);
> +	}
> +
> +	ret = rdtgroup_kn_set_ugid(kn_subdir);
> +	if (ret) {
> +		kernfs_put(l3_mon_kn);
> +		return ret;
> +	}
> +
> +	list_for_each_entry(mevt, &r->mon.evt_list, list) {
> +		if (mevt->mbm_mode == MBM_MODE_ASSIGN) {

I do not think this "mbm_mode" is needed, resctrl_mon::mbm_cntr_assignable is already used
earlier, so would for_each_mbm_event() from the telemetry work be useful here?

> +			kn_subdir2 = kernfs_create_dir(kn_subdir, mevt->name,
> +						       kn_subdir->mode, mevt);
> +			if (IS_ERR(kn_subdir2)) {
> +				ret = PTR_ERR(kn_subdir2);
> +				goto config_out;

"grep goto fs/resctrl/rdtgroup.c" for naming conventions.

> +			}
> +
> +			ret = rdtgroup_kn_set_ugid(kn_subdir2);
> +			if (ret)
> +				goto config_out;
> +
> +			ret = rdtgroup_add_files(kn_subdir2, RFTYPE_ASSIGN_CONFIG);
> +			if (!ret)
> +				kernfs_activate(kn_subdir);
> +		}
> +	}
> +
> +config_out:
> +	kernfs_put(l3_mon_kn);
> +	if (ret)
> +		kernfs_remove(kn_subdir);

This looks unnecessary since caller does kernfs_remove() on error return. Compare
with how rdtgroup_mkdir_info_resdir() handles errors.

> +
> +	return ret;
> +}
> +
>  static unsigned long fflags_from_resource(struct rdt_resource *r)
>  {
>  	switch (r->rid) {
> @@ -2363,6 +2437,12 @@ static int rdtgroup_create_info_dir(struct kernfs_node *parent_kn)
>  		ret = rdtgroup_mkdir_info_resdir(r, name, fflags);
>  		if (ret)
>  			goto out_destroy;
> +
> +		if (r->mon.mbm_cntr_assignable) {
> +			ret = resctrl_mkdir_counter_configs(r, name);
> +			if (ret)
> +				goto out_destroy;
> +		}
>  	}
>  
>  	ret = rdtgroup_kn_set_ugid(kn_info);

Reinette