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

Reinette Chatre
2024-02-21T22:59:50Z

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/