Messages in this thread | | | Date | Wed, 21 Feb 2024 14:59:43 -0800 | Subject | Re: [PATCH v2] x86/resctrl: Fix WARN in get_domain_from_cpu() | From | Reinette Chatre <> |
| |
Hi Tony,
On 2/21/2024 11:31 AM, 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 passing an > array with CDP_NUM_TYPES entries. Then calling for each domain separately > using smp_call_function_single()
This work contains no changes to get_domain_from_cpu(). I expect the WARN within it to be removed as intended with [1] and then this work can build on that without urgency. As I understand, to support the stated goal of this work, I expect get_domain_from_cpu() in the end to not have any WARN or IS_ENABLED checks, but just a lockdep_assert_cpus_held().
Do you have different expectations?
> Change the low level cat_wrmsr(), mba_wrmsr_intel(), and mba_wrmsr_amd() > functions to just take a msr_param structure since it contains the > rdt_resource and rdt_domain information.
Could moving the rdt_domain into msr_param be done in a separate patch?
..
> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c > index 7997b47743a2..09f6e624f1bb 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) > { > @@ -304,59 +288,50 @@ int resctrl_arch_update_one(struct rdt_resource *r, struct rdt_domain *d, > msr_param.res = r; > msr_param.low = idx; > msr_param.high = idx + 1; > - hw_res->msr_update(d, &msr_param, r); > + hw_res->msr_update(&msr_param); >
Is this missing setting the domain in msr_param?
> return 0; > } > > int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid) > { > + struct msr_param msr_param[CDP_NUM_TYPES]; > struct resctrl_staged_config *cfg; > 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; > + bool need_update; > + 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; > + memset(msr_param, 0, sizeof(msr_param)); > list_for_each_entry(d, &r->domains, list) { > hw_dom = resctrl_to_arch_dom(d); > + need_update = false; > for (t = 0; t < CDP_NUM_TYPES; t++) { > cfg = &hw_dom->d_resctrl.staged_config[t]; > if (!cfg->have_new_ctrl) > 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[t].low = idx; > + msr_param[t].high = msr_param[t].low + 1; > + msr_param[t].res = r; > + msr_param[t].dom = d; > + need_update = true; > } > + if (need_update) > + smp_call_function_single(cpu, rdt_ctrl_update, &msr_param, 1);
It is not clear to me why it is needed to pass this additional data. Why not just use the original mechanism of letting the low and high of msr_param span the multiple indices that need updating? There can still be a "need_update" but it can be set when msr_param gets its first data. Any other index needing updating can just update low/high and a single msr_param can be used.
Reinette
[1] https://lore.kernel.org/lkml/20240221122306.633273-1-james.morse@arm.com/
| |