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

[Shrikanth Hegde]

On 7/29/26 4:09 PM, K Prateek Nayak wrote:
> Hello Andrea,
>
> On 7/29/2026 3:05 PM, Andrea Righi wrote:
> > Hi Prateek,
> >
> > On Wed, Jul 29, 2026 at 01:48:14PM +0530, K Prateek Nayak wrote:
> > > Hello Andrea,
> > >
> > > On 7/29/2026 3:14 AM, Andrea Righi wrote:
> > > > @@ -13974,19 +13974,32 @@ static inline int find_new_ilb(void)
> > > >   		if (ilb_cpu == this_cpu)
> > > >   			continue;
> > > >   
> > > > -		if (idle_cpu(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))
> > >
> > > nit.
> > >
> > > is_core_idle() here would iterate all siblings and on systems with SMT-4
> > > and SMT-8, that overhead is apparently visible when one thread per core
> > > is occupied based on past optimizations like f8858d96061f ("sched/fair:
> > > Optimize should_we_balance() for large SMT systems").
> > >
> > > Copying the same approach from that optimization, can we do:
> >
> > Good point. Without removing the remaining siblings, we may call is_core_idle()
> > repeatedly for the same partially busy SMT core. I'll repeat my tests on my Vera
> > system and incorporate your suggestion in v2 if I don't see any regression.
>
> Thank you! From my experience, it is not very visible on SMT-2 but
> Shrikanth can vouch for the overheads on SMT-4, SMT-8 systems.

I had the same thought when I saw is_core_idle there.
I will try to give the patch a try.

> > >    (Only build tested)
> > >
> > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > > index d78467ec6ee1..814bce21ccf1 100644
> > > --- a/kernel/sched/fair.c
> > > +++ b/kernel/sched/fair.c
> > > @@ -13849,21 +13849,35 @@ static inline int on_null_domain(struct rq *rq)
> > >    */
> > >   static inline int find_new_ilb(void)
> > >   {
> > > +	struct cpumask *ilb_cpus = this_cpu_cpumask_var_ptr(select_rq_mask);
> > >   	int this_cpu = smp_processor_id();
> > > -	const struct cpumask *hk_mask;
> > > -	int ilb_cpu;
> > > +	int ilb_cpu, fallback = -1;
> > >   
> > > -	hk_mask = housekeeping_cpumask(HK_TYPE_KERNEL_NOISE);
> > > +	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;
> > > +
> > > +		if (sched_smt_active() && !is_core_idle(ilb_cpu)) {
> > > +			if (fallback == -1)
> > > +				fallback = ilb_cpu;
> > > +			/*
> > > +			 * If the core is not idle, and first SMT sibling which is
> > > +			 * idle has been found, then its not needed to check other
> > > +			 * SMT siblings for idleness:
> > > +			 */
> > > +			cpumask_andnot(ilb_cpus, ilb_cpus, cpu_smt_mask(ilb_cpu));
> > > +			continue;
> > > +		}
> > > +
> > > +		return ilb_cpu;
> > >   	}
> > >   
> > > -	return -1;
> > > +	return fallback;
> > >   }
> > >   
> > >   /*
> > > ---
> > >
> > > It is safe to use "select_rq_mask" here since this is the tick handler
> > > trying to find an ilb_cpu and "select_rq_mask" is only used in contexts
> > > with IRQs disabled. It can probably be renamed to suggest that it is
> > > safe to be used in any IRQ disabled context as a temporary mask.
> > >
> > > Thoughts?
> >
> > Agreed. We can also add lockdep_assert_irqs_disabled() to find_new_ilb() to
> > better document and verify the condition that makes reusing select_rq_mask safe.
>
> That works too but it just looks a bot odd to have the selectrq_mask in
> a load balancing function.
>
> > Speaking of that, instead of renaming it, would it be better to provide a helper
> > to access select_rq_mask with lockdep_assert_irqs_disabled()?
>
> I'll defer to Peter on that :-)
>
> He had previously suggested renaming it when there were discussions to
> reuse it here
> https://lore.kernel.org/lkml/20260320114312.GB3558198@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/