x86/resctrl: Fix WARN in get_domain_from_cpu()
Commit Message
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()
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
Either apply on top of tip x86/cache:
fb700810d30b ("x86/resctrl: Separate arch and fs resctrl locks")
or merge this into that commit.
---
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(-)
Comments
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@intel.com>
>
> ---
> 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
Hi Tony,
On 21/02/2024 00:34, 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()
Cunning - this trades the memory allocation for multiple IPI. I think this is much better
for the case where only the local domains configuration is modified.
With the double-IPI when both CDP configurations are changed fixed:
Reviewed-by: James Morse <james.morse@arm.com>
I think we should rip out the false positive check, I'll post a patch to do that.
I'll double check this was the only IPI path, if so its safe again after this patch and we
can add lockdep_assert_cpus_held(). If anyone ever hits this during a bisect its should be
clear(er).
Thanks,
James
@@ -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;
};
@@ -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);
}
/*
@@ -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);
}
}
- if (cpumask_empty(cpu_mask))
- goto done;
-
- /* Update resource control msr on all the CPUs. */
- on_each_cpu_mask(cpu_mask, rdt_ctrl_update, &msr_param, 1);
-
-done:
- free_cpumask_var(cpu_mask);
-
return 0;
}
@@ -2813,16 +2813,13 @@ static int reset_all_ctrls(struct rdt_resource *r)
struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
struct rdt_hw_domain *hw_dom;
struct msr_param msr_param;
- cpumask_var_t cpu_mask;
struct rdt_domain *d;
+ int cpu;
int i;
/* 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 = r;
msr_param.low = 0;
msr_param.high = hw_res->num_closid;
@@ -2834,17 +2831,14 @@ static int reset_all_ctrls(struct rdt_resource *r)
*/
list_for_each_entry(d, &r->domains, list) {
hw_dom = resctrl_to_arch_dom(d);
- cpumask_set_cpu(cpumask_any(&d->cpu_mask), cpu_mask);
+ cpu = cpumask_any(&d->cpu_mask);
for (i = 0; i < hw_res->num_closid; i++)
hw_dom->ctrl_val[i] = r->default_ctrl;
+ msr_param.dom = d;
+ smp_call_function_single(cpu, rdt_ctrl_update, &msr_param, 1);
}
- /* Update CBM on all the CPUs in cpu_mask */
- on_each_cpu_mask(cpu_mask, rdt_ctrl_update, &msr_param, 1);
-
- free_cpumask_var(cpu_mask);
-
return 0;
}