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

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

Hi Babu,

On 8/6/24 3:00 PM, Babu Moger wrote:
> The event configuration is domain specific and initialized during domain
> initialization. The values is stored in rdt_hw_mon_domain.

"The values is stored in rdt_hw_mon_domain." -> "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 rdt_hw_mon_domain instead.

"rdt_hw_mon_domain" -> "struct rdt_hw_mon_domain"

> 
> 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. Also, remove unused config value
> definitions.

hmmm ... while the config values are not used they are now established
ABI and any other architecture that wants to support configurable events
will need to follow these definitions. It is thus required to keep them
documented in the kernel in support of future changes. I
understand that they are documented in user docs, but could we keep them
in the kernel code also? Since they are unused they could perhaps be moved
to comments as a compromise?

> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> 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/internal.h |  21 -----
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 104 ++++++++++++++-----------
>   include/linux/resctrl.h                |   4 +
>   3 files changed, 64 insertions(+), 65 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 4d8cc36a8d79..1021227d8c7e 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -32,27 +32,6 @@
>    */
>   #define MBM_CNTR_WIDTH_OFFSET_MAX (62 - MBM_CNTR_WIDTH_BASE)
>   
> -/* Reads to Local DRAM Memory */
> -#define READS_TO_LOCAL_MEM		BIT(0)
> -
> -/* Reads to Remote DRAM Memory */
> -#define READS_TO_REMOTE_MEM		BIT(1)
> -
> -/* Non-Temporal Writes to Local Memory */
> -#define NON_TEMP_WRITE_TO_LOCAL_MEM	BIT(2)
> -
> -/* Non-Temporal Writes to Remote Memory */
> -#define NON_TEMP_WRITE_TO_REMOTE_MEM	BIT(3)
> -
> -/* Reads to Local Memory the system identifies as "Slow Memory" */
> -#define READS_TO_LOCAL_S_MEM		BIT(4)
> -
> -/* Reads to Remote Memory the system identifies as "Slow Memory" */
> -#define READS_TO_REMOTE_S_MEM		BIT(5)
> -
> -/* Dirty Victims to All Types of Memory */
> -#define DIRTY_VICTIMS_TO_ALL_MEM	BIT(6)
> -
>   /* Max event bits supported */
>   #define MAX_EVT_CONFIG_BITS		GENMASK(6, 0)
>   
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 02afd3442876..0047b4eb0ff5 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1605,10 +1605,57 @@ static int rdtgroup_size_show(struct kernfs_open_file *of,
>   }
>   
>   struct mon_config_info {
> +	struct rdt_mon_domain *d;
>   	u32 evtid;
>   	u32 mon_config;
>   };
>   
> +u32 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_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_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;
> +		break;
> +	}
> +}
> +
>   /**
>    * mon_event_config_index_get - get the hardware index for the
>    *                              configurable event
> @@ -1631,33 +1678,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;
> +	u32 val;
>   
>   	cpus_read_lock();
>   	mutex_lock(&rdtgroup_mutex);
> @@ -1666,11 +1691,11 @@ 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);
> +		if (val == INVALID_CONFIG_VALUE)

Can this check and the "break" that follows be dropped? val being
INVALID_CONFIG_VALUE would be a kernel bug and resctrl_arch_event_config_get()
would already have printed the WARN. In this unlikely scenario I find it
unexpected that mbm_config_show() will return success in this case and the
below seq_printf() would handle the printing of INVALID_CONFIG_VALUE without
issue anyway.

> +			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");

Reinette