When "mbm_event" counter assignment mode is enabled, users can modify
the event configuration by writing to the 'event_filter' resctrl file.
The event configurations for mbm_event mode are located in
/sys/fs/resctrl/info/L3_MON/event_configs/.

Update the assignments of all CTRL_MON and MON resource groups when the
event configuration is 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>
---
v17: Minor changelog update.
     Cleared mbm_state on every assignment update.
     All the code moved monitor.c.

v16: Moved resctrl_process_configs() and event_filter_write()
     to fs/resctrl/monitor.c.
     Renamed resctrl_process_configs() -> resctrl_parse_mem_transactions().
     Few minor code commnet update.

v15: Updated changelog.
     Updated spacing in resctrl.rst.
     Corrected the name counter_configs -> event_configs.
     Changed the name rdtgroup_assign_cntr() > rdtgroup_update_cntr_event().
     Removed the code to check d->cntr_cfg[cntr_id].evt_cfg.
     Fixed the partial initialization of val in resctrl_process_configs().
     Passed mon_evt where applicable. The struct rdt_resource can be obtained from mon_evt::rid.

v14: Passed struct mon_evt where applicable instead of just the event type.
     Fixed few text corrections about memory trasaction type.
     Renamed few functions resctrl_group_assign() -> rdtgroup_assign_cntr()
     resctrl_update_assign() -> resctrl_assign_cntr_allrdtgrp()
     Removed few extra bases.

v13: Updated changelog for imperative mode.
     Added function description in the prototype.
     Updated the user doc resctrl.rst to address few feedback.
     Resolved conflicts caused by the recent FS/ARCH code restructure.
     The rdtgroup.c/monitor.c file has now been split between the FS and ARCH directories.

v12: New patch to modify event configurations.
---
 Documentation/filesystems/resctrl.rst |  12 +++
 fs/resctrl/internal.h                 |   3 +
 fs/resctrl/monitor.c                  | 119 ++++++++++++++++++++++++++
 fs/resctrl/rdtgroup.c                 |   3 +-
 4 files changed, 136 insertions(+), 1 deletion(-)

diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
index ddd95f1472e6..2e840ef26f68 100644
--- a/Documentation/filesystems/resctrl.rst
+++ b/Documentation/filesystems/resctrl.rst
@@ -343,6 +343,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 "event_configs" directory. The read/write "event_filter" file contains the
+	configuration 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/event_configs/mbm_total_bytes/event_filter
+
+	  # cat /sys/fs/resctrl/info/L3_MON/event_configs/mbm_total_bytes/event_filter
+	   local_reads,local_non_temporal_writes
+
 "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 7b1206fff116..5956570d49fc 100644
--- a/fs/resctrl/internal.h
+++ b/fs/resctrl/internal.h
@@ -407,6 +407,9 @@ void rdtgroup_unassign_cntrs(struct rdtgroup *rdtgrp);
 
 int event_filter_show(struct kernfs_open_file *of, struct seq_file *seq, void *v);
 
+ssize_t event_filter_write(struct kernfs_open_file *of, char *buf, size_t nbytes,
+			   loff_t off);
+
 #ifdef CONFIG_RESCTRL_FS_PSEUDO_LOCK
 int rdtgroup_locksetup_enter(struct rdtgroup *rdtgrp);
 
diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
index 25fec9bf2d61..9201fedd2796 100644
--- a/fs/resctrl/monitor.c
+++ b/fs/resctrl/monitor.c
@@ -1029,6 +1029,125 @@ int event_filter_show(struct kernfs_open_file *of, struct seq_file *seq, void *v
 	return ret;
 }
 
+static int resctrl_parse_mem_transactions(char *tok, u32 *val)
+{
+	u32 temp_val = 0;
+	char *evt_str;
+	bool found;
+	int i;
+
+next_config:
+	if (!tok || tok[0] == '\0') {
+		*val = temp_val;
+		return 0;
+	}
+
+	/* Start processing the strings for each memory transaction type */
+	evt_str = strim(strsep(&tok, ","));
+	found = false;
+	for (i = 0; i < NUM_MBM_TRANSACTIONS; i++) {
+		if (!strcmp(mbm_transactions[i].name, evt_str)) {
+			temp_val |= mbm_transactions[i].val;
+			found = true;
+			break;
+		}
+	}
+
+	if (!found) {
+		rdt_last_cmd_printf("Invalid memory transaction type %s\n", evt_str);
+		return -EINVAL;
+	}
+
+	goto next_config;
+}
+
+/*
+ * rdtgroup_update_cntr_event - Update the counter assignments for the event
+ *				in a group.
+ * @r:		Resource to which update needs to be done.
+ * @rdtgrp:	Resctrl group.
+ * @evtid:	MBM monitor event.
+ */
+static void rdtgroup_update_cntr_event(struct rdt_resource *r, struct rdtgroup *rdtgrp,
+				       enum resctrl_event_id evtid)
+{
+	struct rdt_mon_domain *d;
+	struct mbm_state *m;
+	int cntr_id;
+
+	list_for_each_entry(d, &r->mon_domains, hdr.list) {
+		cntr_id = mbm_cntr_get(r, d, rdtgrp, evtid);
+		if (cntr_id >= 0) {
+			resctrl_arch_config_cntr(r, d, evtid, rdtgrp->mon.rmid,
+						 rdtgrp->closid, cntr_id, true);
+			m = get_mbm_state(d, rdtgrp->closid, rdtgrp->mon.rmid, evtid);
+			if (m)
+				memset(m, 0, sizeof(*m));
+		}
+	}
+}
+
+/*
+ * resctrl_update_cntr_allrdtgrp - Update the counter assignments for the event
+ *				   for all the groups.
+ * @mevt	MBM Monitor event.
+ */
+static void resctrl_update_cntr_allrdtgrp(struct mon_evt *mevt)
+{
+	struct rdt_resource *r = resctrl_arch_get_resource(mevt->rid);
+	struct rdtgroup *prgrp, *crgrp;
+
+	/*
+	 * Find all the groups where the event is assigned and update the
+	 * configuration of existing assignments.
+	 */
+	list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
+		rdtgroup_update_cntr_event(r, prgrp, mevt->evtid);
+
+		list_for_each_entry(crgrp, &prgrp->mon.crdtgrp_list, mon.crdtgrp_list)
+			rdtgroup_update_cntr_event(r, crgrp, mevt->evtid);
+	}
+}
+
+ssize_t event_filter_write(struct kernfs_open_file *of, char *buf, size_t nbytes,
+			   loff_t off)
+{
+	struct mon_evt *mevt = rdt_kn_parent_priv(of->kn);
+	struct rdt_resource *r;
+	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();
+
+	r = resctrl_arch_get_resource(mevt->rid);
+	if (!resctrl_arch_mbm_cntr_assign_enabled(r)) {
+		rdt_last_cmd_puts("mbm_event counter assignment mode is not enabled\n");
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
+	ret = resctrl_parse_mem_transactions(buf, &evt_cfg);
+	if (!ret && mevt->evt_cfg != evt_cfg) {
+		mevt->evt_cfg = evt_cfg;
+		resctrl_update_cntr_allrdtgrp(mevt);
+	}
+
+out_unlock:
+	mutex_unlock(&rdtgroup_mutex);
+	cpus_read_unlock();
+
+	return ret ?: nbytes;
+}
+
 /*
  * rdtgroup_assign_cntr() - Assign/unassign the counter ID for the event, RMID
  * pair in the domain.
diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
index 25a653847f49..8187df7b85d2 100644
--- a/fs/resctrl/rdtgroup.c
+++ b/fs/resctrl/rdtgroup.c
@@ -1925,9 +1925,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",
-- 
2.34.1

Hi Babu,

On 8/14/25 7:25 PM, Babu Moger wrote:

> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index 25fec9bf2d61..9201fedd2796 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -1029,6 +1029,125 @@ int event_filter_show(struct kernfs_open_file *of, struct seq_file *seq, void *v
>  	return ret;
>  }
>  
> +static int resctrl_parse_mem_transactions(char *tok, u32 *val)
> +{
> +	u32 temp_val = 0;
> +	char *evt_str;
> +	bool found;
> +	int i;
> +
> +next_config:
> +	if (!tok || tok[0] == '\0') {
> +		*val = temp_val;
> +		return 0;
> +	}

Looks like resctrl_parse_mem_transactions() can return "success" with a parsed
return value of "0" (*val = 0) ... (follow-up comment in event_filter_write()).

> +
> +	/* Start processing the strings for each memory transaction type */
> +	evt_str = strim(strsep(&tok, ","));
> +	found = false;
> +	for (i = 0; i < NUM_MBM_TRANSACTIONS; i++) {
> +		if (!strcmp(mbm_transactions[i].name, evt_str)) {
> +			temp_val |= mbm_transactions[i].val;
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	if (!found) {
> +		rdt_last_cmd_printf("Invalid memory transaction type %s\n", evt_str);
> +		return -EINVAL;
> +	}
> +
> +	goto next_config;
> +}
> +
> +/*
> + * rdtgroup_update_cntr_event - Update the counter assignments for the event
> + *				in a group.
> + * @r:		Resource to which update needs to be done.
> + * @rdtgrp:	Resctrl group.
> + * @evtid:	MBM monitor event.
> + */
> +static void rdtgroup_update_cntr_event(struct rdt_resource *r, struct rdtgroup *rdtgrp,
> +				       enum resctrl_event_id evtid)
> +{
> +	struct rdt_mon_domain *d;
> +	struct mbm_state *m;
> +	int cntr_id;
> +
> +	list_for_each_entry(d, &r->mon_domains, hdr.list) {
> +		cntr_id = mbm_cntr_get(r, d, rdtgrp, evtid);
> +		if (cntr_id >= 0) {
> +			resctrl_arch_config_cntr(r, d, evtid, rdtgrp->mon.rmid,
> +						 rdtgrp->closid, cntr_id, true);
> +			m = get_mbm_state(d, rdtgrp->closid, rdtgrp->mon.rmid, evtid);
> +			if (m)
> +				memset(m, 0, sizeof(*m));

This looks like open code of rdtgroup_assign_cntr()?

> +		}
> +	}
> +}
> +

...

> +
> +ssize_t event_filter_write(struct kernfs_open_file *of, char *buf, size_t nbytes,
> +			   loff_t off)
> +{
> +	struct mon_evt *mevt = rdt_kn_parent_priv(of->kn);
> +	struct rdt_resource *r;
> +	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();
> +
> +	r = resctrl_arch_get_resource(mevt->rid);
> +	if (!resctrl_arch_mbm_cntr_assign_enabled(r)) {
> +		rdt_last_cmd_puts("mbm_event counter assignment mode is not enabled\n");
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +	ret = resctrl_parse_mem_transactions(buf, &evt_cfg);
> +	if (!ret && mevt->evt_cfg != evt_cfg) {

... is evt_cfg of 0 (a) a valid value (that will not cause hardware to fault) and
(b) a reasonable value to allow? 

Reinette

Hi Reinette,

Thanks for the review of series.

On 9/2/25 21:41, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/14/25 7:25 PM, Babu Moger wrote:
> 
>> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
>> index 25fec9bf2d61..9201fedd2796 100644
>> --- a/fs/resctrl/monitor.c
>> +++ b/fs/resctrl/monitor.c
>> @@ -1029,6 +1029,125 @@ int event_filter_show(struct kernfs_open_file *of, struct seq_file *seq, void *v
>>  	return ret;
>>  }
>>  
>> +static int resctrl_parse_mem_transactions(char *tok, u32 *val)
>> +{
>> +	u32 temp_val = 0;
>> +	char *evt_str;
>> +	bool found;
>> +	int i;
>> +
>> +next_config:
>> +	if (!tok || tok[0] == '\0') {
>> +		*val = temp_val;
>> +		return 0;
>> +	}
> 
> Looks like resctrl_parse_mem_transactions() can return "success" with a parsed
> return value of "0" (*val = 0) ... (follow-up comment in event_filter_write()).

Yes. *val=0 is a valid value.

> 
>> +
>> +	/* Start processing the strings for each memory transaction type */
>> +	evt_str = strim(strsep(&tok, ","));
>> +	found = false;
>> +	for (i = 0; i < NUM_MBM_TRANSACTIONS; i++) {
>> +		if (!strcmp(mbm_transactions[i].name, evt_str)) {
>> +			temp_val |= mbm_transactions[i].val;
>> +			found = true;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (!found) {
>> +		rdt_last_cmd_printf("Invalid memory transaction type %s\n", evt_str);
>> +		return -EINVAL;
>> +	}
>> +
>> +	goto next_config;
>> +}
>> +
>> +/*
>> + * rdtgroup_update_cntr_event - Update the counter assignments for the event
>> + *				in a group.
>> + * @r:		Resource to which update needs to be done.
>> + * @rdtgrp:	Resctrl group.
>> + * @evtid:	MBM monitor event.
>> + */
>> +static void rdtgroup_update_cntr_event(struct rdt_resource *r, struct rdtgroup *rdtgrp,
>> +				       enum resctrl_event_id evtid)
>> +{
>> +	struct rdt_mon_domain *d;
>> +	struct mbm_state *m;
>> +	int cntr_id;
>> +
>> +	list_for_each_entry(d, &r->mon_domains, hdr.list) {
>> +		cntr_id = mbm_cntr_get(r, d, rdtgrp, evtid);
>> +		if (cntr_id >= 0) {
>> +			resctrl_arch_config_cntr(r, d, evtid, rdtgrp->mon.rmid,
>> +						 rdtgrp->closid, cntr_id, true);
>> +			m = get_mbm_state(d, rdtgrp->closid, rdtgrp->mon.rmid, evtid);
>> +			if (m)
>> +				memset(m, 0, sizeof(*m));
> 
> This looks like open code of rdtgroup_assign_cntr()?

Will call rdtgroup_assign_cntr() directly here.

> 
>> +		}
>> +	}
>> +}
>> +
> 
> ...
> 
>> +
>> +ssize_t event_filter_write(struct kernfs_open_file *of, char *buf, size_t nbytes,
>> +			   loff_t off)
>> +{
>> +	struct mon_evt *mevt = rdt_kn_parent_priv(of->kn);
>> +	struct rdt_resource *r;
>> +	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();
>> +
>> +	r = resctrl_arch_get_resource(mevt->rid);
>> +	if (!resctrl_arch_mbm_cntr_assign_enabled(r)) {
>> +		rdt_last_cmd_puts("mbm_event counter assignment mode is not enabled\n");
>> +		ret = -EINVAL;
>> +		goto out_unlock;
>> +	}
>> +
>> +	ret = resctrl_parse_mem_transactions(buf, &evt_cfg);
>> +	if (!ret && mevt->evt_cfg != evt_cfg) {
> 
> ... is evt_cfg of 0 (a) a valid value (that will not cause hardware to fault) and
> (b) a reasonable value to allow? 
> 

The value evt_cfg = 0 is valid and permitted for both ABMC and BMEC. I
have confirmed here through verification and testing. In that case, the
event counter will not be monitoring anything.
-- 
Thanks
Babu Moger

Hi Babu,

On 9/3/25 10:38 AM, Moger, Babu wrote:
> On 9/2/25 21:41, Reinette Chatre wrote:
>> On 8/14/25 7:25 PM, Babu Moger wrote:

>>> +
>>> +ssize_t event_filter_write(struct kernfs_open_file *of, char *buf, size_t nbytes,
>>> +			   loff_t off)
>>> +{
>>> +	struct mon_evt *mevt = rdt_kn_parent_priv(of->kn);
>>> +	struct rdt_resource *r;
>>> +	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();
>>> +
>>> +	r = resctrl_arch_get_resource(mevt->rid);
>>> +	if (!resctrl_arch_mbm_cntr_assign_enabled(r)) {
>>> +		rdt_last_cmd_puts("mbm_event counter assignment mode is not enabled\n");
>>> +		ret = -EINVAL;
>>> +		goto out_unlock;
>>> +	}
>>> +
>>> +	ret = resctrl_parse_mem_transactions(buf, &evt_cfg);
>>> +	if (!ret && mevt->evt_cfg != evt_cfg) {
>>
>> ... is evt_cfg of 0 (a) a valid value (that will not cause hardware to fault) and
>> (b) a reasonable value to allow? 
>>
> 
> The value evt_cfg = 0 is valid and permitted for both ABMC and BMEC. I
> have confirmed here through verification and testing. In that case, the
> event counter will not be monitoring anything.

Thank you for checking.

I do not know what a use case for this would be but I also do not see why kernel
should prevent user space from doing this. Looks like event_filter_show() will then
just print a '\n' that is expected. Are counters expected to always return 0 in this
case or will they return an error? I am not clear on what qualifies as "invalid counter
configuration" for ABMC that results in RMID_VAL_ERROR.

Reinette

Hi Reinette,

On 9/3/25 12:55, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/3/25 10:38 AM, Moger, Babu wrote:
>> On 9/2/25 21:41, Reinette Chatre wrote:
>>> On 8/14/25 7:25 PM, Babu Moger wrote:
> 
>>>> +
>>>> +ssize_t event_filter_write(struct kernfs_open_file *of, char *buf, size_t nbytes,
>>>> +			   loff_t off)
>>>> +{
>>>> +	struct mon_evt *mevt = rdt_kn_parent_priv(of->kn);
>>>> +	struct rdt_resource *r;
>>>> +	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();
>>>> +
>>>> +	r = resctrl_arch_get_resource(mevt->rid);
>>>> +	if (!resctrl_arch_mbm_cntr_assign_enabled(r)) {
>>>> +		rdt_last_cmd_puts("mbm_event counter assignment mode is not enabled\n");
>>>> +		ret = -EINVAL;
>>>> +		goto out_unlock;
>>>> +	}
>>>> +
>>>> +	ret = resctrl_parse_mem_transactions(buf, &evt_cfg);
>>>> +	if (!ret && mevt->evt_cfg != evt_cfg) {
>>>
>>> ... is evt_cfg of 0 (a) a valid value (that will not cause hardware to fault) and
>>> (b) a reasonable value to allow? 
>>>
>>
>> The value evt_cfg = 0 is valid and permitted for both ABMC and BMEC. I
>> have confirmed here through verification and testing. In that case, the
>> event counter will not be monitoring anything.
> 
> Thank you for checking.
> 
> I do not know what a use case for this would be but I also do not see why kernel
> should prevent user space from doing this. Looks like event_filter_show() will then
> just print a '\n' that is expected. Are counters expected to always return 0 in this
> case or will they return an error? I am not clear on what qualifies as "invalid counter
> configuration" for ABMC that results in RMID_VAL_ERROR.

The event counters return 0 in this case. I would not think this as an
invalid event configuration.

One option is to add a text in last_cmd_status from event_filter_show().

rdt_last_cmd_printf("Event %s is not configured to monitor any memory
transactions\n", mevt->name);
-- 
Thanks
Babu Moger

Hi Babu,

On 9/3/25 11:32 AM, Moger, Babu wrote:
> Hi Reinette,
> 
> On 9/3/25 12:55, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 9/3/25 10:38 AM, Moger, Babu wrote:
>>> On 9/2/25 21:41, Reinette Chatre wrote:
>>>> On 8/14/25 7:25 PM, Babu Moger wrote:
>>
>>>>> +
>>>>> +ssize_t event_filter_write(struct kernfs_open_file *of, char *buf, size_t nbytes,
>>>>> +			   loff_t off)
>>>>> +{
>>>>> +	struct mon_evt *mevt = rdt_kn_parent_priv(of->kn);
>>>>> +	struct rdt_resource *r;
>>>>> +	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();
>>>>> +
>>>>> +	r = resctrl_arch_get_resource(mevt->rid);
>>>>> +	if (!resctrl_arch_mbm_cntr_assign_enabled(r)) {
>>>>> +		rdt_last_cmd_puts("mbm_event counter assignment mode is not enabled\n");
>>>>> +		ret = -EINVAL;
>>>>> +		goto out_unlock;
>>>>> +	}
>>>>> +
>>>>> +	ret = resctrl_parse_mem_transactions(buf, &evt_cfg);
>>>>> +	if (!ret && mevt->evt_cfg != evt_cfg) {
>>>>
>>>> ... is evt_cfg of 0 (a) a valid value (that will not cause hardware to fault) and
>>>> (b) a reasonable value to allow? 
>>>>
>>>
>>> The value evt_cfg = 0 is valid and permitted for both ABMC and BMEC. I
>>> have confirmed here through verification and testing. In that case, the
>>> event counter will not be monitoring anything.
>>
>> Thank you for checking.
>>
>> I do not know what a use case for this would be but I also do not see why kernel
>> should prevent user space from doing this. Looks like event_filter_show() will then
>> just print a '\n' that is expected. Are counters expected to always return 0 in this
>> case or will they return an error? I am not clear on what qualifies as "invalid counter
>> configuration" for ABMC that results in RMID_VAL_ERROR.
> 
> The event counters return 0 in this case. I would not think this as an
> invalid event configuration.
> 
> One option is to add a text in last_cmd_status from event_filter_show().
> 
> rdt_last_cmd_printf("Event %s is not configured to monitor any memory
> transactions\n", mevt->name);


event_filter_show() does not return an error in this case so a user may not
check last_cmd_status since the doc specifies that "If the command was successful,
it will read as "ok"." Since the write() succeeded we should not fail the associated
show() to prompt the user to check last_cmd_status though.

Even so, an additional message does not seem necessary to me because as I understand
event_filter_show() returns an empty (just a '\n') file that I think
already reflects that no memory transactions are configured. It remains an awkward
use case to me but from what you stated this is valid for the hardware, it is what the
user requested, and the user can use interface to confirm the state. It does not seem
like changes are needed for this scenario.

Reinette

Hi Reinette,

On 9/3/2025 3:52 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 9/3/25 11:32 AM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 9/3/25 12:55, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 9/3/25 10:38 AM, Moger, Babu wrote:
>>>> On 9/2/25 21:41, Reinette Chatre wrote:
>>>>> On 8/14/25 7:25 PM, Babu Moger wrote:
>>>>>> +
>>>>>> +ssize_t event_filter_write(struct kernfs_open_file *of, char *buf, size_t nbytes,
>>>>>> +			   loff_t off)
>>>>>> +{
>>>>>> +	struct mon_evt *mevt = rdt_kn_parent_priv(of->kn);
>>>>>> +	struct rdt_resource *r;
>>>>>> +	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();
>>>>>> +
>>>>>> +	r = resctrl_arch_get_resource(mevt->rid);
>>>>>> +	if (!resctrl_arch_mbm_cntr_assign_enabled(r)) {
>>>>>> +		rdt_last_cmd_puts("mbm_event counter assignment mode is not enabled\n");
>>>>>> +		ret = -EINVAL;
>>>>>> +		goto out_unlock;
>>>>>> +	}
>>>>>> +
>>>>>> +	ret = resctrl_parse_mem_transactions(buf, &evt_cfg);
>>>>>> +	if (!ret && mevt->evt_cfg != evt_cfg) {
>>>>> ... is evt_cfg of 0 (a) a valid value (that will not cause hardware to fault) and
>>>>> (b) a reasonable value to allow?
>>>>>
>>>> The value evt_cfg = 0 is valid and permitted for both ABMC and BMEC. I
>>>> have confirmed here through verification and testing. In that case, the
>>>> event counter will not be monitoring anything.
>>> Thank you for checking.
>>>
>>> I do not know what a use case for this would be but I also do not see why kernel
>>> should prevent user space from doing this. Looks like event_filter_show() will then
>>> just print a '\n' that is expected. Are counters expected to always return 0 in this
>>> case or will they return an error? I am not clear on what qualifies as "invalid counter
>>> configuration" for ABMC that results in RMID_VAL_ERROR.
>> The event counters return 0 in this case. I would not think this as an
>> invalid event configuration.
>>
>> One option is to add a text in last_cmd_status from event_filter_show().
>>
>> rdt_last_cmd_printf("Event %s is not configured to monitor any memory
>> transactions\n", mevt->name);
>
> event_filter_show() does not return an error in this case so a user may not
> check last_cmd_status since the doc specifies that "If the command was successful,
> it will read as "ok"." Since the write() succeeded we should not fail the associated
> show() to prompt the user to check last_cmd_status though.
>
> Even so, an additional message does not seem necessary to me because as I understand
> event_filter_show() returns an empty (just a '\n') file that I think
> already reflects that no memory transactions are configured. It remains an awkward
> use case to me but from what you stated this is valid for the hardware, it is what the
> user requested, and the user can use interface to confirm the state. It does not seem
> like changes are needed for this scenario.

Sure.  Sounds good.

Thanks

Babu