Re: [PATCH v13 18/27] x86/resctrl: Add definitions for MBM event configuration

["Moger, Babu" <babu.moger@amd.com>]

Hi Reinette,

On 5/22/25 23:41, Reinette Chatre wrote:
> Hi Babu,
> 
> On 5/15/25 3:52 PM, Babu Moger wrote:
>> The "mbm_cntr_assign" mode allows users to manually assign a hardware
>> counter to a specific RMID and event pair. The events available for
>> assignment are configurable.
>>
>> By default, each resctrl group supports two MBM events: mbm_total_bytes
>> and mbm_local_bytes. Each event corresponds to an MBM configuration that
>> specifies the bandwidth sources tracked by the event.
> 
> hmmm ... earlier I thought "bandwidth source" means RMID but here it
> seems to mean the memory transactions? The various terms are confusing.

My bad. Yes. "bandwidth source" means RMID.

I should say "memory transactions"

> 
>>
>> Add definitions of supported bandwidth sources.
> 
> changelog uses "bandwidth sources" while the comments of patch
> uses "memory transactions" ... please be consistent with terms.

Sure.

> 
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v13: Updated the changelog.
>>      Removed the definitions from resctrl_types.h and moved to internal.h.
>>      Removed mbm_assign_config definition. Configurations will be part of
>>      mon_evt list.
>>      Resolved conflicts caused by the recent FS/ARCH code restructure.
>>      The rdtgroup.c file has now been split between the FS and ARCH directories.
>>
>> v12: New patch to support event configurations via new counter_configs
>>      method.
>> ---
>>  fs/resctrl/internal.h | 10 ++++++++++
>>  fs/resctrl/rdtgroup.c | 14 ++++++++++++++
>>  2 files changed, 24 insertions(+)
>>
>> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
>> index 0dfd2efe68fc..019d00bf5adf 100644
>> --- a/fs/resctrl/internal.h
>> +++ b/fs/resctrl/internal.h
>> @@ -203,6 +203,16 @@ struct rdtgroup {
>>  	struct pseudo_lock_region	*plr;
>>  };
>>  
>> +/**
>> + * struct mbm_evt_value - Specific type of memory events.
> 
> I am trying to decipher the terminology. If these are events, then it becomes confusing
> since it becomes "these events are used to configure events". You mention "memory
> transaction" below, this sounds more accurate to me. Above could thus be:
> 
> struct mbm_evt_value - Memory transaction an MBM event can be configured with.

Sure.

> 
> The name of the struct could also do with a rename to avoid the "event" term that
> conflicts with the actual MBM events. Maybe "mbm_cfg_value" ... I do not think this
> is a good name so please consider what would work better.

I can change it to "mbm_config_value".

> 
>> + * @evt_name:		Name of memory transaction type (read, write etc).
> 
> Unclear what "type" means ... maybe just "Name of memory transaction (read, write ...)"?

sure.

> 
> The "evt_" prefix looks unnecessary.

ok

> 
>> + * @evt_val:		Value representing the memory transaction.
> 
> This could just be "val" and the description could be specific:

ok.

> 
> "The bit used to represent the memory transaction within an event's configuration."
> Please feel free to improve.

Sounds good.

> 
>> + */
>> +struct mbm_evt_value {
>> +	char    evt_name[32];
>> +	u32     evt_val;
> 
> Please space member names with TABs.

Sure.

> 
>> +};
>> +
>>  /* rdtgroup.flags */
>>  #define	RDT_DELETED		1
>>  
>> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
>> index 72317a5adee2..b109e91096b0 100644
>> --- a/fs/resctrl/rdtgroup.c
>> +++ b/fs/resctrl/rdtgroup.c
>> @@ -75,6 +75,20 @@ static void rdtgroup_destroy_root(void);
>>  
>>  struct dentry *debugfs_resctrl;
>>  
>> +/* Number of memory transaction types that can be monitored */
> 
> "Number of memory transactions that an MBM event can be configured with."?

Sure.

> 
>> +#define NUM_MBM_EVT_VALUES             7
>> +
>> +/* Decoded values for each type of memory events */
> 
> Please be consistent with terminology. In the above lines it switches
> between "memory transaction types" and "memory events".

"Decoded values for each type of memory transaction types"

> 
>> +struct mbm_evt_value mbm_evt_values[NUM_MBM_EVT_VALUES] = {
>> +	{"local_reads", READS_TO_LOCAL_MEM},
>> +	{"remote_reads", READS_TO_REMOTE_MEM},
>> +	{"local_non_temporal_writes", NON_TEMP_WRITE_TO_LOCAL_MEM},
>> +	{"remote_non_temporal_writes", NON_TEMP_WRITE_TO_REMOTE_MEM},
>> +	{"local_reads_slow_memory", READS_TO_LOCAL_S_MEM},
>> +	{"remote_reads_slow_memory", READS_TO_REMOTE_S_MEM},
>> +	{"dirty_victim_writes_all", DIRTY_VICTIMS_TO_ALL_MEM},
>> +};
>> +
>>  /*
>>   * Memory bandwidth monitoring event to use for the default CTRL_MON group
>>   * and each new CTRL_MON group created by the user.  Only relevant when
> 
> Reinette
> 

-- 
Thanks
Babu Moger