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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1ecc2e54-f62a-4937-be9d-5a275dae0634@amd.com>
Date: Mon, 30 Jun 2025 12:20:38 -0500
From: "Moger, Babu" <babu.moger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>, corbet@....net,
 tony.luck@...el.com, Dave.Martin@....com, james.morse@....com,
 tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
 dave.hansen@...ux.intel.com
Cc: x86@...nel.org, hpa@...or.com, akpm@...ux-foundation.org,
 rostedt@...dmis.org, paulmck@...nel.org, thuth@...hat.com, ardb@...nel.org,
 gregkh@...uxfoundation.org, seanjc@...gle.com, thomas.lendacky@....com,
 pawan.kumar.gupta@...ux.intel.com, manali.shukla@....com,
 perry.yuan@....com, kai.huang@...el.com, peterz@...radead.org,
 xiaoyao.li@...el.com, kan.liang@...ux.intel.com, mario.limonciello@....com,
 xin3.li@...el.com, gautham.shenoy@....com, xin@...or.com,
 chang.seok.bae@...el.com, fenghuay@...dia.com, peternewman@...gle.com,
 maciej.wieczor-retman@...el.com, eranian@...gle.com,
 linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v14 23/32] fs/resctrl: Add definitions for MBM event
 configuration

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