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@xxxxxxx>
> ---
> 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
