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

[K Prateek Nayak <kprateek.nayak@amd.com>]

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. 

> 
>>
>>   (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@noisy.programming.kicks-ass.net/

-- 
Thanks and Regards,
Prateek