Re: [PATCH v4 02/31] x86,fs/resctrl: Prepare for more monitor events

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

Hi Tony,

On 4/28/25 5:33 PM, Tony Luck wrote:
> There's a rule in computer programming that objects appear zero,
> once, or many times. So code accordingly.
> 
> There are two MBM events and resctrl is coded with a lot of
> 
>         if (local)
>                 do one thing
>         if (total)
>                 do a different thing
> 
> Change the rdt_ctrl_domain and rdt_hw_mon_domain structures to hold

rdt_ctrl_domain -> rdt_mon_domain

> arrays of pointers to per event data instead of explicit fields for
> total and local bandwidth.
> 
> Simplify the code by coding for many events using loops on

"Simplify the code by coding" seems redundant, maybe just "Simplify"?

> which are enabled.
> 
> Move resctrl_is_mbm_event() to <linux/resctrl.h> so it
> can be used more widely. Also provide a for_each_mbm_event()
> helper macro.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  include/linux/resctrl.h                | 15 +++++---
>  include/linux/resctrl_types.h          |  3 ++
>  arch/x86/kernel/cpu/resctrl/internal.h |  6 ++--
>  arch/x86/kernel/cpu/resctrl/core.c     | 38 ++++++++++----------
>  arch/x86/kernel/cpu/resctrl/monitor.c  | 33 ++++++++++--------
>  fs/resctrl/monitor.c                   | 13 ++++---
>  fs/resctrl/rdtgroup.c                  | 48 ++++++++++++--------------
>  7 files changed, 84 insertions(+), 72 deletions(-)
> 
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 3c5d111aae65..cef9b0ed984c 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -161,8 +161,7 @@ struct rdt_ctrl_domain {
>   * @hdr:		common header for different domain types
>   * @ci:			cache info for this domain
>   * @rmid_busy_llc:	bitmap of which limbo RMIDs are above threshold
> - * @mbm_total:		saved state for MBM total bandwidth
> - * @mbm_local:		saved state for MBM local bandwidth
> + * @mbm_states:		saved state for each QOS MBM event
>   * @mbm_over:		worker to periodically read MBM h/w counters
>   * @cqm_limbo:		worker to periodically read CQM h/w counters
>   * @mbm_work_cpu:	worker CPU for MBM h/w counters
> @@ -172,8 +171,7 @@ struct rdt_mon_domain {
>  	struct rdt_domain_hdr		hdr;
>  	struct cacheinfo		*ci;
>  	unsigned long			*rmid_busy_llc;
> -	struct mbm_state		*mbm_total;
> -	struct mbm_state		*mbm_local;
> +	struct mbm_state		*mbm_states[QOS_NUM_MBM_EVENTS];
>  	struct delayed_work		mbm_over;
>  	struct delayed_work		cqm_limbo;
>  	int				mbm_work_cpu;
> @@ -376,6 +374,15 @@ void resctrl_enable_mon_event(enum resctrl_event_id evtid);
>  bool resctrl_is_mon_event_enabled(enum resctrl_event_id evt);
>  bool resctrl_arch_is_evt_configurable(enum resctrl_event_id evt);
>  
> +static inline bool resctrl_is_mbm_event(int e)

This looks like a good time to change the parameter type to enum resctrl_event_id.

> +{
> +	return (e >= QOS_L3_MBM_TOTAL_EVENT_ID &&
> +		e <= QOS_L3_MBM_LOCAL_EVENT_ID);
> +}
> +
> +#define for_each_mbm_event(evt)	\
> +	for (evt = QOS_L3_MBM_TOTAL_EVENT_ID; evt <= QOS_L3_MBM_LOCAL_EVENT_ID; evt++)
> +
>  /**
>   * resctrl_arch_mon_event_config_write() - Write the config for an event.
>   * @config_info: struct resctrl_mon_config_info describing the resource, domain
> diff --git a/include/linux/resctrl_types.h b/include/linux/resctrl_types.h
> index a25fb9c4070d..5ef14a24008c 100644
> --- a/include/linux/resctrl_types.h
> +++ b/include/linux/resctrl_types.h
> @@ -47,4 +47,7 @@ enum resctrl_event_id {
>  	QOS_NUM_EVENTS,
>  };
>  
> +#define QOS_NUM_MBM_EVENTS	(QOS_L3_MBM_LOCAL_EVENT_ID - QOS_L3_MBM_TOTAL_EVENT_ID + 1)

To prevent possible future confusion/churn while making existing code easier to read,
could this be "QOS_NUM_L3_MBM_EVENTS"?

> +#define MBM_EVENT_IDX(evt)	((evt) - QOS_L3_MBM_TOTAL_EVENT_ID)

Naming nit: I think "MBM event idx" is close enough to "MBM event id" to cause confusion.
How about "MBM_STATE_IDX"?

> +
>  #endif /* __LINUX_RESCTRL_TYPES_H */


...

> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index fda579251dba..bf7fde07846b 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -160,18 +160,21 @@ static struct arch_mbm_state *get_arch_mbm_state(struct rdt_hw_mon_domain *hw_do
>  						 u32 rmid,
>  						 enum resctrl_event_id eventid)
>  {
> +	struct arch_mbm_state *state;
> +
>  	switch (eventid) {
> -	case QOS_L3_OCCUP_EVENT_ID:
> -		return NULL;
> -	case QOS_L3_MBM_TOTAL_EVENT_ID:
> -		return &hw_dom->arch_mbm_total[rmid];
> -	case QOS_L3_MBM_LOCAL_EVENT_ID:
> -		return &hw_dom->arch_mbm_local[rmid];
>  	default:
>  		/* Never expect to get here */
>  		WARN_ON_ONCE(1);
> +		fallthrough;
> +	case QOS_L3_OCCUP_EVENT_ID:
>  		return NULL;
> +	case QOS_L3_MBM_TOTAL_EVENT_ID:
> +	case QOS_L3_MBM_LOCAL_EVENT_ID:
> +		state = hw_dom->arch_mbm_states[MBM_EVENT_IDX(eventid)];

Please add a "break" here. Although, I find the resulting get_mbm_state() to accomplish
the same in a much simpler way. Why did get_arch_mbm_state() and get_mbm_state() end up
looking so different?

>  	}
> +
> +	return state ? &state[rmid] : NULL;
>  }
>  
>  void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mon_domain *d,

...

> @@ -346,15 +346,14 @@ static struct mbm_state *get_mbm_state(struct rdt_mon_domain *d, u32 closid,
>  				       u32 rmid, enum resctrl_event_id evtid)
>  {
>  	u32 idx = resctrl_arch_rmid_idx_encode(closid, rmid);
> +	struct mbm_state *states;
>  
> -	switch (evtid) {
> -	case QOS_L3_MBM_TOTAL_EVENT_ID:
> -		return &d->mbm_total[idx];
> -	case QOS_L3_MBM_LOCAL_EVENT_ID:
> -		return &d->mbm_local[idx];
> -	default:
> +	if (!resctrl_is_mbm_event(evtid))
>  		return NULL;
> -	}
> +
> +	states = d->mbm_states[MBM_EVENT_IDX(evtid)];
> +
> +	return states ? &states[idx] : NULL;
>  }
>  

get_mbm_state() above for comparison.

Reinette