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