[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4f3f52ab-b2d5-4c9e-bf9e-94ea75d3dade@linux.ibm.com>
Date: Tue, 6 Jan 2026 09:53:13 +0530
From: Shrikanth Hegde <sshegde@...ux.ibm.com>
To: K Prateek Nayak <kprateek.nayak@....com>
Cc: linux-kernel@...r.kernel.org, juri.lelli@...hat.com, vschneid@...hat.com,
tglx@...utronix.de, dietmar.eggemann@....com, anna-maria@...utronix.de,
frederic@...nel.org, wangyang.guo@...el.com, mingo@...nel.org,
peterz@...radead.org, vincent.guittot@...aro.org
Subject: Re: [PATCH v2 2/3] sched/fair: Change likelyhood of nohz.nr_cpus and
do stats update if its due
On 1/6/26 8:33 AM, K Prateek Nayak wrote:
> Hello Shrikanth,
>
> On 1/5/2026 4:09 PM, Shrikanth Hegde wrote:
>>> So cumulatively, including Patch 3, we do:
>>>
>>> flags = 0;
>>>
>>> if (READ_ONCE(nohz.has_blocked_load) && ...)
>>> flags = NOHZ_STATS_KICK;
>>>
>>> if (time_before(now, nohz.next_balance))
>>> goto out; /* Checks nohz.idle_cpus_mask in find_new_ilb() ... (1) */
>>>
>>> if (unlikely(cpumask_empty(nohz.idle_cpus_mask)))
>>> goto out; /* Still goes to kick_ilb() ... (2) */
>>>
>>> ...
>>>
>>> out:
>>> if (READ_ONCE(nohz.needs_update))
>>> flags |= NOHZ_NEXT_KICK;
>>>
>>> /* assume either NOHZ_STATS_KICK or NOHZ_NEXT_KICK is set */
>>> kick_ilb()
>>> {
>>> if (flags & NOHZ_BALANCE_KICK) /* Not possible */
>>> ...
>>>
>>> ilb_cpu = find_new_ilb(); /* Find CPU in nohz.idle_cpus_mask */
>>>
>>>
>>> If we arrive here from (2), we know "nohz.idle_cpus_mask" was empty a
>>> while back and we've not updated any global "nohz" state. If we don't
>>> find an ilb_cpu, we just do:
>>>
>>> if (ilb_cpu < 0)
>>> return;
>>>
>>> So why not simply return from (2)?
>>>
>>
>> I see, kick_ilb though called will not do a balance since ilb_cpu was not found.
>>
>> I don't want to have that return in between the two out's.
>>
>> How about we do below? When there are no idle CPUs left, both has_blocked_load
>> and needs_update should be reset. no?
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 805b53d9709e..fa0e6065bc9c 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -12377,6 +12377,15 @@ static inline int find_new_ilb(void)
>> return ilb_cpu;
>> }
>>
>> + /* There is no idle CPU left.
>> + * reset has_blocked_load and needs_update, such that unless
>> + * some CPU enters idle state, it will not trigger kick_ilb
>> + */
cpumask_empty check.
>> + if (READ_ONCE(nohz.has_blocked_load))
>> + WRITE_ONCE(nohz.has_blocked_load, 0);
>> + if (READ_ONCE(nohz.needs_update))
>> + WRITE_ONCE(nohz.needs_update, 0);
>> +
>
> I'm slightly skeptical - find_new_ilb() will also fail to find any CPU
> if idle_cpu() returns false momentarily.
>
Yes. I even thought to put cpumask_empty check here and then do these updates.
But since there might be a window where a remote CPU may be going idle,
might race with it and possibly lose a chance for idle balance.
Since it is extreme case of 100% busy, all these checks may not be necessary.
Will put a return instead :)
Thanks for pointing that out.
> Those CPUs can again go back to idle without updating
> "nohz.has_blocked_load" - tick_nohz_idle_stop_tick() will skip
> nohz_balance_enter_idle() if the CPU already had TS_FLAG_STOPPED set.
>
> For those cases, we'll need to retain the "nohz" state as is until a
> suitable ILB CPU can be found - this is the reason why we delay
> clearing the "nohz" state until _nohz_idle_balance() and reconstruct it
> once the CPU is done with idle balancing.
>
> There are also nuances like the smp_mb__after_atomic() in
That reminds me, need to upgrade this to smp_mb now, given atomic is gone.
> nohz_balance_enter_idle() which requires us to check the
> "nohz.idle_cpus_mask" after we are done clearing "nohz.needs_update"
> and "nohz.has_blocked_load".
>
>> return -1;
>> }
>>
>>
>
Powered by blists - more mailing lists