Re: [PATCH v3] x86/resctrl: Preserve CDP enable over cpuhp

[Reinette Chatre]

Hi James,

On 2/21/2020 8:21 AM, James Morse wrote:
> Resctrl assumes that all CPUs are online when the filesystem is
> mounted, and that CPUs remember their CDP-enabled state over CPU
> hotplug.
> 
> This goes wrong when resctrl's CDP-enabled state changes while all
> the CPUs in a domain are offline.
> 
> When a domain comes online, enable (or disable!) CDP to match resctrl's
> current setting.
> 
> Fixes: 5ff193fbde20 ("x86/intel_rdt: Add basic resctrl filesystem support")
> Suggested-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> Signed-off-by: James Morse <james.morse@xxxxxxx>
> ---
>  arch/x86/kernel/cpu/resctrl/core.c     |  2 ++
>  arch/x86/kernel/cpu/resctrl/internal.h |  1 +
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 13 +++++++++++++
>  3 files changed, 16 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 89049b343c7a..d8cc5223b7ce 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -578,6 +578,8 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
>  	d->id = id;
>  	cpumask_set_cpu(cpu, &d->cpu_mask);
>  
> +	rdt_domain_reconfigure_cdp(r);
> +
>  	if (r->alloc_capable && domain_setup_ctrlval(r, d)) {
>  		kfree(d);
>  		return;
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 181c992f448c..3dd13f3a8b23 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -601,5 +601,6 @@ bool has_busy_rmid(struct rdt_resource *r, struct rdt_domain *d);
>  void __check_limbo(struct rdt_domain *d, bool force_free);
>  bool cbm_validate_intel(char *buf, u32 *data, struct rdt_resource *r);
>  bool cbm_validate_amd(char *buf, u32 *data, struct rdt_resource *r);
> +void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>  
>  #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 064e9ef44cd6..1c78908ef395 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1859,6 +1859,19 @@ static int set_cache_qos_cfg(int level, bool enable)
>  	return 0;
>  }
>  
> +/* Restore the qos cfg state when a domain comes online */
> +void rdt_domain_reconfigure_cdp(struct rdt_resource *r)
> +{
> +	if (!r->alloc_capable)
> +		return;
> +
> +	if (r == &rdt_resources_all[RDT_RESOURCE_L2DATA])
> +		l2_qos_cfg_update(&r->alloc_enabled);
> +
> +	if (r == &rdt_resources_all[RDT_RESOURCE_L3DATA])
> +		l3_qos_cfg_update(&r->alloc_enabled);
> +}
> +
>  /*
>   * Enable or disable the MBA software controller
>   * which helps user specify bandwidth in MBps.
> 

As mentioned in my response to v2 the lockdep annotation that formed
part of this fix is welcome. It is not clear to me if you will be
submitting again with the annotation added back. Since it is not
required for this fix I will add my tag here and you could include it if
you do decide to resubmit.

Thank you

Reviewed-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>

Reinette