Re: [PATCH v14 23/32] fs/resctrl: Add definitions for MBM event configuration

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

Hi Reinette,

On 6/24/25 23:32, Reinette Chatre wrote:
> Hi Babu,
> 
> On 6/13/25 2:05 PM, Babu Moger wrote:
>> The "mbm_event" mode allows the user to assign a hardware counter ID to
> 
> "The "mbm_event" mode" -> "The "mbm_event" counter assignment mode"
> (I'll stop noting this)
> 
Sure.

>> an RMID, event pair and monitor the bandwidth as long as it is assigned.
>> Additionally, the user can specify a particular type of memory
>> transactions for the counter to track.
> 
> hmmm ... this is not "Additionally" since the event is used to specify
> the memory transactions to track, no? Also please note mix of singular
> and plural: *a* particular type of memory *transactions*.

Sure.

> 
>>
>> 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 memory transactions being tracked by the event.
> 
> Unclear how this is relevant to this change. This is just about the
> memory transactions.

Removed it.
> 
>>
>> Add definitions for supported memory transactions (e.g., read, write,
>> etc.).
> 
> I think this changelog needs to connect that the memory transactions
> defined here is what MBM events can be configured with.

Yes.

Changed the whole changelog.

fs/resctrl: Add definitions for MBM event configuration

The "mbm_event" counter assignment mode allows the user to assign a
hardware counter to an RMID, event pair and monitor the bandwidth as long
as it is assigned. The user can specify a particular type of memory
transaction for the counter to track.

Add the definitions for supported memory transactions (e.g., read, write,
etc.) the counter can be configured with.


> 
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
> ...
> 
>> ---
>>  fs/resctrl/internal.h | 11 +++++++++++
>>  fs/resctrl/rdtgroup.c | 14 ++++++++++++++
>>  2 files changed, 25 insertions(+)
>>
>> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
>> index 4a7130018aa1..84a136194d9a 100644
>> --- a/fs/resctrl/internal.h
>> +++ b/fs/resctrl/internal.h
>> @@ -212,6 +212,17 @@ struct rdtgroup {
>>  	struct pseudo_lock_region	*plr;
>>  };
>>  
>> +/**
>> + * struct mbm_config_value - Memory transaction an MBM event can be configured with.
>> + * @name:	Name of memory transaction (read, write ...).
>> + * @val:	The bit used to represent the memory transaction within an
>> + *		event's configuration.
>> + */
>> +struct mbm_config_value {
>> +	char	name[32];
>> +	u32	val;
>> +};
> 
> "value" in struct name and "val" in member seems redundant. "config"
> is also very generic. How about "struct mbm_transaction"? All the
> descriptions already reflect this :)

Sure.

> 
>> +
>>  /* rdtgroup.flags */
>>  #define	RDT_DELETED		1
>>  
>> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
>> index 08bcca9bd8b6..5fb6a9939e23 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 transactions that an MBM event can be configured with. */
>> +#define NUM_MBM_EVT_VALUES             7
> 
> I think this should be in include/linux/resctrl_types.h to be with the
> values it represents. Regarding name, how about "NUM_MBM_TRANSACTIONS"?

Sure.

> 
>> +
>> +/* Decoded values for each type of memory transactions */
>> +struct mbm_config_value mbm_config_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