[PATCH v10 11/24] x86/resctrl: Remove MSR reading of event configuration value

Babu Moger posted 24 patches 1 year ago
There is a newer version of this series
[PATCH v10 11/24] x86/resctrl: Remove MSR reading of event configuration value
Posted by Babu Moger 1 year ago
The event configuration is domain specific and initialized during domain
initialization. The values are stored in struct rdt_hw_mon_domain.

It is not required to read the configuration register every time user asks
for it. Use the value stored in struct rdt_hw_mon_domain instead.

Introduce resctrl_arch_mon_event_config_get() and
resctrl_arch_mon_event_config_set() to get/set architecture domain specific
mbm_total_cfg/mbm_local_cfg values.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
v10: Moved the mon_config_info structure definition to resctrl.h.

v9: Removed QOS_L3_OCCUP_EVENT_ID switch case in resctrl_arch_mon_event_config_set.
    Fixed a unnecessary space.

v8: Renamed
    resctrl_arch_event_config_get() to resctrl_arch_mon_event_config_get().
    resctrl_arch_event_config_set() to resctrl_arch_mon_event_config_set().

v7: Removed check if (val == INVALID_CONFIG_VALUE) as resctrl_arch_event_config_get
    already prints warning.
    Kept the Event config value definitions as is.

v6: Fixed inconstancy with types. Made all the types to u32 for config
    value.
    Removed few rdt_last_cmd_puts as it is not necessary.
    Removed unused config value definitions.
    Few more updates to commit message.

v5: Introduced resctrl_arch_event_config_get and
    resctrl_arch_event_config_get() based on our discussion.
    https://lore.kernel.org/lkml/68e861f9-245d-4496-a72e-46fc57d19c62@amd.com/

v4: New patch.
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 105 +++++++++++++------------
 include/linux/resctrl.h                |  16 ++++
 2 files changed, 72 insertions(+), 49 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 541bd353c567..682f47e0beb1 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1577,10 +1577,51 @@ static int rdtgroup_size_show(struct kernfs_open_file *of,
 	return ret;
 }
 
-struct mon_config_info {
-	u32 evtid;
-	u32 mon_config;
-};
+u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
+				      enum resctrl_event_id eventid)
+{
+	struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
+
+	switch (eventid) {
+	case QOS_L3_OCCUP_EVENT_ID:
+		break;
+	case QOS_L3_MBM_TOTAL_EVENT_ID:
+		return hw_dom->mbm_total_cfg;
+	case QOS_L3_MBM_LOCAL_EVENT_ID:
+		return hw_dom->mbm_local_cfg;
+	}
+
+	/* Never expect to get here */
+	WARN_ON_ONCE(1);
+
+	return INVALID_CONFIG_VALUE;
+}
+
+void resctrl_arch_mon_event_config_set(void *info)
+{
+	struct mon_config_info *mon_info = info;
+	struct rdt_hw_mon_domain *hw_dom;
+	unsigned int index;
+
+	index = mon_event_config_index_get(mon_info->evtid);
+	if (index == INVALID_CONFIG_INDEX)
+		return;
+
+	wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
+
+	hw_dom = resctrl_to_arch_mon_dom(mon_info->d);
+
+	switch (mon_info->evtid) {
+	case QOS_L3_MBM_TOTAL_EVENT_ID:
+		hw_dom->mbm_total_cfg = mon_info->mon_config;
+		break;
+	case QOS_L3_MBM_LOCAL_EVENT_ID:
+		hw_dom->mbm_local_cfg = mon_info->mon_config;
+		break;
+	default:
+		break;
+	}
+}
 
 /**
  * mon_event_config_index_get - get the hardware index for the
@@ -1604,33 +1645,11 @@ unsigned int mon_event_config_index_get(u32 evtid)
 	}
 }
 
-static void mon_event_config_read(void *info)
-{
-	struct mon_config_info *mon_info = info;
-	unsigned int index;
-	u64 msrval;
-
-	index = mon_event_config_index_get(mon_info->evtid);
-	if (index == INVALID_CONFIG_INDEX) {
-		pr_warn_once("Invalid event id %d\n", mon_info->evtid);
-		return;
-	}
-	rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
-
-	/* Report only the valid event configuration bits */
-	mon_info->mon_config = msrval & MAX_EVT_CONFIG_BITS;
-}
-
-static void mondata_config_read(struct rdt_mon_domain *d, struct mon_config_info *mon_info)
-{
-	smp_call_function_any(&d->hdr.cpu_mask, mon_event_config_read, mon_info, 1);
-}
-
 static int mbm_config_show(struct seq_file *s, struct rdt_resource *r, u32 evtid)
 {
-	struct mon_config_info mon_info;
 	struct rdt_mon_domain *dom;
 	bool sep = false;
+	u32 val;
 
 	cpus_read_lock();
 	mutex_lock(&rdtgroup_mutex);
@@ -1639,11 +1658,8 @@ static int mbm_config_show(struct seq_file *s, struct rdt_resource *r, u32 evtid
 		if (sep)
 			seq_puts(s, ";");
 
-		memset(&mon_info, 0, sizeof(struct mon_config_info));
-		mon_info.evtid = evtid;
-		mondata_config_read(dom, &mon_info);
-
-		seq_printf(s, "%d=0x%02x", dom->hdr.id, mon_info.mon_config);
+		val = resctrl_arch_mon_event_config_get(dom, evtid);
+		seq_printf(s, "%d=0x%02x", dom->hdr.id, val);
 		sep = true;
 	}
 	seq_puts(s, "\n");
@@ -1674,33 +1690,23 @@ static int mbm_local_bytes_config_show(struct kernfs_open_file *of,
 	return 0;
 }
 
-static void mon_event_config_write(void *info)
-{
-	struct mon_config_info *mon_info = info;
-	unsigned int index;
-
-	index = mon_event_config_index_get(mon_info->evtid);
-	if (index == INVALID_CONFIG_INDEX) {
-		pr_warn_once("Invalid event id %d\n", mon_info->evtid);
-		return;
-	}
-	wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
-}
 
 static void mbm_config_write_domain(struct rdt_resource *r,
 				    struct rdt_mon_domain *d, u32 evtid, u32 val)
 {
 	struct mon_config_info mon_info = {0};
+	u32 config_val;
 
 	/*
-	 * Read the current config value first. If both are the same then
+	 * Check the current config value first. If both are the same then
 	 * no need to write it again.
 	 */
-	mon_info.evtid = evtid;
-	mondata_config_read(d, &mon_info);
-	if (mon_info.mon_config == val)
+	config_val = resctrl_arch_mon_event_config_get(d, evtid);
+	if (config_val == INVALID_CONFIG_VALUE || config_val == val)
 		return;
 
+	mon_info.d = d;
+	mon_info.evtid = evtid;
 	mon_info.mon_config = val;
 
 	/*
@@ -1709,7 +1715,8 @@ static void mbm_config_write_domain(struct rdt_resource *r,
 	 * are scoped at the domain level. Writing any of these MSRs
 	 * on one CPU is observed by all the CPUs in the domain.
 	 */
-	smp_call_function_any(&d->hdr.cpu_mask, mon_event_config_write,
+	smp_call_function_any(&d->hdr.cpu_mask,
+			      resctrl_arch_mon_event_config_set,
 			      &mon_info, 1);
 
 	/*
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index f11d6fdfd977..c8ab3d7a0dab 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -118,6 +118,18 @@ struct rdt_mon_domain {
 	int				cqm_work_cpu;
 };
 
+/**
+ * struct mon_config_info - Monitoring event configuratiin details
+ * @d:			Domain for the event
+ * @evtid:		Event type
+ * @mon_config:		Event configuration value
+ */
+struct mon_config_info {
+	struct rdt_mon_domain *d;
+	enum resctrl_event_id evtid;
+	u32 mon_config;
+};
+
 /**
  * struct resctrl_cache - Cache allocation related data
  * @cbm_len:		Length of the cache bit mask
@@ -352,6 +364,10 @@ void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mon_domain *d,
  */
 void resctrl_arch_reset_rmid_all(struct rdt_resource *r, struct rdt_mon_domain *d);
 
+void resctrl_arch_mon_event_config_set(void *info);
+u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
+				      enum resctrl_event_id eventid);
+
 extern unsigned int resctrl_rmid_realloc_threshold;
 extern unsigned int resctrl_rmid_realloc_limit;
 
-- 
2.34.1
Re: [PATCH v10 11/24] x86/resctrl: Remove MSR reading of event configuration value
Posted by Reinette Chatre 11 months, 4 weeks ago
Hi Babu,

On 12/12/24 12:15 PM, Babu Moger wrote:

> @@ -1604,33 +1645,11 @@ unsigned int mon_event_config_index_get(u32 evtid)
>  	}
>  }
>  
> -static void mon_event_config_read(void *info)
> -{
> -	struct mon_config_info *mon_info = info;
> -	unsigned int index;
> -	u64 msrval;
> -
> -	index = mon_event_config_index_get(mon_info->evtid);
> -	if (index == INVALID_CONFIG_INDEX) {
> -		pr_warn_once("Invalid event id %d\n", mon_info->evtid);
> -		return;
> -	}
> -	rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
> -
> -	/* Report only the valid event configuration bits */
> -	mon_info->mon_config = msrval & MAX_EVT_CONFIG_BITS;
> -}
> -
> -static void mondata_config_read(struct rdt_mon_domain *d, struct mon_config_info *mon_info)
> -{
> -	smp_call_function_any(&d->hdr.cpu_mask, mon_event_config_read, mon_info, 1);
> -}
> -
>  static int mbm_config_show(struct seq_file *s, struct rdt_resource *r, u32 evtid)
>  {
> -	struct mon_config_info mon_info;
>  	struct rdt_mon_domain *dom;
>  	bool sep = false;
> +	u32 val;

Could this variable name be more descriptive? For example, mon_config, or config_val as
used in mbm_config_write_domain()?

...

> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index f11d6fdfd977..c8ab3d7a0dab 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -118,6 +118,18 @@ struct rdt_mon_domain {
>  	int				cqm_work_cpu;
>  };
>  
> +/**
> + * struct mon_config_info - Monitoring event configuratiin details

configuratiin -> configuration

... but actually, the motivation for moving this struct here was
to make it available for an arch to interpret the data passed
via resctrl_arch_mon_event_config_set(). This patch passes data
in this struct but a later patch modifies
resctrl_arch_mon_event_config_set() to not use struct anymore ...
and then leaves struct mon_config_info here.

Even so, considering Boris's preference this is no longer needed.

> + * @d:			Domain for the event
> + * @evtid:		Event type
> + * @mon_config:		Event configuration value
> + */
> +struct mon_config_info {
> +	struct rdt_mon_domain *d;
> +	enum resctrl_event_id evtid;
> +	u32 mon_config;
> +};
> +
>  /**
>   * struct resctrl_cache - Cache allocation related data
>   * @cbm_len:		Length of the cache bit mask
> @@ -352,6 +364,10 @@ void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mon_domain *d,
>   */
>  void resctrl_arch_reset_rmid_all(struct rdt_resource *r, struct rdt_mon_domain *d);
>  
> +void resctrl_arch_mon_event_config_set(void *info);
> +u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
> +				      enum resctrl_event_id eventid);
> +

Please move to internal header file instead and consider this for
all changes to include/linux/resctrl.h

>  extern unsigned int resctrl_rmid_realloc_threshold;
>  extern unsigned int resctrl_rmid_realloc_limit;
>  

Reinette
Re: [PATCH v10 11/24] x86/resctrl: Remove MSR reading of event configuration value
Posted by Moger, Babu 11 months, 4 weeks ago
Hi Reinette,

On 12/19/2024 4:12 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 12/12/24 12:15 PM, Babu Moger wrote:
> 
>> @@ -1604,33 +1645,11 @@ unsigned int mon_event_config_index_get(u32 evtid)
>>   	}
>>   }
>>   
>> -static void mon_event_config_read(void *info)
>> -{
>> -	struct mon_config_info *mon_info = info;
>> -	unsigned int index;
>> -	u64 msrval;
>> -
>> -	index = mon_event_config_index_get(mon_info->evtid);
>> -	if (index == INVALID_CONFIG_INDEX) {
>> -		pr_warn_once("Invalid event id %d\n", mon_info->evtid);
>> -		return;
>> -	}
>> -	rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
>> -
>> -	/* Report only the valid event configuration bits */
>> -	mon_info->mon_config = msrval & MAX_EVT_CONFIG_BITS;
>> -}
>> -
>> -static void mondata_config_read(struct rdt_mon_domain *d, struct mon_config_info *mon_info)
>> -{
>> -	smp_call_function_any(&d->hdr.cpu_mask, mon_event_config_read, mon_info, 1);
>> -}
>> -
>>   static int mbm_config_show(struct seq_file *s, struct rdt_resource *r, u32 evtid)
>>   {
>> -	struct mon_config_info mon_info;
>>   	struct rdt_mon_domain *dom;
>>   	bool sep = false;
>> +	u32 val;
> 
> Could this variable name be more descriptive? For example, mon_config, or config_val as
> used in mbm_config_write_domain()?

Will change it to config_val.

> 
> ...
> 
>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>> index f11d6fdfd977..c8ab3d7a0dab 100644
>> --- a/include/linux/resctrl.h
>> +++ b/include/linux/resctrl.h
>> @@ -118,6 +118,18 @@ struct rdt_mon_domain {
>>   	int				cqm_work_cpu;
>>   };
>>   
>> +/**
>> + * struct mon_config_info - Monitoring event configuratiin details
> 
> configuratiin -> configuration

Sure.

> 
> ... but actually, the motivation for moving this struct here was
> to make it available for an arch to interpret the data passed
> via resctrl_arch_mon_event_config_set(). This patch passes data
> in this struct but a later patch modifies
> resctrl_arch_mon_event_config_set() to not use struct anymore ...
> and then leaves struct mon_config_info here.
> 
> Even so, considering Boris's preference this is no longer needed.

ok. I will move the "struct mon_config_info" definition where it is 
used(rdtgroup.c).

> 
>> + * @d:			Domain for the event
>> + * @evtid:		Event type
>> + * @mon_config:		Event configuration value
>> + */
>> +struct mon_config_info {
>> +	struct rdt_mon_domain *d;
>> +	enum resctrl_event_id evtid;
>> +	u32 mon_config;
>> +};
>> +
>>   /**
>>    * struct resctrl_cache - Cache allocation related data
>>    * @cbm_len:		Length of the cache bit mask
>> @@ -352,6 +364,10 @@ void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mon_domain *d,
>>    */
>>   void resctrl_arch_reset_rmid_all(struct rdt_resource *r, struct rdt_mon_domain *d);
>>   
>> +void resctrl_arch_mon_event_config_set(void *info);
>> +u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
>> +				      enum resctrl_event_id eventid);
>> +
> 
> Please move to internal header file instead and consider this for
> all changes to include/linux/resctrl.h

Sure. Thanks

Babu