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

[Shrikanth Hegde <sshegde@linux.ibm.com>]

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