linux-kernel - Re: [PATCH 1/3] sched/core: Remove the unnecessary need_resched() check in nohz_csd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <027912c1-c158-8a3a-6d50-9c08dcf4db31@amd.com>
Date: Wed, 10 Jul 2024 23:27:49 +0530
From: K Prateek Nayak <kprateek.nayak@....com>
To: Peter Zijlstra <peterz@...radead.org>
CC: Ingo Molnar <mingo@...hat.com>, Juri Lelli <juri.lelli@...hat.com>,
	Vincent Guittot <vincent.guittot@...aro.org>, <linux-kernel@...r.kernel.org>,
	Dietmar Eggemann <dietmar.eggemann@....com>, Steven Rostedt
	<rostedt@...dmis.org>, Ben Segall <bsegall@...gle.com>, Mel Gorman
	<mgorman@...e.de>, Daniel Bristot de Oliveira <bristot@...hat.com>, "Valentin
 Schneider" <vschneid@...hat.com>, "Paul E. McKenney" <paulmck@...nel.org>,
	Imran Khan <imran.f.khan@...cle.com>, Leonardo Bras <leobras@...hat.com>,
	"Guo Ren" <guoren@...nel.org>, Rik van Riel <riel@...riel.com>, Tejun Heo
	<tj@...nel.org>, Cruz Zhao <CruzZhao@...ux.alibaba.com>, Lai Jiangshan
	<jiangshanlai@...il.com>, Joel Fernandes <joel@...lfernandes.org>, Zqiang
	<qiang.zhang1211@...il.com>, Julia Lawall <julia.lawall@...ia.fr>, "Gautham
 R. Shenoy" <gautham.shenoy@....com>
Subject: Re: [PATCH 1/3] sched/core: Remove the unnecessary need_resched()
 check in nohz_csd_func()

Hello Peter,

On 7/10/2024 8:23 PM, Peter Zijlstra wrote:
> On Wed, Jul 10, 2024 at 09:02:08AM +0000, K Prateek Nayak wrote:
>> The need_resched() check currently in nohz_csd_func() can be tracked
>> to have been added in scheduler_ipi() back in 2011 via commit
>> ca38062e57e9 ("sched: Use resched IPI to kick off the nohz idle balance")
>>
>> Since then, it has travelled quite a bit but it seems like an idle_cpu()
>> check currently is sufficient to detect the need to bail out from an
>> idle load balancing. To justify this removal, consider all the following
>> case where an idle load balancing could race with a task wakeup:
>>
>> o Since commit f3dd3f674555b ("sched: Remove the limitation of WF_ON_CPU
>>    on wakelist if wakee cpu is idle") a target perceived to be idle
>>    (target_rq->nr_running == 0) will return true for
>>    ttwu_queue_cond(target) which will offload the task wakeup to the idle
>>    target via an IPI.
>>
>>    In all such cases target_rq->ttwu_pending will be set to 1 before
>>    queuing the wake function.
>>
>>    If an idle load balance races here, following scenarios are possible:
>>
>>    - The CPU is not in TIF_POLLING_NRFLAG mode in which case an actual
>>      IPI is sent to the CPU to wake it out of idle. If the
>>      nohz_csd_func() queues before sched_ttwu_pending(), the idle load
>>      balance will bail out since idle_cpu(target) returns 0 since
>>      target_rq->ttwu_pending is 1. If the nohz_csd_func() is queued after
>>      sched_ttwu_pending() it should see rq->nr_running to be non-zero and
>>      bail out of idle load balancing.
>>
>>    - The CPU is in TIF_POLLING_NRFLAG mode and instead of an actual IPI,
>>      the sender will simply set TIF_NEED_RESCHED for the target to put it
>>      out of idle and flush_smp_call_function_queue() in do_idle() will
>>      execute the call function. Depending on the ordering of the queuing
>>      of nohz_csd_func() and sched_ttwu_pending(), the idle_cpu() check in
>>      nohz_csd_func() should either see target_rq->ttwu_pending = 1 or
>>      target_rq->nr_running to be non-zero if there is a genuine task
>>      wakeup racing with the idle load balance kick.
> 
> For completion sake, we should also consider the !TTWU_QUEUE case, this
> configuration is default for PREEMPT_RT, where the wake_list is a source
> of non-determinism.
> 
> In quick reading I think that case should be fine, since we directly
> enqueue remotely and ->nr_running adjusts accordingly, but it is late in
> the day and I'm easily mistaken.

 From what I've seen, an enqueue will always update "rq->nr_running"
before setting the "NEED_RESCHED" flag but I'll go confirm that again
and report back in case what that is false.

> 
>> o The waker CPU perceives the target CPU to be busy
>>    (targer_rq->nr_running != 0) but the CPU is in fact going idle and due
>>    to a series of unfortunate events, the system reaches a case where the
>>    waker CPU decides to perform the wakeup by itself in ttwu_queue() on
>>    the target CPU but target is concurrently selected for idle load
>>    balance (Can this happen? I'm not sure, but we'll consider its
>>    possibility to estimate the worst case scenario).
>>
>>    ttwu_do_activate() calls enqueue_task() which would increment
>>    "rq->nr_running" post which it calls wakeup_preempt() which is
>>    responsible for setting TIF_NEED_RESCHED (via a resched IPI or by
>>    setting TIF_NEED_RESCHED on a TIF_POLLING_NRFLAG idle CPU) The key
>>    thing to note in this case is that rq->nr_running is already non-zero
>>    in case of a wakeup before TIF_NEED_RESCHED is set which would
>>    lead to idle_cpu() check returning false.
>>
>> In all cases, it seems that need_resched() check is unnecessary when
>> checking for idle_cpu() first since an impending wakeup racing with idle
>> load balancer will either set the "rq->ttwu_pending" or indicate a newly
>> woken task via "rq->nr_running".
> 
> Right.
> 
>> [..snip..]

-- 
Thanks and Regards,
Prateek