Messages in this thread | | | Date | Fri, 31 Mar 2023 16:24:59 -0700 | Subject | Re: [PATCH v3 08/19] x86/resctrl: Add cpumask_any_housekeeping() for limbo/overflow | From | Reinette Chatre <> |
| |
Hi James,
On 3/20/2023 10:26 AM, James Morse wrote: > The limbo and overflow code picks a CPU to use from the domain's list > of online CPUs. Work is then scheduled on these CPUs to maintain > the limbo list and any counters that may overflow. > > cpumask_any() may pick a CPU that is marked nohz_full, which will > either penalise the work that CPU was dedicated to, or delay the
penalise -> penalize
> processing of limbo list or counters that may overflow. Perhaps > indefinitely. Delaying the overflow handling will skew the bandwidth > values calculated by mba_sc, which expects to be called once a second. > > Add cpumask_any_housekeeping() as a replacement for cpumask_any() > that prefers housekeeping CPUs. This helper will still return > a nohz_full CPU if that is the only option. The CPU to use is > re-evaluated each time the limbo/overflow work runs. This ensures > the work will move off a nohz_full CPU once a houskeeping CPU is > available. > > Signed-off-by: James Morse <james.morse@arm.com> > --- > arch/x86/kernel/cpu/resctrl/internal.h | 23 +++++++++++++++++++++++ > arch/x86/kernel/cpu/resctrl/monitor.c | 17 ++++++++++++----- > include/linux/tick.h | 3 ++- > 3 files changed, 37 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h > index 87545e4beb70..0b5fd5a0cda2 100644 > --- a/arch/x86/kernel/cpu/resctrl/internal.h > +++ b/arch/x86/kernel/cpu/resctrl/internal.h > @@ -7,6 +7,7 @@ > #include <linux/kernfs.h> > #include <linux/fs_context.h> > #include <linux/jump_label.h> > +#include <linux/tick.h> > #include <asm/resctrl.h> > > #define L3_QOS_CDP_ENABLE 0x01ULL > @@ -55,6 +56,28 @@ > /* Max event bits supported */ > #define MAX_EVT_CONFIG_BITS GENMASK(6, 0) > > +/** > + * cpumask_any_housekeeping() - Chose any cpu in @mask, preferring those that > + * aren't marked nohz_full
"Chose any cpu" -> "Choose any CPU"
> + * @mask: The mask to pick a CPU from. > + * > + * Returns a CPU in @mask. If there are houskeeping CPUs that don't use > + * nohz_full, these are preferred. > + */ > +static inline unsigned int cpumask_any_housekeeping(const struct cpumask *mask) > +{ > + int cpu, hk_cpu; > + > + cpu = cpumask_any(mask); > + if (tick_nohz_full_cpu(cpu)) { > + hk_cpu = cpumask_nth_andnot(0, mask, tick_nohz_full_mask); > + if (hk_cpu < nr_cpu_ids) > + cpu = hk_cpu; > + } > +
I think as a start this could perhaps be a #if defined(CONFIG_NO_HZ_FULL). There appears to be a precedent for this in kernel/rcu/tree_nocb.h.
Apart from the issue that Ilpo pointed out I would prefer that any changes outside resctrl are submitted separately to that subsystem.
...
> @@ -801,6 +803,11 @@ void mbm_handle_overflow(struct work_struct *work) > update_mba_bw(prgrp, d); > } > > + /* > + * Re-check for housekeeping CPUs. This allows the overflow handler to > + * move off a nohz_full CPU quickly. > + */ > + cpu = cpumask_any_housekeeping(&d->cpu_mask); > schedule_delayed_work_on(cpu, &d->mbm_over, delay); > > out_unlock:
From what I can tell the nohz_full CPUs are set during boot and do not change.
> @@ -814,7 +821,7 @@ void mbm_setup_overflow_handler(struct rdt_domain *dom, unsigned long delay_ms) > > if (!static_branch_likely(&rdt_mon_enable_key)) > return; > - cpu = cpumask_any(&dom->cpu_mask); > + cpu = cpumask_any_housekeeping(&dom->cpu_mask); > dom->mbm_work_cpu = cpu; > schedule_delayed_work_on(cpu, &dom->mbm_over, delay); > } > diff --git a/include/linux/tick.h b/include/linux/tick.h > index bfd571f18cfd..ae2e9019fc18 100644 > --- a/include/linux/tick.h > +++ b/include/linux/tick.h > @@ -174,9 +174,10 @@ static inline u64 get_cpu_iowait_time_us(int cpu, u64 *unused) { return -1; } > static inline void tick_nohz_idle_stop_tick_protected(void) { } > #endif /* !CONFIG_NO_HZ_COMMON */ > > +extern cpumask_var_t tick_nohz_full_mask; > + > #ifdef CONFIG_NO_HZ_FULL > extern bool tick_nohz_full_running; > -extern cpumask_var_t tick_nohz_full_mask; > > static inline bool tick_nohz_full_enabled(void) > {
In addition to what Ilpo pointed out, be careful here. cpumask_var_t is a pointer (or array) and needs to be allocated before use. Moving its declaration but not the allocation code seems risky.
Reinette
| |