Message-ID: <fc66675c-7230-4774-b3ba-617639197579@amd.com>
Date: Mon, 30 Jun 2025 14:06:38 -0500
From: "Moger, Babu" <babu.moger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>, corbet@....net,
 tony.luck@...el.com, Dave.Martin@....com, james.morse@....com,
 tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
 dave.hansen@...ux.intel.com
Cc: x86@...nel.org, hpa@...or.com, akpm@...ux-foundation.org,
 rostedt@...dmis.org, paulmck@...nel.org, thuth@...hat.com, ardb@...nel.org,
 gregkh@...uxfoundation.org, seanjc@...gle.com, thomas.lendacky@....com,
 pawan.kumar.gupta@...ux.intel.com, manali.shukla@....com,
 perry.yuan@....com, kai.huang@...el.com, peterz@...radead.org,
 xiaoyao.li@...el.com, kan.liang@...ux.intel.com, mario.limonciello@....com,
 xin3.li@...el.com, gautham.shenoy@....com, xin@...or.com,
 chang.seok.bae@...el.com, fenghuay@...dia.com, peternewman@...gle.com,
 maciej.wieczor-retman@...el.com, eranian@...gle.com,
 linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v14 24/32] fs/resctrl: Add event configuration directory
 under info/L3_MON/

Hi Reinette,

On 6/25/25 18:23, Reinette Chatre wrote:
> Hi Babu,
> 
> On 6/13/25 2:05 PM, Babu Moger wrote:
>> 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:
>> $ mount -t resctrl resctrl /sys/fs/resctrl
>> $ cd /sys/fs/resctrl/
>> $ cat info/L3_MON/event_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/event_configs/mbm_local_bytes/event_filter
>>   local_reads, local_non_temporal_writes, local_reads_slow_memory
> 
> Please let these examples match what the patch does wrt spacing.

Sure.

> 
>>
>> Signed-off-by: Babu Moger <babu.moger@....com>
>> ---
> 
> ...
> 
>> ---
>>  Documentation/filesystems/resctrl.rst | 32 +++++++++++
>>  fs/resctrl/internal.h                 |  2 +
>>  fs/resctrl/monitor.c                  |  1 +
>>  fs/resctrl/rdtgroup.c                 | 78 +++++++++++++++++++++++++++
>>  4 files changed, 113 insertions(+)
>>
>> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
>> index 18de335e1ff8..b1db1a53db2a 100644
>> --- a/Documentation/filesystems/resctrl.rst
>> +++ b/Documentation/filesystems/resctrl.rst
>> @@ -310,6 +310,38 @@ with the following files:
>>  	  # cat /sys/fs/resctrl/info/L3_MON/available_mbm_cntrs
>>  	  0=30;1=30
>>  
>> +"event_configs":
>> +	Directory that exists when "mbm_event" mode is supported. Contains
> 
> ""mbm_event" mode" -> ""mbm_event" counter assignment mode"

Sure.

> 
>> +	sub-directory for each MBM event that can be assigned to a counter.
>> +
>> +	Two MBM events are supported by default: mbm_local_bytes and mbm_total_bytes.
>> +	Each MBM event's sub-directory contains a file named "event_filter" that is
>> +	used to view and modify which memory transactions the MBM event is configured
>> +	with.
>> +
>> +	List of memory transaction types supported:
>> +
>> +	==========================  ========================================================
>> +	Name			    Description
>> +	==========================  ========================================================
>> +	dirty_victim_writes_all     Dirty Victims from the QOS domain to all types of memory
>> +	remote_reads_slow_memory    Reads to slow memory in the non-local NUMA domain
>> +	local_reads_slow_memory     Reads to slow memory in the local NUMA domain
>> +	remote_non_temporal_writes  Non-temporal writes to non-local NUMA domain
>> +	local_non_temporal_writes   Non-temporal writes to local NUMA domain
>> +	remote_reads                Reads to memory in the non-local NUMA domain
>> +	local_reads                 Reads to memory in the local NUMA domain
>> +	==========================  ========================================================
>> +
>> +	For example::
>> +
>> +	  # cat /sys/fs/resctrl/info/L3_MON/event_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/event_configs/mbm_local_bytes/event_filter
>> +	  local_reads, local_non_temporal_writes, local_reads_slow_memory
>> +
> 
> Please let these examples match what the patch does wrt spacing.

Sure.

> 
>>  "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 84a136194d9a..ed0e3b695ad5 100644
>> --- a/fs/resctrl/internal.h
>> +++ b/fs/resctrl/internal.h
>> @@ -248,6 +248,8 @@ struct mbm_config_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 ef6ef58f180b..09a49029a800 100644
>> --- a/fs/resctrl/monitor.c
>> +++ b/fs/resctrl/monitor.c
>> @@ -956,6 +956,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 5fb6a9939e23..e2fa5e10c2dd 100644
>> --- a/fs/resctrl/rdtgroup.c
>> +++ b/fs/resctrl/rdtgroup.c
>> @@ -1909,6 +1909,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_config_values[i].val) {
>> +			if (sep)
>> +				seq_putc(seq, ',');
>> +			seq_printf(seq, "%s", mbm_config_values[i].name);
> 
> Taking a closer look I think we need to be more careful about how the
> code is organized. Ideally the monitoring related code and data should
> be located in fs/resctrl/monitor.c. Having event_filter_show() here is
> ok because of its use in res_common_files[]. Since it is monitoring related
> I expected its code/data to be in fs/resctrl/monitor.c, thus that
> mbm_config_values[] (mbm_transactions[]?) to be in fs/resctrl/monitor.c,
> (just like mon_event_all[]).

Sure. Moved mbm_transactions[] to monitor.c.

Defined it as extern in fs/resctrl/rdtgroup.c

extern struct mbm_transaction mbm_transactions[NUM_MBM_TRANSACTIONS];


> 
>> +			sep = true;
>> +		}
>> +	}
>> +	seq_putc(seq, '\n');
>> +
>> +	return 0;
>> +}
>> +
>>  /* rdtgroup information files for one cache resource. */
>>  static struct rftype res_common_files[] = {
>>  	{
>> @@ -2033,6 +2052,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,
>> @@ -2315,6 +2340,53 @@ static int rdtgroup_mkdir_info_resdir(void *priv, char *name,
>>  	return ret;
>>  }
>>  
>> +static int resctrl_mkdir_counter_configs(struct rdt_resource *r, char *name)
> 
> This can now be named resctrl_mkdir_event_configs()?

Sure.

> 
> Also, I cannot see where the struct rdt_resource parameter is used. It should not
> be removed though, as mentioned earlier it should be used to ensure
> to check the mon_evt::rid values so that only events associated with resource
> are considered.

Added the check.

mevt->rid != r->rid

> 
>> +{
>> +	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;
> 
> Needing to figure out this kn does not seem necessary. Can it not be
> provided via parameter instead?
> 
> For example, resctrl_mkdir_counter_configs() (rather resctrl_mkdir_event_configs())
> can be called from rdtgroup_mkdir_info_resdir(). I understand rdtgroup_mkdir_info_resdir()
> is also called for  struct resctrl_schema parameter but I think the fflags can be used
> to make the right decision. Something like:
> 
> 	rdtgroup_mkdir_info_resdir() {
> 		struct rdt_resource *r;
> 
> 		...
> 		if (fflags & RFTYPE_MON_INFO) {
> 			r = priv;
> 			if (r->mon.mbm_cntr_assignable) {
> 				ret = resctrl_mkdir_event_configs(kn_subdir, r);
> 				...
> 			}
> 		}
> 	}
> 
> What do you think?	

We can do that.  Need to test it. Hopefully its fine.
	
> 
>> +
>> +	kn_subdir = kernfs_create_dir(l3_mon_kn, "event_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;
>> +	}
>> +
>> +	for (mevt = &mon_event_all[0]; mevt < &mon_event_all[QOS_NUM_EVENTS]; mevt++) {
> 
> Here is a spot where the for_each_mon_event() should be used.

Sure.

> 
>> +		if (!mevt->enabled || !resctrl_is_mbm_event(mevt->evtid))
>> +			continue;
>> +
>> +		kn_subdir2 = kernfs_create_dir(kn_subdir, mevt->name, kn_subdir->mode, mevt);
>> +		if (IS_ERR(kn_subdir2)) {
>> +			ret = PTR_ERR(kn_subdir2);
>> +			goto out_config;
>> +		}
>> +
>> +		ret = rdtgroup_kn_set_ugid(kn_subdir2);
>> +		if (ret)
>> +			goto out_config;
>> +
>> +		ret = rdtgroup_add_files(kn_subdir2, RFTYPE_ASSIGN_CONFIG);
>> +		if (!ret)
>> +			kernfs_activate(kn_subdir);
>> +	}
>> +
>> +out_config:
>> +	kernfs_put(l3_mon_kn);
>> +
>> +	return ret;
>> +}
>> +
>>  static unsigned long fflags_from_resource(struct rdt_resource *r)
>>  {
>>  	switch (r->rid) {
>> @@ -2361,6 +2433,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

Powered by blists - more mailing lists