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

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

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