Re: [PATCH v12 18/26] x86/resctrl: Add default MBM event configurations for mbm_cntr_assign mode

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

Hi Babu,

On 4/3/25 5:18 PM, Babu Moger wrote:
> By default, each resctrl group supports two MBM events: mbm_total_bytes
> and mbm_local_bytes. To maintain the same level of support, two default
> MBM configurations are added. These configurations will initially be used
> to set up the counters upon mounting, while users will have the option to
> modify them as needed.

This jumps in quite fast by stating that MBM configurations are added but
there is no definition of what an MBM configuration is.

> 
> Event configuration values:
> ========================================================
>  Bits    Mnemonics       Description
> ====   ========================================================
>  6       VictimBW        Dirty Victims from all types of memory
>  5       RmtSlowFill     Reads to slow memory in the non-local NUMA domain
>  4       LclSlowFill     Reads to slow memory in the local NUMA domain
>  3       RmtNTWr         Non-temporal writes to non-local NUMA domain
>  2       LclNTWr         Non-temporal writes to local NUMA domain
>  1       mtFill          Reads to memory in the non-local NUMA domain
>  0       LclFill         Reads to memory in the local NUMA domain
> ====    ========================================================

What is the purpose of the mnemonics?

> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> v12: New patch to support event configurations via new counter_configs
>      method.
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 15 +++++++++++++++
>  include/linux/resctrl_types.h          | 17 +++++++++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index d84f47db4e43..aba23e2096db 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -57,6 +57,21 @@ static struct kernfs_node *kn_mongrp;
>  /* Kernel fs node for "mon_data" directory under root */
>  static struct kernfs_node *kn_mondata;
>  
> +struct mbm_evt_value mbm_evt_values[NUM_MBM_EVT_VALUES] = {
> +	{"local_reads", 0x1},
> +	{"remote_reads", 0x2},
> +	{"local_non_temporal_writes", 0x4},
> +	{"remote_non_temporal_writes", 0x8},
> +	{"local_reads_slow_memory", 0x10},
> +	{"remote_reads_slow_memory", 0x20},
> +	{"dirty_victim_writes_all", 0x40},
> +};
> +
> +struct mbm_assign_config mbm_assign_configs[NUM_MBM_ASSIGN_CONFIGS] = {
> +	{"mbm_total_bytes", QOS_L3_MBM_TOTAL_EVENT_ID, 0x7f},
> +	{"mbm_local_bytes", QOS_L3_MBM_LOCAL_EVENT_ID, 0x15},
> +};
> +
>  /*
>   * Used to store the max resource name width to display the schemata names in
>   * a tabular format.
> diff --git a/include/linux/resctrl_types.h b/include/linux/resctrl_types.h
> index f26450b3326b..3d98c7bdb459 100644
> --- a/include/linux/resctrl_types.h
> +++ b/include/linux/resctrl_types.h

Please read changelog of f16adbaf9272 ("x86/resctrl: Move resctrl types to a separate header")
for a good explanation of what resctrl_types.h is used for.

> @@ -31,6 +31,9 @@
>  /* Max event bits supported */
>  #define MAX_EVT_CONFIG_BITS		GENMASK(6, 0)
>  
> +#define NUM_MBM_EVT_VALUES		7
> +#define NUM_MBM_ASSIGN_CONFIGS		2

Please keep changes to internal header files unless required.

> +
>  enum resctrl_res_level {
>  	RDT_RESOURCE_L3,
>  	RDT_RESOURCE_L2,
> @@ -51,4 +54,18 @@ enum resctrl_event_id {
>  	QOS_L3_MBM_LOCAL_EVENT_ID	= 0x03,
>  };
>  
> +struct mbm_evt_value {
> +	char	evt_name[32];
> +	u32	evt_val;
> +};

I cannot see how this belongs in resctrl_types.h.

> +
> +/**
> + * struct mbm_assign_config - Configuration values

Please include a run of scripts/kernel-doc in your patch preparation steps.

The description "Configuration values" is incredibly vague.

> + */
> +struct mbm_assign_config {
> +	char			name[32];
> +	enum resctrl_event_id	evtid;
> +	u32			val;
> +};

Why is this new struct needed? It looks to me like a duplicate of struct
mon_evt with one member added. There is also already the evt_list as part
of a monitor resource that the array introduced here seems to duplicate.

Could the event configuration be made a member of struct mon_evt instead?
This exposes the need to integrate this better with BMEC support to make
clear how existing "configurable" member should used and/or expanded.

There seems more and more overlap with Tony's RMID work. Did you get a
chance to look at that?

> +
>  #endif /* __LINUX_RESCTRL_TYPES_H */

Reinette