Re: [PATCH v12 01/26] x86/resctrl: Introduce mbm_total_cfg and mbm_local_cfg in struct rdt_hw_mon_domain

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

Hi Reinette,

Thanks for the quick response to the series.

On 4/11/25 15:49, Reinette Chatre wrote:
> Hi Babu,
> 
> On 4/3/25 5:18 PM, Babu Moger wrote:
>> If the BMEC (Bandwidth Monitoring Event Configuration) feature is
>> supported, the bandwidth events can be configured to track specific
>> events. The event configuration is domain specific. Event configurations
>> are not stored in resctrl but instead always read from or written to
>> hardware directly when prompted by user space.
> 
> Why is this a problem?

I mean it involves an extra MSR read every time use asks for it.

> 
>>
>> Read the event configuration from the hardware during domain
>> initialization and store the configuration value in the rdt_hw_mon_domain
>> structure for later use when the user requests to display it.
> 
> Why is this required?

Minor optimization.

> 
> This series is about adding support for ABMC while this appears to be
> an optimization for BMEC. Even more, as I see it, this optimization makes
> resctrl support of ABMC and BMEC confusing (more below).
> 
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v12: Fixed the conflicts due to recent merge.
>>      This patch is for BMEC and there is no dependancy on ABMC feature.
> 
> Why still do it?

Ok. Will drop it for now.

> 
>>      Moved it earlier.
>>
>> v11: Resolved minor conflicts due to code displacement. Actual code didnt
>>      change.
>>
>> v10: Conflicts due to code displacement. Actual code didnt change.
>>
>> v9: Added Reviewed-by tag. No other changes.
>>
>> v8: Renamed resctrl_mbm_evt_config_init() to arch_mbm_evt_config_init()
>>     Minor commit message update.
>>
>> v7: Fixed initializing INVALID_CONFIG_VALUE to mbm_local_cfg in case of error.
>>
>> v6: Renamed resctrl_arch_mbm_evt_config -> resctrl_mbm_evt_config_init
>>     Initialized value to INVALID_CONFIG_VALUE if it is not configurable.
>>     Minor commit message update.
>>
>> v5: Exported mon_event_config_index_get.
>>     Renamed arch_domain_mbm_evt_config to resctrl_arch_mbm_evt_config.
>>
>> v4: Read the configuration information from the hardware to initialize.
>>     Added few commit messages.
>>     Fixed the tab spaces.
>>
>> v3: Minor changes related to rebase in mbm_config_write_domain.
>>
>> v2: No changes.
>> ---
>>  arch/x86/kernel/cpu/resctrl/core.c     |  2 ++
>>  arch/x86/kernel/cpu/resctrl/internal.h |  9 +++++++++
>>  arch/x86/kernel/cpu/resctrl/monitor.c  | 26 ++++++++++++++++++++++++++
>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |  2 +-
>>  4 files changed, 38 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>> index cf29681d01e0..a28de257168f 100644
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>> @@ -558,6 +558,8 @@ static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
>>  		return;
>>  	}
>>  
>> +	arch_mbm_evt_config_init(hw_dom);
>> +
>>  	list_add_tail_rcu(&d->hdr.list, add_pos);
>>  
>>  	err = resctrl_online_mon_domain(r, d);
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index c44c5b496355..9846153aa48f 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -32,6 +32,9 @@
>>   */
>>  #define MBM_CNTR_WIDTH_OFFSET_MAX (62 - MBM_CNTR_WIDTH_BASE)
>>  
>> +#define INVALID_CONFIG_VALUE		U32_MAX
>> +#define INVALID_CONFIG_INDEX		UINT_MAX
>> +
>>  /**
>>   * cpumask_any_housekeeping() - Choose any CPU in @mask, preferring those that
>>   *			        aren't marked nohz_full
>> @@ -335,6 +338,8 @@ struct rdt_hw_ctrl_domain {
>>   * @d_resctrl:	Properties exposed to the resctrl file system
>>   * @arch_mbm_total:	arch private state for MBM total bandwidth
>>   * @arch_mbm_local:	arch private state for MBM local bandwidth
>> + * @mbm_total_cfg:	MBM total bandwidth configuration
>> + * @mbm_local_cfg:	MBM local bandwidth configuration
>>   *
>>   * Members of this structure are accessed via helpers that provide abstraction.
>>   */
>> @@ -342,6 +347,8 @@ struct rdt_hw_mon_domain {
>>  	struct rdt_mon_domain		d_resctrl;
>>  	struct arch_mbm_state		*arch_mbm_total;
>>  	struct arch_mbm_state		*arch_mbm_local;
>> +	u32				mbm_total_cfg;
>> +	u32				mbm_local_cfg;
>>  };
> 
> This introduces an architecture managed per-domain event configuration while
> the rest of the series introduces a resctrl fs managed global event configuration.
> I see this as the start of a source for confusion about how events are configured since
> there is no further connection between this per-domain event configuration maintained
> by the architecture and the global event configuration maintained by resctrl fs.
> 
>>  
>>  static inline struct rdt_hw_ctrl_domain *resctrl_to_arch_ctrl_dom(struct rdt_ctrl_domain *r)
>> @@ -504,6 +511,8 @@ void resctrl_file_fflags_init(const char *config, unsigned long fflags);
>>  void rdt_staged_configs_clear(void);
>>  bool closid_allocated(unsigned int closid);
>>  int resctrl_find_cleanest_closid(void);
>> +void arch_mbm_evt_config_init(struct rdt_hw_mon_domain *hw_dom);
>> +unsigned int mon_event_config_index_get(u32 evtid);
>>  
>>  #ifdef CONFIG_RESCTRL_FS_PSEUDO_LOCK
>>  int rdtgroup_locksetup_enter(struct rdtgroup *rdtgrp);
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index a93ed7d2a160..abd337fbd01d 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -1284,6 +1284,32 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>>  	return 0;
>>  }
>>  
>> +void arch_mbm_evt_config_init(struct rdt_hw_mon_domain *hw_dom)
>> +{
>> +	unsigned int index;
>> +	u64 msrval;
>> +
>> +	/*
>> +	 * Read the configuration registers QOS_EVT_CFG_n, where <n> is
>> +	 * the BMEC event number (EvtID).
>> +	 */
>> +	if (mbm_total_event.configurable) {
> 
> Please keep an eye on where things are going in the arch/fs split.
> mbm_total_event is private to resctrl fs and arch code cannot reach into it.
> There is the arch helper resctrl_arch_is_evt_configurable() but I also
> think that this helper needs to be reconsidered in the light of ABMC.

ok

> 
> Overall I think this ABMC support needs to consider what already exists
> for BMEC support and ensure that both are supported coherently. For example,
> when a monitor domain has a "MBM local bandwidth configuration" then it should
> be obvious what that means.

ok. Agreed. Lets drop these two patches. Lets address ABMC in this series.

-- 
Thanks
Babu Moger