Re: [PATCH v6 01/30] x86,fs/resctrl: Consolidate monitor event descriptions

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

Hi Tony,

On 6/26/25 9:49 AM, Tony Luck wrote:
> There are currently only three monitor events, all associated with
> the RDT_RESOURCE_L3 resource. Growing support for additional events
> will be easier with some restructuring to have a single point in
> file system code where all attributes of all events are defined.
> 
> Place all event descriptions into an array mon_event_all[]. Doing
> this has the beneficial side effect of removing the need for
> rdt_resource::evt_list.
> 
> Add resctrl_event_id::QOS_FIRST_EVENT for a lower bound on range
> checks for event ids and as the starting index to scan mon_event_all[].
> 
> Drop the code that builds evt_list and change the two places where
> the list is scanned to scan mon_event_all[] instead using a new
> helper macro for_each_mon_event().
> 
> Architecture code now informs file system code which events are
> available with resctrl_enable_mon_event().
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  include/linux/resctrl.h            |  4 +-
>  include/linux/resctrl_types.h      | 12 ++++--
>  fs/resctrl/internal.h              | 13 ++++--
>  arch/x86/kernel/cpu/resctrl/core.c | 12 ++++--
>  fs/resctrl/monitor.c               | 63 +++++++++++++++---------------
>  fs/resctrl/rdtgroup.c              | 11 +++---
>  6 files changed, 66 insertions(+), 49 deletions(-)
> 
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 6fb4894b8cfd..2944042bd84c 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -269,7 +269,6 @@ enum resctrl_schema_fmt {
>   * @mon_domains:	RCU list of all monitor domains for this resource
>   * @name:		Name to use in "schemata" file.
>   * @schema_fmt:		Which format string and parser is used for this schema.
> - * @evt_list:		List of monitoring events
>   * @mbm_cfg_mask:	Bandwidth sources that can be tracked when bandwidth
>   *			monitoring events can be configured.
>   * @cdp_capable:	Is the CDP feature available on this resource
> @@ -287,7 +286,6 @@ struct rdt_resource {
>  	struct list_head	mon_domains;
>  	char			*name;
>  	enum resctrl_schema_fmt	schema_fmt;
> -	struct list_head	evt_list;
>  	unsigned int		mbm_cfg_mask;
>  	bool			cdp_capable;
>  };
> @@ -372,6 +370,8 @@ u32 resctrl_arch_get_num_closid(struct rdt_resource *r);
>  u32 resctrl_arch_system_num_rmid_idx(void);
>  int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid);
>  
> +void resctrl_enable_mon_event(enum resctrl_event_id eventid);
> +
>  bool resctrl_arch_is_evt_configurable(enum resctrl_event_id evt);
>  
>  /**
> diff --git a/include/linux/resctrl_types.h b/include/linux/resctrl_types.h
> index a25fb9c4070d..2dadbc54e4b3 100644
> --- a/include/linux/resctrl_types.h
> +++ b/include/linux/resctrl_types.h
> @@ -34,11 +34,15 @@
>  /* Max event bits supported */
>  #define MAX_EVT_CONFIG_BITS		GENMASK(6, 0)
>  
> -/*
> - * Event IDs, the values match those used to program IA32_QM_EVTSEL before
> - * reading IA32_QM_CTR on RDT systems.
> - */
> +/* Event IDs */
>  enum resctrl_event_id {
> +	/* Must match value of first event below */
> +	QOS_FIRST_EVENT			= 0x01,
> +
> +	/*
> +	 * These values match those used to program IA32_QM_EVTSEL before
> +	 * reading IA32_QM_CTR on RDT systems.
> +	 */
>  	QOS_L3_OCCUP_EVENT_ID		= 0x01,
>  	QOS_L3_MBM_TOTAL_EVENT_ID	= 0x02,
>  	QOS_L3_MBM_LOCAL_EVENT_ID	= 0x03,
> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
> index 0a1eedba2b03..445a41060724 100644
> --- a/fs/resctrl/internal.h
> +++ b/fs/resctrl/internal.h
> @@ -52,19 +52,26 @@ static inline struct rdt_fs_context *rdt_fc2context(struct fs_context *fc)
>  }
>  
>  /**
> - * struct mon_evt - Entry in the event list of a resource
> + * struct mon_evt - Properties of a monitor event
>   * @evtid:		event id
> + * @rid:		index of the resource for this event

x86 uses @rid as an index but this is not something that resctrl
fs enforces (please correct me if I am wrong). To prevent such assumption
this can just be "resource id for this event" or "ID of the resource 
associated with this event" or ?.

Patch looks good otherwise.
| Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette