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

[Andrea Righi <arighi@nvidia.com>]

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.

> 
>   (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.

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()?

Thanks,
-Andrea