Message-ID: <35238a5e-fa04-4e08-97ba-7e206c36bf4d@amd.com>
Date: Mon, 30 Jun 2025 19:43:02 -0500
From: "Moger, Babu" <bmoger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>,
 Babu Moger <babu.moger@....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 25/32] fs/resctrl: Provide interface to update the
 event configurations

Hi Reinette,

On 6/25/2025 6:21 PM, Reinette Chatre wrote:
> 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"?

"users"

> 
>> configuration by writing to the 'event_filter' interface file. The event
> 
> nit: "interface file" -> "resctrl file"

Sure.

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

sure.
> 
>> 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@....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"

Sure.

> 
> Note that "event_filter" is not consistently in quotes.

Sure.

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

Sure.

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

Sure.

>> +	   local_reads, local_non_temporal_writes
> 
> Please let example match what code 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/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()?
> 

Sure.

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

Agree.

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

Yes. Taken care of this.

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

Sure.

> 
>> +}
>> +
>> +/**
>> + * 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()

resctrl_update_cntr_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"?

Find.

> 
> tyoe -> type?

Sure.

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

Yes. Changed it.

  if (!tok || tok[0] == '\0') {
                *val = temp_val;
                return 0;
  }


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

changed it

  r = resctrl_arch_get_resource(mevt->rid);

> Do any of these new functions need a struct rdt_resource parameter in addition
> to struct mon_evt?

We need to make a call resctrl_arch_mbm_cntr_assign_enabled(r)) to 
proceed. So we need  struct rdt_resource.

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

Sure.

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

Kept event_filter_write() and resctrl_process_configs() here. Moved 
other two functions to 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
> 

thanks
Babu

Powered by blists - more mailing lists