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

[Moger, Babu]

Hi Reinette,

On 4/11/25 16:44, Reinette Chatre wrote:
> 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.

How about this?


By default, each resctrl group supports two MBM events: mbm_total_bytes
and mbm_local_bytes. These represent total and local memory bandwidth
monitoring, respectively. Each event corresponds to a specific MBM
configuration. Use these default configurations to set up the counters
during mount. Allow users to modify the configurations as needed after
initialization.

Initialize resctrl MBM events with default configurations.

>> to set up the counters upon mounting, while users will have the option to
>> modify them as needed.

> 
>>
>> 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?

I replace with full text on each of these.

> 
>>
>> 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.

Sure.

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

Will move these to internal header.

> 
>> +
>>  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.

Will move these to internal header.

> 
>> +
>> +/**
>> + * struct mbm_assign_config - Configuration values
> 
> Please include a run of scripts/kernel-doc in your patch preparation steps.

ok. Sure.

> 
> The description "Configuration values" is incredibly vague.

ok. Will add details.

> 
>> + */
>> +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.

Yes. We can probably do that.

> 
> 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.

Sure.

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

Looked little bit. Will have look bit closer again.

> 
>> +
>>  #endif /* __LINUX_RESCTRL_TYPES_H */
> 
> Reinette
> 

-- 
Thanks
Babu Moger