Message-ID: <7018b196-cfd6-4e4f-8ada-d91f43d6ce2e@intel.com>
Date: Fri, 18 Apr 2025 14:17:21 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Tony Luck <tony.luck@...el.com>, Fenghua Yu <fenghuay@...dia.com>, "Maciej
 Wieczor-Retman" <maciej.wieczor-retman@...el.com>, Peter Newman
	<peternewman@...gle.com>, James Morse <james.morse@....com>, Babu Moger
	<babu.moger@....com>, Drew Fustini <dfustini@...libre.com>, Dave Martin
	<Dave.Martin@....com>, Anil Keshavamurthy <anil.s.keshavamurthy@...el.com>
CC: <linux-kernel@...r.kernel.org>, <patches@...ts.linux.dev>
Subject: Re: [PATCH v3 02/26] fs-x86/resctrl: Prepare for more monitor events

Hi Tony,

On 4/7/25 4:40 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
> 

first change:
> Simplify the code by coding for many events using loops on
> which are enabled.

Please elaborate on how the primary change is the change in data
structure and that is what enables loops to be used.

second change:
> 
> Make rdt_mon_features a bitmap to allow for expansion.

... and then a third change: Introduce rdt_lookup_evtid_by_name()
and rdt_event_name().

I recognize three logical changes. Could you please split this patch?

> 
> Move resctrl_is_mbm_event() to <asm/resctrl.h> as it gets used by core.c

What the patch actually does is move resctrl_is_mbm_event() to
include/linux/resctrl_types.h that is in itself unexpected
considering what resctrl_types.h is intended to be used for. See
details in changelog of commit

f16adbaf9272 ("x86/resctrl: Move resctrl types to a separate header")

for details on how resctrl_types.h is intended to be used.

> 
> Signed-off-by: Tony Luck <tony.luck@...el.com>
> ---
>  include/linux/resctrl.h                |  6 +--
>  include/linux/resctrl_types.h          |  8 +++
>  arch/x86/include/asm/resctrl.h         |  8 +--
>  arch/x86/kernel/cpu/resctrl/internal.h |  6 +--
>  fs/resctrl/internal.h                  |  4 ++
>  arch/x86/kernel/cpu/resctrl/core.c     | 45 +++++++++--------
>  arch/x86/kernel/cpu/resctrl/monitor.c  | 33 ++++++------
>  fs/resctrl/ctrlmondata.c               | 41 ++++-----------
>  fs/resctrl/monitor.c                   | 70 ++++++++++++++++----------
>  fs/resctrl/rdtgroup.c                  | 47 ++++++++---------
>  10 files changed, 133 insertions(+), 135 deletions(-)
> 

...

> diff --git a/include/linux/resctrl_types.h b/include/linux/resctrl_types.h
> index a7faf2cd5406..898068a99ef7 100644
> --- a/include/linux/resctrl_types.h
> +++ b/include/linux/resctrl_types.h
> @@ -55,5 +55,13 @@ enum resctrl_event_id {
>  };
>  
>  #define QOS_NUM_EVENTS		(QOS_L3_MBM_LOCAL_EVENT_ID + 1)
> +#define QOS_NUM_MBM_EVENTS	(QOS_L3_MBM_LOCAL_EVENT_ID - QOS_L3_MBM_TOTAL_EVENT_ID + 1)
> +#define MBM_EVENT_IDX(evt)	((evt) - QOS_L3_MBM_TOTAL_EVENT_ID)
> +
> +static inline bool resctrl_is_mbm_event(int e)
> +{
> +	return (e >= QOS_L3_MBM_TOTAL_EVENT_ID &&
> +		e <= QOS_L3_MBM_LOCAL_EVENT_ID);
> +}

include/linux/resctrl.h should be a better fit.

>  
>  #endif /* __LINUX_RESCTRL_TYPES_H */

...
>  
>  static int get_domain_id_from_scope(int cpu, enum resctrl_scope scope)
> @@ -864,13 +865,13 @@ static __init bool get_rdt_mon_resources(void)
>  	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>  
>  	if (rdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC))
> -		rdt_mon_features |= (1 << QOS_L3_OCCUP_EVENT_ID);
> +		__set_bit(QOS_L3_OCCUP_EVENT_ID, rdt_mon_features);
>  	if (rdt_cpu_has(X86_FEATURE_CQM_MBM_TOTAL))
> -		rdt_mon_features |= (1 << QOS_L3_MBM_TOTAL_EVENT_ID);
> +		__set_bit(QOS_L3_MBM_TOTAL_EVENT_ID, rdt_mon_features);
>  	if (rdt_cpu_has(X86_FEATURE_CQM_MBM_LOCAL))
> -		rdt_mon_features |= (1 << QOS_L3_MBM_LOCAL_EVENT_ID);
> +		__set_bit(QOS_L3_MBM_LOCAL_EVENT_ID, rdt_mon_features);
>  
> -	if (!rdt_mon_features)
> +	if (find_first_bit(rdt_mon_features, QOS_NUM_EVENTS) == QOS_NUM_EVENTS)
>  		return false;

Could you please use bitmap_empty() instead? It does the same, but makes it obvious what
is being tested for.

>  
>  	return !rdt_get_mon_l3_config(r);
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 163174cc0d3e..06623d51d006 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -36,7 +36,7 @@ bool rdt_mon_capable;
>  /*
>   * Global to indicate which monitoring events are enabled.
>   */
> -unsigned int rdt_mon_features;
> +DECLARE_BITMAP(rdt_mon_features, QOS_NUM_EVENTS);
>  
>  #define CF(cf)	((unsigned long)(1048576 * (cf) + 0.5))
>  
> @@ -168,19 +168,14 @@ static struct arch_mbm_state *get_arch_mbm_state(struct rdt_hw_mon_domain *hw_do
>  						 u32 rmid,
>  						 enum resctrl_event_id eventid)
>  {
> -	switch (eventid) {
> -	case QOS_L3_OCCUP_EVENT_ID:
> +	struct arch_mbm_state *state;
> +
> +	if (!resctrl_is_mbm_event(eventid))
>  		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];
> -	}
>  
> -	/* Never expect to get here */
> -	WARN_ON_ONCE(1);
> +	state = hw_dom->arch_mbm_states[MBM_EVENT_IDX(eventid)];
>  
> -	return NULL;
> +	return state ? &state[rmid] : NULL;
>  }
>  
>  void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mon_domain *d,
> @@ -209,14 +204,16 @@ 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)
>  {
>  	struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
> +	int evt, idx;
> +
> +	for_each_set_bit(evt, rdt_mon_features, QOS_NUM_EVENTS) {
> +		idx = MBM_EVENT_IDX(evt);
> +		if (!hw_dom->arch_mbm_states[idx])
> +			continue;

This does not look safe. Missing a resctrl_is_mbm_event() check?

> +		memset(hw_dom->arch_mbm_states[idx], 0,
> +		       sizeof(struct arch_mbm_state) * r->num_rmid);
> +	}
>  
> -	if (resctrl_arch_is_mbm_total_enabled())
> -		memset(hw_dom->arch_mbm_total, 0,
> -		       sizeof(*hw_dom->arch_mbm_total) * r->num_rmid);
> -
> -	if (resctrl_arch_is_mbm_local_enabled())
> -		memset(hw_dom->arch_mbm_local, 0,
> -		       sizeof(*hw_dom->arch_mbm_local) * r->num_rmid);
>  }
>  

...

> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index 3fe21dcf0fde..66e613906f3e 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -347,15 +347,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;
>  }
>  
>  static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
> @@ -843,20 +842,40 @@ static void dom_data_exit(struct rdt_resource *r)
>  	mutex_unlock(&rdtgroup_mutex);
>  }
>  
> -static struct mon_evt llc_occupancy_event = {
> -	.name		= "llc_occupancy",
> -	.evtid		= QOS_L3_OCCUP_EVENT_ID,
> +static struct mon_evt all_events[QOS_NUM_EVENTS] = {

"all_events" is very generic for a global. How about "mon_event_all" 
(placing the "_all" at end to match, for example, resctrl_schema_all).

> +	[QOS_L3_OCCUP_EVENT_ID] = {
> +		.name	= "llc_occupancy",
> +		.evtid	= QOS_L3_OCCUP_EVENT_ID,
> +	},
> +	[QOS_L3_MBM_TOTAL_EVENT_ID] = {
> +		.name	= "mbm_total_bytes",
> +		.evtid	= QOS_L3_MBM_TOTAL_EVENT_ID,
> +	},
> +	[QOS_L3_MBM_LOCAL_EVENT_ID] = {
> +		.name	= "mbm_local_bytes",
> +		.evtid	= QOS_L3_MBM_LOCAL_EVENT_ID,
> +	},
>  };
>  
> -static struct mon_evt mbm_total_event = {
> -	.name		= "mbm_total_bytes",
> -	.evtid		= QOS_L3_MBM_TOTAL_EVENT_ID,
> -};
> +int rdt_lookup_evtid_by_name(char *name)
> +{
> +	int evt;

Since this is resctrl fs code, please replace "rdt" with
"resctrl".

>  
> -static struct mon_evt mbm_local_event = {
> -	.name		= "mbm_local_bytes",
> -	.evtid		= QOS_L3_MBM_LOCAL_EVENT_ID,
> -};
> +	for_each_set_bit(evt, rdt_mon_features, QOS_NUM_EVENTS) {
> +		if (!strcmp(name, all_events[evt].name))
> +			return evt;
> +	}

* This is resctrl fs code. rdt_mon_features should be private to
  x86 so resctrl fs should not be accessing it directly. Perhaps
  there can be a new arch helper that can be used by resctrl to
  query if event is enabled? Similar to resctrl_arch_is_llc_occupancy_enabled()
  and friend but where the event ID is parameter and arch code can
  use rdt_mon_features.
* While the function name is "lookup_evtid_by_name" this function
  does not just look up the event id by name but also ensures that the
  event is enabled. The caller then uses "lookup_evtid_by_name" as
  a proxy for "is this event enabled". I think the code will be easier
  to understand if the functions do not have such hidden "features".
  resctrl fs could first use new arch helper to determine if
  event is enabled and then use a fs helper that reads name
  directly from the event array.
* For a function returning event id the return type is expected to
  be enum resctrl_event_id.

> +
> +	return -EINVAL;
> +}
> +
> +char *rdt_event_name(enum resctrl_event_id evt)
> +{
> +	if (!test_bit(evt, rdt_mon_features))
> +		return "unknown";
> +
> +	return all_events[evt].name;
> +}

Same comments as rdt_lookup_evtid_by_name()

Also please watch for rdt_mon_features in rest of resctrl fs changes.

Reinette

Powered by blists - more mailing lists