[PATCH v12 20/26] x86/resctrl: Provide interface to update the event configurations

Babu Moger posted 26 patches 8 months, 2 weeks ago
Only 25 patches received!
There is a newer version of this series
[PATCH v12 20/26] x86/resctrl: Provide interface to update the event configurations
Posted by Babu Moger 8 months, 2 weeks ago
Users can modify the event configuration by writing to the event_filter
interface file. The event configurations for mbm_cntr_assign mode are
located in /sys/fs/resctrl/info/event_configs/.

Update the assignments of all groups when the event configuration is
modified.

Example:
$ cd /sys/fs/resctrl/
$ echo "local_reads, local_non_temporal_writes" >
  info/L3_MON/counter_configs/mbm_total_bytes/event_filter

$ cat info/L3_MON/counter_configs/mbm_total_bytes/event_filter
 local_reads, local_non_temporal_writes

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
v12: New patch to modify event configurations.
---
 Documentation/arch/x86/resctrl.rst     |  10 +++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 115 ++++++++++++++++++++++++-
 2 files changed, 124 insertions(+), 1 deletion(-)

diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
index 99f9f4b9b501..4e6feba6fb08 100644
--- a/Documentation/arch/x86/resctrl.rst
+++ b/Documentation/arch/x86/resctrl.rst
@@ -335,6 +335,16 @@ with the following files:
 	    # cat /sys/fs/resctrl/info/L3_MON/counter_configs/mbm_local_bytes/event_filter
 	    local_reads, local_non_temporal_writes, local_reads_slow_memory
 
+	The event configuration can be modified by writing to the event_filter file within
+	the configuration directory.
+	::
+
+	    # 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/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index b2122a1dd36c..7792455f0b26 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1926,6 +1926,118 @@ static int event_filter_show(struct kernfs_open_file *of, struct seq_file *seq,
 	return 0;
 }
 
+static int resctrl_group_assign(struct rdt_resource *r, struct rdtgroup *rdtgrp,
+				enum resctrl_event_id evtid, u32 evt_cfg)
+{
+	struct rdt_mon_domain *d;
+	int cntr_id, ret;
+
+	list_for_each_entry(d, &r->mon_domains, hdr.list) {
+		cntr_id = mbm_cntr_get(r, d, rdtgrp, evtid);
+		if (cntr_id >= 0 && d->cntr_cfg[cntr_id].evt_cfg != evt_cfg) {
+			d->cntr_cfg[cntr_id].evt_cfg = evt_cfg;
+			ret = resctrl_arch_config_cntr(r, d, evtid, rdtgrp->mon.rmid,
+						       rdtgrp->closid, cntr_id, evt_cfg, true);
+			if (ret) {
+				rdt_last_cmd_printf("Assign failed event %d domain %d group %s\n",
+						    evtid, d->hdr.id, rdtgrp->kn->name);
+				return ret;
+			}
+		}
+	}
+
+	return 0;
+}
+
+static int resctrl_update_assign(struct rdt_resource *r, enum resctrl_event_id evtid,
+				 u32 evt_cfg)
+{
+	struct rdtgroup *prgrp, *crgrp;
+	int ret;
+
+	/* Check if the cntr_id is associated to the event type updated */
+	list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
+		ret = resctrl_group_assign(r, prgrp, evtid, evt_cfg);
+		if (ret)
+			return ret;
+
+		list_for_each_entry(crgrp, &prgrp->mon.crdtgrp_list, mon.crdtgrp_list) {
+			ret = resctrl_group_assign(r, crgrp, evtid, evt_cfg);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int resctrl_process_configs(char *tok, u32 *val)
+{
+	char *evt_str;
+	bool found;
+	int i;
+
+next_config:
+	if (!tok || tok[0] == '\0')
+		return 0;
+
+	/* Start processing the strings for each event type */
+	evt_str = strim(strsep(&tok, ","));
+	found = false;
+	for (i = 0; i < NUM_MBM_EVT_VALUES; i++) {
+		if (!strcmp(mbm_evt_values[i].evt_name, evt_str)) {
+			*val |=  mbm_evt_values[i].evt_val;
+			found = true;
+			break;
+		}
+	}
+
+	if (!found) {
+		rdt_last_cmd_printf("Invalid event type %s\n", evt_str);
+		return -EINVAL;
+	}
+
+	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 mbm_assign_config *assign_config = of->kn->parent->priv;
+	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 unlock_out;
+	}
+
+	ret = resctrl_process_configs(buf, &evt_cfg);
+	if (!ret && assign_config->val != evt_cfg) {
+		assign_config->val = evt_cfg;
+		ret = resctrl_update_assign(r, assign_config->evtid, evt_cfg);
+	}
+
+unlock_out:
+	mutex_unlock(&rdtgroup_mutex);
+	cpus_read_unlock();
+
+	return ret ?: nbytes;
+}
+
 /* rdtgroup information files for one cache resource. */
 static struct rftype res_common_files[] = {
 	{
@@ -2040,9 +2152,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
Re: [PATCH v12 20/26] x86/resctrl: Provide interface to update the event configurations
Posted by Reinette Chatre 8 months, 1 week ago
Hi Babu,

On 4/3/25 5:18 PM, Babu Moger wrote:
> Users can modify the event configuration by writing to the event_filter
> interface file. The event configurations for mbm_cntr_assign mode are
> located in /sys/fs/resctrl/info/event_configs/.
> 
> Update the assignments of all groups when the event configuration is
> modified.
> 
> Example:
> $ cd /sys/fs/resctrl/
> $ echo "local_reads, local_non_temporal_writes" >
>   info/L3_MON/counter_configs/mbm_total_bytes/event_filter
> 
> $ cat info/L3_MON/counter_configs/mbm_total_bytes/event_filter
>  local_reads, local_non_temporal_writes
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> v12: New patch to modify event configurations.
> ---
>  Documentation/arch/x86/resctrl.rst     |  10 +++
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 115 ++++++++++++++++++++++++-
>  2 files changed, 124 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
> index 99f9f4b9b501..4e6feba6fb08 100644
> --- a/Documentation/arch/x86/resctrl.rst
> +++ b/Documentation/arch/x86/resctrl.rst
> @@ -335,6 +335,16 @@ with the following files:
>  	    # cat /sys/fs/resctrl/info/L3_MON/counter_configs/mbm_local_bytes/event_filter
>  	    local_reads, local_non_temporal_writes, local_reads_slow_memory
>  
> +	The event configuration can be modified by writing to the event_filter file within
> +	the configuration directory.

Please use imperative tone.

> +	::
> +
> +	    # 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/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index b2122a1dd36c..7792455f0b26 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1926,6 +1926,118 @@ static int event_filter_show(struct kernfs_open_file *of, struct seq_file *seq,
>  	return 0;
>  }
>  

Could you please add comments to these new functions to explain what they do?

> +static int resctrl_group_assign(struct rdt_resource *r, struct rdtgroup *rdtgrp,
> +				enum resctrl_event_id evtid, u32 evt_cfg)
> +{
> +	struct rdt_mon_domain *d;
> +	int cntr_id, ret;
> +
> +	list_for_each_entry(d, &r->mon_domains, hdr.list) {
> +		cntr_id = mbm_cntr_get(r, d, rdtgrp, evtid);
> +		if (cntr_id >= 0 && d->cntr_cfg[cntr_id].evt_cfg != evt_cfg) {
> +			d->cntr_cfg[cntr_id].evt_cfg = evt_cfg;
> +			ret = resctrl_arch_config_cntr(r, d, evtid, rdtgrp->mon.rmid,
> +						       rdtgrp->closid, cntr_id, evt_cfg, true);
> +			if (ret) {
> +				rdt_last_cmd_printf("Assign failed event %d domain %d group %s\n",
> +						    evtid, d->hdr.id, rdtgrp->kn->name);

Please provide the actual event name to user space. The event IDs are not visible to
user space.

Reinette
Re: [PATCH v12 20/26] x86/resctrl: Provide interface to update the event configurations
Posted by Moger, Babu 8 months ago
Hi Reinette,

On 4/11/25 17:07, Reinette Chatre wrote:
> Hi Babu,
> 
> On 4/3/25 5:18 PM, Babu Moger wrote:
>> Users can modify the event configuration by writing to the event_filter
>> interface file. The event configurations for mbm_cntr_assign mode are
>> located in /sys/fs/resctrl/info/event_configs/.
>>
>> Update the assignments of all groups when the event configuration is
>> modified.
>>
>> Example:
>> $ cd /sys/fs/resctrl/
>> $ echo "local_reads, local_non_temporal_writes" >
>>   info/L3_MON/counter_configs/mbm_total_bytes/event_filter
>>
>> $ cat info/L3_MON/counter_configs/mbm_total_bytes/event_filter
>>  local_reads, local_non_temporal_writes
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v12: New patch to modify event configurations.
>> ---
>>  Documentation/arch/x86/resctrl.rst     |  10 +++
>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 115 ++++++++++++++++++++++++-
>>  2 files changed, 124 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
>> index 99f9f4b9b501..4e6feba6fb08 100644
>> --- a/Documentation/arch/x86/resctrl.rst
>> +++ b/Documentation/arch/x86/resctrl.rst
>> @@ -335,6 +335,16 @@ with the following files:
>>  	    # cat /sys/fs/resctrl/info/L3_MON/counter_configs/mbm_local_bytes/event_filter
>>  	    local_reads, local_non_temporal_writes, local_reads_slow_memory
>>  
>> +	The event configuration can be modified by writing to the event_filter file within
>> +	the configuration directory.
> 
> Please use imperative tone.

Sure.

Basic question - Should the user doc also be in imperative mode? I thought
it only applies to commit log.

> 
>> +	::
>> +
>> +	    # 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/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index b2122a1dd36c..7792455f0b26 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1926,6 +1926,118 @@ static int event_filter_show(struct kernfs_open_file *of, struct seq_file *seq,
>>  	return 0;
>>  }
>>  
> 
> Could you please add comments to these new functions to explain what they do?

Sure.

> 
>> +static int resctrl_group_assign(struct rdt_resource *r, struct rdtgroup *rdtgrp,
>> +				enum resctrl_event_id evtid, u32 evt_cfg)
>> +{
>> +	struct rdt_mon_domain *d;
>> +	int cntr_id, ret;
>> +
>> +	list_for_each_entry(d, &r->mon_domains, hdr.list) {
>> +		cntr_id = mbm_cntr_get(r, d, rdtgrp, evtid);
>> +		if (cntr_id >= 0 && d->cntr_cfg[cntr_id].evt_cfg != evt_cfg) {
>> +			d->cntr_cfg[cntr_id].evt_cfg = evt_cfg;
>> +			ret = resctrl_arch_config_cntr(r, d, evtid, rdtgrp->mon.rmid,
>> +						       rdtgrp->closid, cntr_id, evt_cfg, true);
>> +			if (ret) {
>> +				rdt_last_cmd_printf("Assign failed event %d domain %d group %s\n",
>> +						    evtid, d->hdr.id, rdtgrp->kn->name);
> 
> Please provide the actual event name to user space. The event IDs are not visible to
> user space.

Sure.

-- 
Thanks
Babu Moger
Re: [PATCH v12 20/26] x86/resctrl: Provide interface to update the event configurations
Posted by Reinette Chatre 8 months ago
Hi Babu,

On 4/15/25 1:37 PM, Moger, Babu wrote:
> Hi Reinette,
> 
> On 4/11/25 17:07, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 4/3/25 5:18 PM, Babu Moger wrote:
>>> Users can modify the event configuration by writing to the event_filter
>>> interface file. The event configurations for mbm_cntr_assign mode are
>>> located in /sys/fs/resctrl/info/event_configs/.
>>>
>>> Update the assignments of all groups when the event configuration is
>>> modified.
>>>
>>> Example:
>>> $ cd /sys/fs/resctrl/
>>> $ echo "local_reads, local_non_temporal_writes" >
>>>   info/L3_MON/counter_configs/mbm_total_bytes/event_filter
>>>
>>> $ cat info/L3_MON/counter_configs/mbm_total_bytes/event_filter
>>>  local_reads, local_non_temporal_writes
>>>
>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>>> ---
>>> v12: New patch to modify event configurations.
>>> ---
>>>  Documentation/arch/x86/resctrl.rst     |  10 +++
>>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 115 ++++++++++++++++++++++++-
>>>  2 files changed, 124 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
>>> index 99f9f4b9b501..4e6feba6fb08 100644
>>> --- a/Documentation/arch/x86/resctrl.rst
>>> +++ b/Documentation/arch/x86/resctrl.rst
>>> @@ -335,6 +335,16 @@ with the following files:
>>>  	    # cat /sys/fs/resctrl/info/L3_MON/counter_configs/mbm_local_bytes/event_filter
>>>  	    local_reads, local_non_temporal_writes, local_reads_slow_memory
>>>  
>>> +	The event configuration can be modified by writing to the event_filter file within
>>> +	the configuration directory.
>>
>> Please use imperative tone.
> 
> Sure.
> 
> Basic question - Should the user doc also be in imperative mode? I thought
> it only applies to commit log.

I am not aware of a documented rule that user doc should be in imperative mode. I
requested imperative tone here because writing in this way helps to remove ambiguity
and fits with how the rest of the resctrl files are described.

Looking at this specific addition I realized that there is no initial description of
what "event_filter" contains and to make things more confusing the term "event" is
used for both the individual "events" being counted (remote_reads, local_reads, etc.) as
well as the (what will eventually be dynamic) name for collection of "events" being counted,
mbm_total_bytes and mbm_local_bytes. 

Since "event" have been used for mbm_total_bytes and mbm_local_bytes since beginning we
should try to come up with term that can describe what they are configured with.

Below is a start of trying to address this but I think more refinement is needed (other
possible terms for "transactions" could perhaps be "data sources"? ... what do you think?):

	"The read/write event_filter file contains the configuration of the event
	 that reflects which transactions(?) are being counted by it."

Reinette
Re: [PATCH v12 20/26] x86/resctrl: Provide interface to update the event configurations
Posted by Moger, Babu 8 months ago
Hi Reinette,

On 4/16/25 13:52, Reinette Chatre wrote:
> Hi Babu,
> 
> On 4/15/25 1:37 PM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 4/11/25 17:07, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 4/3/25 5:18 PM, Babu Moger wrote:
>>>> Users can modify the event configuration by writing to the event_filter
>>>> interface file. The event configurations for mbm_cntr_assign mode are
>>>> located in /sys/fs/resctrl/info/event_configs/.
>>>>
>>>> Update the assignments of all groups when the event configuration is
>>>> modified.
>>>>
>>>> Example:
>>>> $ cd /sys/fs/resctrl/
>>>> $ echo "local_reads, local_non_temporal_writes" >
>>>>   info/L3_MON/counter_configs/mbm_total_bytes/event_filter
>>>>
>>>> $ cat info/L3_MON/counter_configs/mbm_total_bytes/event_filter
>>>>  local_reads, local_non_temporal_writes
>>>>
>>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>>>> ---
>>>> v12: New patch to modify event configurations.
>>>> ---
>>>>  Documentation/arch/x86/resctrl.rst     |  10 +++
>>>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 115 ++++++++++++++++++++++++-
>>>>  2 files changed, 124 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
>>>> index 99f9f4b9b501..4e6feba6fb08 100644
>>>> --- a/Documentation/arch/x86/resctrl.rst
>>>> +++ b/Documentation/arch/x86/resctrl.rst
>>>> @@ -335,6 +335,16 @@ with the following files:
>>>>  	    # cat /sys/fs/resctrl/info/L3_MON/counter_configs/mbm_local_bytes/event_filter
>>>>  	    local_reads, local_non_temporal_writes, local_reads_slow_memory
>>>>  
>>>> +	The event configuration can be modified by writing to the event_filter file within
>>>> +	the configuration directory.
>>>
>>> Please use imperative tone.
>>
>> Sure.
>>
>> Basic question - Should the user doc also be in imperative mode? I thought
>> it only applies to commit log.
> 
> I am not aware of a documented rule that user doc should be in imperative mode. I
> requested imperative tone here because writing in this way helps to remove ambiguity
> and fits with how the rest of the resctrl files are described.
> 
> Looking at this specific addition I realized that there is no initial description of
> what "event_filter" contains and to make things more confusing the term "event" is
> used for both the individual "events" being counted (remote_reads, local_reads, etc.) as
> well as the (what will eventually be dynamic) name for collection of "events" being counted,
> mbm_total_bytes and mbm_local_bytes. 
> 
> Since "event" have been used for mbm_total_bytes and mbm_local_bytes since beginning we
> should try to come up with term that can describe what they are configured with.
> 
> Below is a start of trying to address this but I think more refinement is needed (other
> possible terms for "transactions" could perhaps be "data sources"? ... what do you think?):
> 
> 	"The read/write event_filter file contains the configuration of the event
> 	 that reflects which transactions(?) are being counted by it."
> 

How about?

"The read/write event_filter file contains the configuration of the event
that reflects which memory transactions are being counted by it."


-- 
Thanks
Babu Moger
Re: [PATCH v12 20/26] x86/resctrl: Provide interface to update the event configurations
Posted by Reinette Chatre 8 months ago
Hi Babu,

On 4/17/25 7:34 AM, Moger, Babu wrote:
> On 4/16/25 13:52, Reinette Chatre wrote:
>>
>> Below is a start of trying to address this but I think more refinement is needed (other
>> possible terms for "transactions" could perhaps be "data sources"? ... what do you think?):
>>
>> 	"The read/write event_filter file contains the configuration of the event
>> 	 that reflects which transactions(?) are being counted by it."
>>
> 
> How about?
> 
> "The read/write event_filter file contains the configuration of the event
> that reflects which memory transactions are being counted by it."
> 

Looks good to me. Perhaps "being" can be dropped? Thank you.

Reinette
Re: [PATCH v12 20/26] x86/resctrl: Provide interface to update the event configurations
Posted by Moger, Babu 8 months ago
Hi Reinette,

On 4/17/25 10:09, Reinette Chatre wrote:
> Hi Babu,
> 
> On 4/17/25 7:34 AM, Moger, Babu wrote:
>> On 4/16/25 13:52, Reinette Chatre wrote:
>>>
>>> Below is a start of trying to address this but I think more refinement is needed (other
>>> possible terms for "transactions" could perhaps be "data sources"? ... what do you think?):
>>>
>>> 	"The read/write event_filter file contains the configuration of the event
>>> 	 that reflects which transactions(?) are being counted by it."
>>>
>>
>> How about?
>>
>> "The read/write event_filter file contains the configuration of the event
>> that reflects which memory transactions are being counted by it."
>>
> 
> Looks good to me. Perhaps "being" can be dropped? Thank you.
> 

Sure.

-- 
Thanks
Babu Moger