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