Re: [PATCH] x86/resctrl: Fix WARN in get_domain_from_cpu()

[Reinette Chatre]

Hi Tony,

Regarding the implication made in the subject ...
from what I understand the WARN is a false positive.

On 2/20/2024 4:34 PM, Tony Luck wrote:
> reset_all_ctrls() and resctrl_arch_update_domains() use
> on_each_cpu_mask() to call rdt_ctrl_update() on potentially
> one CPU from each domain.
> 
> But this means rdt_ctrl_update() needs to figure out which domain
> to apply changes to. Doing so requires a search of all domains
> in a resource, which can only be done safely if cpus_lock is
> held. Both callers do hold this lock, but there isn't a way
> for a function called on another CPU via IPI to verify this.
> 
> Fix by adding the target domain to the msr_param structure and
> calling for each domain separately using smp_call_function_single()

This sounds reasonable to me. Thank you for the proposal.

> Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx>
> 
> ---
> Either apply on top of tip x86/cache:
> 
>  fb700810d30b ("x86/resctrl: Separate arch and fs resctrl locks")
> 
> or merge this into that commit.

I do not know if it would be preferred to take this approach as
part of this work or just remove the WARN and add this
improvement/refactoring later as a follow-up. 

> ---
>  arch/x86/kernel/cpu/resctrl/internal.h    |  1 +
>  arch/x86/kernel/cpu/resctrl/core.c        | 10 +----
>  arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 50 +++++------------------
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c    | 14 ++-----
>  4 files changed, 16 insertions(+), 59 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index c99f26ebe7a6..c30d7697b431 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -383,6 +383,7 @@ static inline struct rdt_hw_domain *resctrl_to_arch_dom(struct rdt_domain *r)
>   */
>  struct msr_param {
>  	struct rdt_resource	*res;
> +	struct rdt_domain	*dom;
>  	u32			low;
>  	u32			high;
>  };
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 8a4ef4f5bddc..8d8b8abcda98 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -390,16 +390,8 @@ void rdt_ctrl_update(void *arg)
>  	struct msr_param *m = arg;
>  	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(m->res);
>  	struct rdt_resource *r = m->res;
> -	int cpu = smp_processor_id();
> -	struct rdt_domain *d;
>  
> -	d = get_domain_from_cpu(cpu, r);
> -	if (d) {
> -		hw_res->msr_update(d, m, r);
> -		return;
> -	}
> -	pr_warn_once("cpu %d not found in any domain for resource %s\n",
> -		     cpu, r->name);
> +	hw_res->msr_update(m->dom, m, r);

It looks redundant to provide struct msr_param as well as two of its
members as parameters. 

>  }
>  
>  /*
> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index 7997b47743a2..aed702d06314 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -272,22 +272,6 @@ static u32 get_config_index(u32 closid, enum resctrl_conf_type type)
>  	}
>  }
>  
> -static bool apply_config(struct rdt_hw_domain *hw_dom,
> -			 struct resctrl_staged_config *cfg, u32 idx,
> -			 cpumask_var_t cpu_mask)
> -{
> -	struct rdt_domain *dom = &hw_dom->d_resctrl;
> -
> -	if (cfg->new_ctrl != hw_dom->ctrl_val[idx]) {
> -		cpumask_set_cpu(cpumask_any(&dom->cpu_mask), cpu_mask);
> -		hw_dom->ctrl_val[idx] = cfg->new_ctrl;
> -
> -		return true;
> -	}
> -
> -	return false;
> -}
> -
>  int resctrl_arch_update_one(struct rdt_resource *r, struct rdt_domain *d,
>  			    u32 closid, enum resctrl_conf_type t, u32 cfg_val)
>  {
> @@ -315,17 +299,13 @@ int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid)
>  	struct rdt_hw_domain *hw_dom;
>  	struct msr_param msr_param;
>  	enum resctrl_conf_type t;
> -	cpumask_var_t cpu_mask;
>  	struct rdt_domain *d;
> +	int cpu;
>  	u32 idx;
>  
>  	/* Walking r->domains, ensure it can't race with cpuhp */
>  	lockdep_assert_cpus_held();
>  
> -	if (!zalloc_cpumask_var(&cpu_mask, GFP_KERNEL))
> -		return -ENOMEM;
> -
> -	msr_param.res = NULL;
>  	list_for_each_entry(d, &r->domains, list) {
>  		hw_dom = resctrl_to_arch_dom(d);
>  		for (t = 0; t < CDP_NUM_TYPES; t++) {
> @@ -334,29 +314,19 @@ int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid)
>  				continue;
>  
>  			idx = get_config_index(closid, t);
> -			if (!apply_config(hw_dom, cfg, idx, cpu_mask))
> +			if (cfg->new_ctrl == hw_dom->ctrl_val[idx])
>  				continue;
> -
> -			if (!msr_param.res) {
> -				msr_param.low = idx;
> -				msr_param.high = msr_param.low + 1;
> -				msr_param.res = r;
> -			} else {
> -				msr_param.low = min(msr_param.low, idx);
> -				msr_param.high = max(msr_param.high, idx + 1);
> -			}
> +			hw_dom->ctrl_val[idx] = cfg->new_ctrl;
> +			cpu = cpumask_any(&d->cpu_mask);
> +
> +			msr_param.low = idx;
> +			msr_param.high = msr_param.low + 1;
> +			msr_param.res = r;
> +			msr_param.dom = d;
> +			smp_call_function_single(cpu, rdt_ctrl_update, &msr_param, 1);

When CDP is enabled, could this not end up sending IPI to the same CPU twice, each
requesting CPU to do one MSR write instead of sending an IPI once to write all
needed MSRs?


Reinette