Re: [PATCH v6 06/22] x86/resctrl: Add support to enable/disable AMD ABMC feature

[James Morse <james.morse@arm.com>]

Hi Babu,

Some boring comments about where the code goes...

On 06/08/2024 23:00, Babu Moger wrote:
> Add the functionality to enable/disable AMD ABMC feature.
> 
> AMD ABMC feature is enabled by setting enabled bit(0) in MSR
> L3_QOS_EXT_CFG.  When the state of ABMC is changed, the MSR needs
> to be updated on all the logical processors in the QOS Domain.
> 
> Hardware counters will reset when ABMC state is changed. Reset the
> architectural state so that reading of hardware counter is not considered
> as an overflow in next update.
> 
> The ABMC feature details are documented in APM listed below [1].
> [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
> Publication # 24593 Revision 3.41 section 19.3.3.3 Assignable Bandwidth
> Monitoring (ABMC).

> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 2bd207624eec..154983a67646 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h

> @@ -536,6 +541,14 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable);
>  
>  void arch_mon_domain_online(struct rdt_resource *r, struct rdt_mon_domain *d);
>  
> +static inline bool resctrl_arch_get_abmc_enabled(void)
> +{
> +	return rdt_resources_all[RDT_RESOURCE_L3].mbm_cntr_assign_enabled;
> +}

Once the filesystem code moves to /fs/resctrl, this can't be inlined from the
architectures internal.h. Accessing rdt_resources_all[] from asm/resctrl.h isn't something
that is done today... could you move this to be a non-inline function in core.c?

(this saves me moving it later!)


> +int resctrl_arch_mbm_cntr_assign_enable(void);
> +void resctrl_arch_mbm_cntr_assign_disable(void);

Please add these in linux/resctrl.h - it saves me moving them later!


> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 7e76f8d839fc..6075b1e5bb77 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2402,6 +2402,63 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable)

> +static void _resctrl_abmc_enable(struct rdt_resource *r, bool enable)
> +{
> +	struct rdt_mon_domain *d;


> +	/*
> +	 * Hardware counters will reset after switching the monitor mode.
> +	 * Reset the architectural state so that reading of hardware
> +	 * counter is not considered as an overflow in the next update.
> +	 */
> +	list_for_each_entry(d, &r->mon_domains, hdr.list) {
> +		on_each_cpu_mask(&d->hdr.cpu_mask,
> +				 resctrl_abmc_set_one_amd, &enable, 1);
> +		resctrl_arch_reset_rmid_all(r, d);
> +	}

Is there any mileage in getting resctrl_arch_mbm_cntr_assign_enable()'s caller to do this?
Every architecture that supports this will have to do this, and neither x86 nor arm64 are
able to do it atomically, or quicker than calling resctrl_arch_reset_rmid_all() for each
domain.

> +}


> +int resctrl_arch_mbm_cntr_assign_enable(void)

Could we pass the struct rdt_resource in - instead of hard coding it to be the L3 - you
already check hw_res->mbm_cntr_assign_enabled so no additional check is needed...

Background: I'd like to reduce the amount of "I magically know its the L3" to reduce the
work for whoever has to add monitor support for something other than the L3.
(I've currently no plans - but someone is going to build it!)


> +{
> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> +	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);

> +	lockdep_assert_held(&rdtgroup_mutex);

After the split between the architecture and filesystem code - this lock is private to the
filesystem. If you need to prevent concurrent enable/disable calls the architecture should
take its own mutex.

| static DEFINE_MUTEX(abmc_lock);
?


> +	if (r->mon.mbm_cntr_assignable && !hw_res->mbm_cntr_assign_enabled) {
> +		_resctrl_abmc_enable(r, true);
> +		hw_res->mbm_cntr_assign_enabled = true;
> +	}
> +
> +	return 0;
> +}
> +
> +void resctrl_arch_mbm_cntr_assign_disable(void)
> +{
> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> +	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
> +
> +	lockdep_assert_held(&rdtgroup_mutex);

(same plea for passing the resource in, and not referring to the filesystem's locks)


> +	if (hw_res->mbm_cntr_assign_enabled) {
> +		_resctrl_abmc_enable(r, false);
> +		hw_res->mbm_cntr_assign_enabled = false;
> +	}
> +}


The work you do in these functions is pretty symmetric. Is it worth combining them into:
| resctrl_arch_mbm_cntr_assign_set(struct rdt_resource *r, bool enable) {
|	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
|
|	if (hw_res->mbm_cntr_assign_enabled != enable) {
|		_resctrl_abmc_enable(r, enable
|		hw_res->mbm_cntr_assign_enabled = enable;
|	}
| }

I think you need a resctrl_arch_mbm_cntr_assign_test() too - I'll comment on that patch...


Thanks,

James