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

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

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)

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

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

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

> 
> 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 :)

> +
>  /* 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"?

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