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