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

["Moger, Babu" <babu.moger@amd.com>]

Hi Reinette,

On 5/22/25 23:43, Reinette Chatre wrote:
> 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."
> 

Looks good. Thanks. The directory name will be "event_configs". Will
change it in the code as well.

>>
>> 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 ...

Sounds good.


> 
>> +
>> +	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? 

Should be "memory trasactions"

> 
>> +	configurations: mbm_local_bytes and mbm_total_bytes.
> 
> "two default configurations"? These are not "configurations" but "events", no?

Sure. Should be "two default events"

> 
>> +
>> +	Following types of events are supported:
> 
> events -> memory transactions?

Sure.

> 
> I am unable to parse the above.

The following are the types of memory transactions that an MBM event can
be configured with:

> 
> 
>> +
>> +	==== ========================= ============================================================
>> +	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?

Not required. Will remove it.

> 
>> +
>> +	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?

Yes.
Need to introduce evt_cfg member and also need to initialize the default
values during the init. Will order it correctly to make little bit clear.


> 
>> +			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?

Yes. Will remove mbm_mode and use Tony's telemetry work.

> 
>> +			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.

Yes. I see it. It should be "out_config".

> 
>> +			}
>> +
>> +			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.

Yes. Will remove it.

> 
>> +
>> +	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
> 

-- 
Thanks
Babu Moger