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

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

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.

> 
> Add definitions of supported bandwidth sources.

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

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

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.

> + * @evt_name:		Name of memory transaction type (read, write etc).

Unclear what "type" means ... maybe just "Name of memory transaction (read, write ...)"?

The "evt_" prefix looks unnecessary.

> + * @evt_val:		Value representing the memory transaction.

This could just be "val" and the description could be specific:

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

> + */
> +struct mbm_evt_value {
> +	char    evt_name[32];
> +	u32     evt_val;

Please space member names with TABs.

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

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

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