On Tue, Jan 05, 2021 at 03:25:10PM -0800, Paul E. McKenney wrote:
On Tue, Jan 05, 2021 at 10:55:03AM +0100, Peter Zijlstra wrote:
On Mon, Jan 04, 2021 at 04:20:55PM +0100, Frederic Weisbecker wrote:
Entering RCU idle mode may cause a deferred wake up of an RCU NOCB_GP kthread (rcuog) to be serviced.
Usually a wake up happening while running the idle task is spotted in one of the need_resched() checks carefully placed within the idle loop that can break to the scheduler.
Urgh, this is horrific and fragile :/ You having had to audit and fix a number of rcu_idle_enter() callers should've made you realize that making rcu_idle_enter() return something would've been saner.
Also, I might hope that when RCU does do that wakeup, it will not have put RCU in idle mode? So it is a natural 'fail' state for rcu_idle_enter(), *sigh* it continues to put RCU to sleep, so that needs fixing too.
It depends on what is being awakened. For example, the nocb rcuog and rcuoc kthreads might be well on some other CPU, so RCU might need the wakeup to happen, but might also need to go completely to sleep on this CPU.
But yes, if the wakeup needs to be on the current CPU, then idle must be exited and RCU needs to again be watching. However, RCU has no idea what CPU the to-be-awakened kthread will be running on. And even if it were to know at the time it does the wakeup, that kthread's location might well have changed by the time the current CPU enters idle.
A simple check for need_resched() would do the trick. Sure that could also catch other sources of wake up that would have been otherwise handled once IRQs get re-enabled but that's not a problem.
I'm thinking that rcu_user_enter() will have the exact same problem? Did you audit that?
Something like the below, combined with a fixup for all callers (which the compiler will help us find thanks to __must_check).
Looks at least somewhat plausible at first glance.
Though given the above, it is possible (likely, even) that rcu_user_enter() returns true, but that this CPU still needs to enter idle. So isn't a subsequent check of need_resched() or friends still required? Or is your point that this will happen automatically upon exit from the idle loop?
Exactly, upon "wake_up_process(rdp_gp->nocb_gp_kthread)", we just need to make sure that need_resched() is set before returning false, namely:
@@ -644,7 +644,14 @@ static noinstr void rcu_eqs_enter(bool user) trace_rcu_dyntick(TPS("Start"), rdp->dynticks_nesting, 0, atomic_read(&rdp->dynticks)); WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !user && !is_idle_task(current)); rdp = this_cpu_ptr(&rcu_data);
- do_nocb_deferred_wakeup(rdp);
- if (do_nocb_deferred_wakeup(rdp)) {
/** We did the wakeup, don't enter EQS, we'll need to abort idle* and schedule.*/return false;
Right here.
But still I think we should decouple the wake up from rcu_eqs_enter().
And have:
rcu_eqs_enter_prepare(): does the deferred wakeup and forbid from calling call_rcu() from here.
rcu_eqs_enter(): enter RCU extended quiescent state
This way we can more easily fix the rcu_user_enter() case as it happens past the last scheduler entrypoint before returning to userspace.
Thanks.