Re: [PATCH v5 11/20] x86/resctrl: Remove MSR reading of event configuration value

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Babu,

On 7/3/24 2:48 PM, Babu Moger wrote:
> The event configuration is domain specific and initialized during domain
> initialization. It is not required to read the configuration register
> every time user asks for it. Use the value stored in rdt_mon_hw_domain

rdt_mon_hw_domain -> rdt_hw_mon_domain

> instead. Also update the configuration value when user writes it.

Please separate the context/problem/solution clearly.

> 
> Introduce resctrl_arch_event_config_get() and
> resctrl_arch_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>
> ---
> 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 | 112 +++++++++++++++----------
>   include/linux/resctrl.h                |   4 +
>   2 files changed, 72 insertions(+), 44 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index b2b751741dd8..91c5d45ac367 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1591,10 +1591,59 @@ static int rdtgroup_size_show(struct kernfs_open_file *of,
>   }
>   
>   struct mon_config_info {
> +	struct rdt_mon_domain *d;
>   	u32 evtid;
>   	u32 mon_config;
>   };

as seen above, mon_config is a u32

>   
> +#define INVALID_CONFIG_VALUE   UINT_MAX

So an invalid config value can be U32_MAX?

> +
> +unsigned int resctrl_arch_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_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_VALUE) {

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);
> +
> +	hw_dom = resctrl_to_arch_mon_dom(mon_info->d);
> +
> +	switch (mon_info->evtid) {
> +	case QOS_L3_OCCUP_EVENT_ID:
> +		break;
> +	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;

Please add a break here.

> +	}
> +}
> +
>   #define INVALID_CONFIG_INDEX   UINT_MAX
>   
>   /**
> @@ -1619,33 +1668,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 = {0};
>   	struct rdt_mon_domain *dom;
>   	bool sep = false;
> +	int val;
>   
>   	cpus_read_lock();
>   	mutex_lock(&rdtgroup_mutex);
> @@ -1654,11 +1681,13 @@ 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);
> +		val = resctrl_arch_event_config_get(dom, evtid);

There are too many types used interchangeably. The mon_config is a "u32", but the new function
returns "unsigned int", which is then assigned to an "int". Please just use one type
consistently, it is a u32 so resctrl_arch_event_config_get() can return u32 and "val" should
be u32.

> +		if (val == INVALID_CONFIG_VALUE) {
> +			rdt_last_cmd_puts("Invalid event configuration\n");

I do not see a reason to print message to user space here. If this error is encountered
then it is a kernel bug and resctrl_arch_event_config_get() would already have triggered
a WARN.

Since this is a "never should happen" scenario I wonder if we can not just print
the INVALID_CONFIG_VALUE to user space?


> +			break;
> +		}
>   
> -		seq_printf(s, "%d=0x%02x", dom->hdr.id, mon_info.mon_config);
> +		seq_printf(s, "%d=0x%02x", dom->hdr.id, val);
>   		sep = true;
>   	}
>   	seq_puts(s, "\n");
> @@ -1689,33 +1718,27 @@ 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};
> +	int 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_event_config_get(d, evtid);
> +	if (config_val == INVALID_CONFIG_VALUE) {
> +		rdt_last_cmd_puts("Invalid event configuration\n");

same here about unneeded print to user space. When this is encountered it is
a kernel bug.

> +		return;
> +	}
> +	if (config_val == val)
>   		return;
>   
> +	mon_info.d = d;
> +	mon_info.evtid = evtid;
>   	mon_info.mon_config = val;
>   
>   	/*
> @@ -1724,7 +1747,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_event_config_set,
>   			      &mon_info, 1);
>   
>   	/*
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 62f0f002ef41..f017258ebf85 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -352,6 +352,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_event_config_set(void *info);
> +unsigned int resctrl_arch_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;
>   

Reinette