Re: [PATCH v2] sched/fair: Prefer fully idle cores for NOHZ balancing

[Andrea Righi]

Hi Mete,

On Fri, Jul 31, 2026 at 02:53:55PM +0200, Mete Durlu wrote:
> On 29/07/2026 18:32, Andrea Righi wrote:
> > find_new_ilb() selects the first idle housekeeping CPU without
> > considering whether another thread is running on the same physical core.
> > On an SMT system, the idle load balancer can therefore activate both
> > siblings even when another housekeeping CPU has an entirely idle core.
> > 
> > On most SMT systems, this is not problematic because the idle load
> > balancer is a short-lived activity and the transient wakeup of a sibling
> > has negligible performance impact.
> > 
> > However, this can be particularly costly on NVIDIA Olympus cores used in
> > Vera. Briefly activating an otherwise idle sibling can reduce the
> > performance available to the other sibling and this effect does not
> > necessarily end once the activated sibling becomes idle: after the ILB
> > finishes and its CPU enters WFI, full single-thread performance is
> > restored only after the sibling has remained idle for a qualification
> > interval (10 Ki cycles on the tested Vera system). Repeated short
> > sibling wakeups can therefore sustain the interference even with little
> > actual overlap.
> > 
> > Prevent this by preferring an idle housekeeping CPU whose entire SMT
> > core is idle. Retain the first idle CPU as a fallback when no fully idle
> > core is available, so NOHZ balancing continues to make forward progress.
> > Once a partially busy core has been examined, skip its remaining SMT
> > siblings to avoid repeating the core-idle check on wide SMT systems.
> > 
> > Tests performed using an ad hoc GEMM benchmark running one CPU-intensive
> > task per SMT core within its CPU affinity mask improved from
> > approximately 6.2 TFLOP/s to 9.4 TFLOP/s.
> > 
> > Note that this preference may wake a fully idle physical core instead of
> > using an idle sibling of an active core, potentially increasing ILB
> > wakeup latency or energy consumption on some architectures. It may also
> > scan additional CPUs before selecting the one to run the ILB. The
> > selection falls back to the first idle CPU when no fully idle SMT core
> > is available. Non-SMT systems continue to select the first idle
> > housekeeping CPU.
> > 
> > Cc: K Prateek Nayak <kprateek.nayak@xxxxxxx>
> > Cc: Shrikanth Hegde <sshegde@xxxxxxxxxxxxx>
> > Signed-off-by: Andrea Righi <arighi@xxxxxxxxxx>
> > ---
> > Changes in v2:
> >   - Avoid repeated is_core_idle() checks on wide SMT systems by pruning
> >     the remaining siblings of a partially busy core (Prateek Nayak)
> >   - Link to v1: https://lore.kernel.org/r/20260728214442.1648483-1-arighi@xxxxxxxxxx/
> > 
> >   kernel/sched/fair.c | 48 ++++++++++++++++++++++++++++++++++++---------
> >   1 file changed, 39 insertions(+), 9 deletions(-)
> 
> Hi,
> 
> thank you for the interesting patch! I am testing this on s390
> to see how it impacts our platform. After reading the discussion
> on v1 and reviewing the code I got minor nit. See below;

Awesome! Let me know how it works on the s390.

A comment below about your nit.

> 
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 37001c63452e5..b9e26938fb12a 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -13965,28 +13965,58 @@ static inline int on_null_domain(struct rq *rq)
> >   static inline int find_new_ilb(void)
> >   {
> >   	int this_cpu = smp_processor_id();
> > -	const struct cpumask *hk_mask;
> > -	int ilb_cpu;
> > +	struct cpumask *ilb_cpus;
> > +	int ilb_cpu, fallback = -1;
> > +
> > +	lockdep_assert_irqs_disabled();
> > -	hk_mask = housekeeping_cpumask(HK_TYPE_KERNEL_NOISE);
> > +	/*
> > +	 * Reuse the per-CPU select_rq_mask, which is protected from concurrent
> > +	 * use on this CPU by having interrupts disabled.
> > +	 */
> > +	ilb_cpus = this_cpu_cpumask_var_ptr(select_rq_mask);
> > +	cpumask_and(ilb_cpus, nohz.idle_cpus_mask,
> > +		    housekeeping_cpumask(HK_TYPE_KERNEL_NOISE));
> > -	for_each_cpu_and(ilb_cpu, nohz.idle_cpus_mask, hk_mask) {
> > +	for_each_cpu(ilb_cpu, ilb_cpus) {
> >   		if (ilb_cpu == this_cpu)
> >   			continue;
> > -		if (idle_cpu(ilb_cpu))
> > -			return ilb_cpu;
> > +		if (!idle_cpu(ilb_cpu))
> > +			continue;
> > +
> > +		/*
> > +		 * Running the idle load balancer on an idle sibling of a busy
> > +		 * SMT core can reduce the capacity available to its sibling. Prefer
> > +		 * a CPU whose entire core is idle, but retain the first idle CPU as
> > +		 * a fallback so idle balancing can still make progress when no fully
> > +		 * idle core exists.
> > +		 */
> > +		if (sched_smt_active() && !is_core_idle(ilb_cpu)) {
> > +			if (fallback < 0)
> > +				fallback = ilb_cpu;
> > +
> > +			/*
> > +			 * The core is not idle, so there is no need to check
> > +			 * any of its other SMT siblings.
> > +			 */
> > +			cpumask_andnot(ilb_cpus, ilb_cpus,
> > +				       cpu_smt_mask(ilb_cpu));
> 
> Just a nit but;
> 
> I think the purpose here is to move between cores if smt is active but
> current logic seems to be doing that only after an idle cpu is found.
> Consider that we first found an idle cpu but the siblings are busy,
> after we move to the next core we start traversing per cpu again.
> 
> Wouldn't it be better if we always move per core after a fallback is
> found? How about sth like below?

The idea mekes sense, however...

> 
>         ...
>         for_each_cpu(ilb_cpu, ilb_cpus) {
>                 if (ilb_cpu == this_cpu)
>                         continue;
> 
>                 if (sched_smt_active()) {
>                         if (fallback < 0) {
>                                 if (!idle_cpu(ilb_cpu))
>                                         continue;
>                                 fallback = ilb_cpu;
>                         }
>                         /*
>                          * Running the idle load balancer on an idle sibling
> of
>                          * a busy SMT core can reduce the capacity available
> to
>                          * its sibling. Prefer a CPU whose entire core is
> idle,
>                          * but retain the first idle CPU as a fallback so
> idle
>                          * balancing can still make progress when no fully
>                          * idle core exists.
>                          */
>                         if (is_core_idle(ilb_cpu))
>                                 return ilb_cpu;

...is_core_idle() doesn't check the supplied CPU itself, only its siblings. So
we could return a busy ilb_cpu if all of its siblings are idle.

I think a safe improvement, following your logic, could be this:

   for_each_cpu(ilb_cpu, ilb_cpus) {
  	if (ilb_cpu == this_cpu)
  		continue;

  	if (!idle_cpu(ilb_cpu)) {
  		/*
  		 * Once an idle fallback exists, a busy CPU proves that
  		 * this core cannot be fully idle. Skip its siblings.
  		 */
  		if (sched_smt_active() && fallback >= 0)
  			cpumask_andnot(ilb_cpus, ilb_cpus,
  				       cpu_smt_mask(ilb_cpu));
  		continue;
  	}

  	/*
  	 * Running the idle load balancer on an idle sibling of a busy
  	 * SMT core can reduce the capacity available to its sibling. Prefer
  	 * a CPU whose entire core is idle, but retain the first idle CPU as
  	 * a fallback so idle balancing can still make progress when no fully
  	 * idle core exists.
  	 */
  	if (sched_smt_active() && !is_core_idle(ilb_cpu)) {
  		if (fallback < 0)
  			fallback = ilb_cpu;

  		cpumask_andnot(ilb_cpus, ilb_cpus,
  			       cpu_smt_mask(ilb_cpu));
  		continue;
  	}

  	return ilb_cpu;
  }

This should implement the right behavior:
 - before finding a fallback, inspect CPUs individually because another sibling
   might be idle,
 - once a fallback exists, a busy CPU proves its core is not fully idle, so we
   can skip the entire core,
 - for an idle CPU, checking is_core_idle() remains valid, because its own idle
   state has already been established.

What do you think? I'll run some tests on my side with this new logic applied.

Thanks!
-Andrea

>                         cpumask_andnot(ilb_cpus, ilb_cpus,
> cpu_smt_mask(ilb_cpu));
>                         continue;
>                 }
> 
>                 if (idle_cpu(ilb_cpu))
>                         return ilb_cpu;
>         }
> 
>         return fallback;
> 
>