Re: [PATCH v13 12/27] x86/resctrl: Introduce event configuration modes

["Moger, Babu" <babu.moger@amd.com>]

Hi Reinette,

On 5/22/25 17:05, Reinette Chatre wrote:
> Hi Babu,
> 
> On 5/15/25 3:51 PM, Babu Moger wrote:
>> MBM events can be configured using either BMEC (Bandwidth Monitoring Event
>> Configuration) or the mbm_cntr_assign mode.
>>
>> Introduce a data structure to represent the various event configuration
>> modes and their corresponding values.
>>
>> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> 
> I cannot recall suggesting this.
> 
> (/me digs)
> 
> Are you perhaps referring to https://lore.kernel.org/lkml/d2966a26-4483-4808-a538-bb20973dd2a1@intel.com/

Yes.
> 
> This is not referring to new modes but the existing mbm_cntr_assign modes.
> resctrl knows which "mbm_cntr_assign" mode is active and it can use that
> to determine whether BMEC can be exposed to user space or not. There is
> already enough information in resctrl to know whether BMEC files should be
> exposed or not.
> 
> I think this work self makes clear that these modes are useless since
> patch #25 that determines whether to hide BMEC files doesn't even
> use it.

Sure. I will remove change related to mbm_mode.

> 
> 
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v13: New patch to handle different event configuration types with
>>      mbm_cntr_assign mode.
>> ---
>>  fs/resctrl/internal.h         |  6 ++++--
>>  fs/resctrl/monitor.c          |  4 ++--
>>  fs/resctrl/rdtgroup.c         |  2 +-
>>  include/linux/resctrl_types.h | 11 +++++++++++
>>  4 files changed, 18 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
>> index 9a8cf6f11151..0fae374559ba 100644
>> --- a/fs/resctrl/internal.h
>> +++ b/fs/resctrl/internal.h
>> @@ -55,13 +55,15 @@ static inline struct rdt_fs_context *rdt_fc2context(struct fs_context *fc)
>>   * struct mon_evt - Entry in the event list of a resource
>>   * @evtid:		event id
>>   * @name:		name of the event
>> - * @configurable:	true if the event is configurable
>> + * @mbm_mode:		monitoring mode (BMEC or mbm_cntr_assign)
>> + * @evt_cfg:		event configuration value decoding reads, writes.
>>   * @list:		entry in &rdt_resource->evt_list
>>   */
>>  struct mon_evt {
>>  	enum resctrl_event_id	evtid;
>>  	char			*name;
>> -	bool			configurable;
>> +	enum resctrl_mbm_mode	mbm_mode;
>> +	u32			evt_cfg;
> 
> This very important yet totally unrelated member sneaked in without
> any mention.

Yes. We only need evt_cfg. I will just add it as separate patch.

> 
>>  	struct list_head	list;
>>  };
>>  
>> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
>> index 2548aee0151c..8e403587a02f 100644
>> --- a/fs/resctrl/monitor.c
>> +++ b/fs/resctrl/monitor.c
>> @@ -903,12 +903,12 @@ int resctrl_mon_resource_init(void)
>>  	l3_mon_evt_init(r);
>>  
>>  	if (resctrl_arch_is_evt_configurable(QOS_L3_MBM_TOTAL_EVENT_ID)) {
>> -		mbm_total_event.configurable = true;
>> +		mbm_total_event.mbm_mode = MBM_MODE_BMEC;
>>  		resctrl_file_fflags_init("mbm_total_bytes_config",
>>  					 RFTYPE_MON_INFO | RFTYPE_RES_CACHE);
>>  	}
>>  	if (resctrl_arch_is_evt_configurable(QOS_L3_MBM_LOCAL_EVENT_ID)) {
>> -		mbm_local_event.configurable = true;
>> +		mbm_local_event.mbm_mode = MBM_MODE_BMEC;
>>  		resctrl_file_fflags_init("mbm_local_bytes_config",
>>  					 RFTYPE_MON_INFO | RFTYPE_RES_CACHE);
>>  	}
>> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
>> index 752750e3e443..f192b2736a77 100644
>> --- a/fs/resctrl/rdtgroup.c
>> +++ b/fs/resctrl/rdtgroup.c
>> @@ -1152,7 +1152,7 @@ static int rdt_mon_features_show(struct kernfs_open_file *of,
>>  
>>  	list_for_each_entry(mevt, &r->mon.evt_list, list) {
>>  		seq_printf(seq, "%s\n", mevt->name);
>> -		if (mevt->configurable)
>> +		if (mevt->mbm_mode == MBM_MODE_BMEC)
> 
> This can instead be a call to a utility that returns whether BMEC should be
> visible based on resctrl_mon::mbm_cntr_assignable and rdt_hw_resource::mbm_cntr_assign_enabled
> (via resctrl_arch_mbm_cntr_assign_enabled() of course).

Sure. Will do.

> 
>>  			seq_printf(seq, "%s_config\n", mevt->name);
>>  	}
>>  
>> diff --git a/include/linux/resctrl_types.h b/include/linux/resctrl_types.h
>> index a25fb9c4070d..26cd1fec72db 100644
>> --- a/include/linux/resctrl_types.h
>> +++ b/include/linux/resctrl_types.h
>> @@ -47,4 +47,15 @@ enum resctrl_event_id {
>>  	QOS_NUM_EVENTS,
>>  };
>>  
>> +/*
>> + * Event configuration mode.
>> + * Events can be configured either in BMEC (Bandwidth Monitoring Event
>> + * Configuration) mode or mbm_cntr_assign mode.
>> + */
>> +enum resctrl_mbm_mode {
>> +	MBM_MODE_NONE,
>> +	MBM_MODE_BMEC,
>> +	MBM_MODE_ASSIGN,
>> +};
>> +
>>  #endif /* __LINUX_RESCTRL_TYPES_H */
> 
> Reinette
> 

-- 
Thanks
Babu Moger