When assignable counters are supported the users can modify the event
configuration by writing to the 'event_filter' interface file. The event
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
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>
---
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/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
+ 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
+
+ # cat /sys/fs/resctrl/info/L3_MON/counter_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/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
+ * 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;
+ resctrl_arch_config_cntr(r, d, mevt->evtid, rdtgrp->mon.rmid,
+ rdtgrp->closid, cntr_id, true);
+ }
+ }
+
+ return 0;
+}
+
+/**
+ * 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)
+{
+ struct rdtgroup *prgrp, *crgrp;
+
+ /*
+ * Check all the groups where the event tyoe is assigned and update
+ * 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;
+
+ 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);
+ 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");
+ 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);
+ }
+
+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",
--
2.34.1Hi 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"?
> configuration by writing to the 'event_filter' interface file. The event
nit: "interface file" -> "resctrl file"
> 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"
> 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>
> ---
...
> ---
> 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"
Note that "event_filter" is not consistently in quotes.
> + 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
> +
> + # cat /sys/fs/resctrl/info/L3_MON/counter_configs/mbm_total_bytes/event_filter
counter_configs -> event_configs
> + local_reads, local_non_temporal_writes
Please let example match what code does wrt spacing.
> +
> "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()?
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.
> + * 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/
> + 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.
> +}
> +
> +/**
> + * 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()
> +{
> + 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"?
tyoe -> type?
> + * 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.
> +
> + 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?
Do any of these new functions need a struct rdt_resource parameter in addition
to struct mon_evt?
> + 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.
> + 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?
> + }
> +
> +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
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@amd.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
Hi Babu,
On 6/30/25 5:43 PM, Moger, Babu wrote:
> On 6/25/2025 6:21 PM, Reinette Chatre wrote:
>> On 6/13/25 2:05 PM, Babu Moger wrote:
...
>>> + * 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;
> }
You may just not have included this in your snippet, but please ensure temp_val is always
initialized. Just this snippet on top of original patch risks using uninitialized variable.
>>> +
>>> + 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.
Understood, but since struct rdt_resource can be determined from mon_evt::rid
it is not obvious to me that providing both is always needed by all these functions.
Reinette
Hi Reinette,
On 6/30/25 20:33, Reinette Chatre wrote:
> Hi Babu,
>
> On 6/30/25 5:43 PM, Moger, Babu wrote:
>> On 6/25/2025 6:21 PM, Reinette Chatre wrote:
>>> On 6/13/25 2:05 PM, Babu Moger wrote:
>
> ...
>
>>>> + * 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;
>> }
>
> You may just not have included this in your snippet, but please ensure temp_val is always
> initialized. Just this snippet on top of original patch risks using uninitialized variable.
Yes. Got it. Should have pasted the full change. Its taken care already.
>
>>>> +
>>>> + 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.
>
> Understood, but since struct rdt_resource can be determined from mon_evt::rid
> it is not obvious to me that providing both is always needed by all these functions.
>
Yes. Got it. Taken care of this.
--
Thanks
Babu Moger
© 2016 - 2025 Red Hat, Inc.