diff mbox series

[1/4] sched/idle: Fix missing need_resched() check after rcu_idle_enter()

Message ID 20210104152058.36642-2-frederic@kernel.org
State New
Headers show
Series sched/idle: Fix missing need_resched() checks after rcu_idle_enter() v2 | expand

Commit Message

Frederic Weisbecker Jan. 4, 2021, 3:20 p.m. UTC
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.

Unfortunately in default_idle_call(), the call to rcu_idle_enter() is
already beyond the last need_resched() check and we may halt the CPU
with a resched request unhandled, leaving the task hanging.

Fix this with performing a last minute need_resched() check after
calling rcu_idle_enter().

Reported-and-tested-by: Paul E. McKenney <paulmck@kernel.org>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Fixes: 96d3fd0d315a (rcu: Break call_rcu() deadlock involving scheduler and perf)
Cc: stable@vger.kernel.org
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar<mingo@kernel.org>
Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
---
 kernel/sched/idle.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Comments

Peter Zijlstra Jan. 5, 2021, 9:55 a.m. UTC | #1
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.

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).

---

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index de0826411311..612f66c16078 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -95,10 +95,10 @@ static inline void rcu_sysrq_end(void) { }
 #endif /* #else #ifdef CONFIG_RCU_STALL_COMMON */
 
 #ifdef CONFIG_NO_HZ_FULL
-void rcu_user_enter(void);
+bool __must_check rcu_user_enter(void);
 void rcu_user_exit(void);
 #else
-static inline void rcu_user_enter(void) { }
+static inline bool __must_check rcu_user_enter(void) { return true; }
 static inline void rcu_user_exit(void) { }
 #endif /* CONFIG_NO_HZ_FULL */
 
diff --git a/include/linux/rcutree.h b/include/linux/rcutree.h
index df578b73960f..9ba0c5d9e99e 100644
--- a/include/linux/rcutree.h
+++ b/include/linux/rcutree.h
@@ -43,7 +43,7 @@ bool rcu_gp_might_be_stalled(void);
 unsigned long get_state_synchronize_rcu(void);
 void cond_synchronize_rcu(unsigned long oldstate);
 
-void rcu_idle_enter(void);
+bool __must_check rcu_idle_enter(void);
 void rcu_idle_exit(void);
 void rcu_irq_enter(void);
 void rcu_irq_exit(void);
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 40e5e3dd253e..13e19e5db0b8 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -625,7 +625,7 @@ EXPORT_SYMBOL_GPL(rcutorture_get_gp_data);
  * the possibility of usermode upcalls having messed up our count
  * of interrupt nesting level during the prior busy period.
  */
-static noinstr void rcu_eqs_enter(bool user)
+static noinstr bool rcu_eqs_enter(bool user)
 {
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
 
@@ -636,7 +636,7 @@ static noinstr void rcu_eqs_enter(bool user)
 	if (rdp->dynticks_nesting != 1) {
 		// RCU will still be watching, so just do accounting and leave.
 		rdp->dynticks_nesting--;
-		return;
+		return true;
 	}
 
 	lockdep_assert_irqs_disabled();
@@ -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;
+	}
+
 	rcu_prepare_for_idle();
 	rcu_preempt_deferred_qs(current);
 
@@ -657,6 +664,8 @@ static noinstr void rcu_eqs_enter(bool user)
 	rcu_dynticks_eqs_enter();
 	// ... but is no longer watching here.
 	rcu_dynticks_task_enter();
+
+	return true;
 }
 
 /**
@@ -670,10 +679,10 @@ static noinstr void rcu_eqs_enter(bool user)
  * If you add or remove a call to rcu_idle_enter(), be sure to test with
  * CONFIG_RCU_EQS_DEBUG=y.
  */
-void rcu_idle_enter(void)
+bool rcu_idle_enter(void)
 {
 	lockdep_assert_irqs_disabled();
-	rcu_eqs_enter(false);
+	return rcu_eqs_enter(false);
 }
 EXPORT_SYMBOL_GPL(rcu_idle_enter);
 
@@ -689,10 +698,10 @@ EXPORT_SYMBOL_GPL(rcu_idle_enter);
  * If you add or remove a call to rcu_user_enter(), be sure to test with
  * CONFIG_RCU_EQS_DEBUG=y.
  */
-noinstr void rcu_user_enter(void)
+noinstr bool rcu_user_enter(void)
 {
 	lockdep_assert_irqs_disabled();
-	rcu_eqs_enter(true);
+	return rcu_eqs_enter(true);
 }
 #endif /* CONFIG_NO_HZ_FULL */
 
diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index 7708ed161f4a..9226f4021a36 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -433,7 +433,7 @@ static bool rcu_nocb_try_bypass(struct rcu_data *rdp, struct rcu_head *rhp,
 static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_empty,
 				 unsigned long flags);
 static int rcu_nocb_need_deferred_wakeup(struct rcu_data *rdp);
-static void do_nocb_deferred_wakeup(struct rcu_data *rdp);
+static bool do_nocb_deferred_wakeup(struct rcu_data *rdp);
 static void rcu_boot_init_nocb_percpu_data(struct rcu_data *rdp);
 static void rcu_spawn_cpu_nocb_kthread(int cpu);
 static void __init rcu_spawn_nocb_kthreads(void);
diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 7e291ce0a1d6..8ca41b3fe4f9 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -1631,7 +1631,7 @@ bool rcu_is_nocb_cpu(int cpu)
  * Kick the GP kthread for this NOCB group.  Caller holds ->nocb_lock
  * and this function releases it.
  */
-static void wake_nocb_gp(struct rcu_data *rdp, bool force,
+static bool wake_nocb_gp(struct rcu_data *rdp, bool force,
 			   unsigned long flags)
 	__releases(rdp->nocb_lock)
 {
@@ -1654,8 +1654,11 @@ static void wake_nocb_gp(struct rcu_data *rdp, bool force,
 		trace_rcu_nocb_wake(rcu_state.name, rdp->cpu, TPS("DoWake"));
 	}
 	raw_spin_unlock_irqrestore(&rdp_gp->nocb_gp_lock, flags);
-	if (needwake)
+	if (needwake) {
 		wake_up_process(rdp_gp->nocb_gp_kthread);
+		return true;
+	}
+	return false;
 }
 
 /*
@@ -2155,17 +2158,19 @@ static int rcu_nocb_need_deferred_wakeup(struct rcu_data *rdp)
 static void do_nocb_deferred_wakeup_common(struct rcu_data *rdp)
 {
 	unsigned long flags;
+	bool ret;
 	int ndw;
 
 	rcu_nocb_lock_irqsave(rdp, flags);
 	if (!rcu_nocb_need_deferred_wakeup(rdp)) {
 		rcu_nocb_unlock_irqrestore(rdp, flags);
-		return;
+		return false;
 	}
 	ndw = READ_ONCE(rdp->nocb_defer_wakeup);
 	WRITE_ONCE(rdp->nocb_defer_wakeup, RCU_NOCB_WAKE_NOT);
-	wake_nocb_gp(rdp, ndw == RCU_NOCB_WAKE_FORCE, flags);
+	ret = wake_nocb_gp(rdp, ndw == RCU_NOCB_WAKE_FORCE, flags);
 	trace_rcu_nocb_wake(rcu_state.name, rdp->cpu, TPS("DeferredWake"));
+	return ret;
 }
 
 /* Do a deferred wakeup of rcu_nocb_kthread() from a timer handler. */
@@ -2181,10 +2186,12 @@ static void do_nocb_deferred_wakeup_timer(struct timer_list *t)
  * This means we do an inexact common-case check.  Note that if
  * we miss, ->nocb_timer will eventually clean things up.
  */
-static void do_nocb_deferred_wakeup(struct rcu_data *rdp)
+static bool do_nocb_deferred_wakeup(struct rcu_data *rdp)
 {
 	if (rcu_nocb_need_deferred_wakeup(rdp))
-		do_nocb_deferred_wakeup_common(rdp);
+		return do_nocb_deferred_wakeup_common(rdp);
+
+	return false;
 }
 
 void __init rcu_init_nohz(void)
@@ -2518,8 +2525,9 @@ static int rcu_nocb_need_deferred_wakeup(struct rcu_data *rdp)
 	return false;
 }
 
-static void do_nocb_deferred_wakeup(struct rcu_data *rdp)
+static bool do_nocb_deferred_wakeup(struct rcu_data *rdp)
 {
+	return false
 }
 
 static void rcu_spawn_cpu_nocb_kthread(int cpu)
Frederic Weisbecker Jan. 5, 2021, 12:57 p.m. UTC | #2
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.


Heh, yes you're right, that looks saner.

> 

> I'm thinking that rcu_user_enter() will have the exact same problem? Did

> you audit that?


Yes and I wanted to fix it seperately since it's a bit harder to fix because
we are past the last need_resched() check, all syscall exit works, lockdep
hardirqs on entry prep, tracing hardirqs on, etc... I need to manage to
rollback safely and cleanly.

Unless I can decouple the wakeup from rcu_user_enter() and put it around the
exit_to_user_mode_loop(). But then I must make sure that call_rcu() isn't called
afterward.

> 

> Something like the below, combined with a fixup for all callers (which

> the compiler will help us find thanks to __must_check).


Right, I just need to make sure that the wake up is local as the kthread
awaken can be queued anywhere. But a simple need_resched() check after the
wake up should be fine to get that.

Thanks.
Paul E. McKenney Jan. 5, 2021, 11:25 p.m. UTC | #3
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.

> 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?

							Thanx, Paul

> ---

> 

> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h

> index de0826411311..612f66c16078 100644

> --- a/include/linux/rcupdate.h

> +++ b/include/linux/rcupdate.h

> @@ -95,10 +95,10 @@ static inline void rcu_sysrq_end(void) { }

>  #endif /* #else #ifdef CONFIG_RCU_STALL_COMMON */

>  

>  #ifdef CONFIG_NO_HZ_FULL

> -void rcu_user_enter(void);

> +bool __must_check rcu_user_enter(void);

>  void rcu_user_exit(void);

>  #else

> -static inline void rcu_user_enter(void) { }

> +static inline bool __must_check rcu_user_enter(void) { return true; }

>  static inline void rcu_user_exit(void) { }

>  #endif /* CONFIG_NO_HZ_FULL */

>  

> diff --git a/include/linux/rcutree.h b/include/linux/rcutree.h

> index df578b73960f..9ba0c5d9e99e 100644

> --- a/include/linux/rcutree.h

> +++ b/include/linux/rcutree.h

> @@ -43,7 +43,7 @@ bool rcu_gp_might_be_stalled(void);

>  unsigned long get_state_synchronize_rcu(void);

>  void cond_synchronize_rcu(unsigned long oldstate);

>  

> -void rcu_idle_enter(void);

> +bool __must_check rcu_idle_enter(void);

>  void rcu_idle_exit(void);

>  void rcu_irq_enter(void);

>  void rcu_irq_exit(void);

> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c

> index 40e5e3dd253e..13e19e5db0b8 100644

> --- a/kernel/rcu/tree.c

> +++ b/kernel/rcu/tree.c

> @@ -625,7 +625,7 @@ EXPORT_SYMBOL_GPL(rcutorture_get_gp_data);

>   * the possibility of usermode upcalls having messed up our count

>   * of interrupt nesting level during the prior busy period.

>   */

> -static noinstr void rcu_eqs_enter(bool user)

> +static noinstr bool rcu_eqs_enter(bool user)

>  {

>  	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);

>  

> @@ -636,7 +636,7 @@ static noinstr void rcu_eqs_enter(bool user)

>  	if (rdp->dynticks_nesting != 1) {

>  		// RCU will still be watching, so just do accounting and leave.

>  		rdp->dynticks_nesting--;

> -		return;

> +		return true;

>  	}

>  

>  	lockdep_assert_irqs_disabled();

> @@ -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;

> +	}

> +

>  	rcu_prepare_for_idle();

>  	rcu_preempt_deferred_qs(current);

>  

> @@ -657,6 +664,8 @@ static noinstr void rcu_eqs_enter(bool user)

>  	rcu_dynticks_eqs_enter();

>  	// ... but is no longer watching here.

>  	rcu_dynticks_task_enter();

> +

> +	return true;

>  }

>  

>  /**

> @@ -670,10 +679,10 @@ static noinstr void rcu_eqs_enter(bool user)

>   * If you add or remove a call to rcu_idle_enter(), be sure to test with

>   * CONFIG_RCU_EQS_DEBUG=y.

>   */

> -void rcu_idle_enter(void)

> +bool rcu_idle_enter(void)

>  {

>  	lockdep_assert_irqs_disabled();

> -	rcu_eqs_enter(false);

> +	return rcu_eqs_enter(false);

>  }

>  EXPORT_SYMBOL_GPL(rcu_idle_enter);

>  

> @@ -689,10 +698,10 @@ EXPORT_SYMBOL_GPL(rcu_idle_enter);

>   * If you add or remove a call to rcu_user_enter(), be sure to test with

>   * CONFIG_RCU_EQS_DEBUG=y.

>   */

> -noinstr void rcu_user_enter(void)

> +noinstr bool rcu_user_enter(void)

>  {

>  	lockdep_assert_irqs_disabled();

> -	rcu_eqs_enter(true);

> +	return rcu_eqs_enter(true);

>  }

>  #endif /* CONFIG_NO_HZ_FULL */

>  

> diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h

> index 7708ed161f4a..9226f4021a36 100644

> --- a/kernel/rcu/tree.h

> +++ b/kernel/rcu/tree.h

> @@ -433,7 +433,7 @@ static bool rcu_nocb_try_bypass(struct rcu_data *rdp, struct rcu_head *rhp,

>  static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_empty,

>  				 unsigned long flags);

>  static int rcu_nocb_need_deferred_wakeup(struct rcu_data *rdp);

> -static void do_nocb_deferred_wakeup(struct rcu_data *rdp);

> +static bool do_nocb_deferred_wakeup(struct rcu_data *rdp);

>  static void rcu_boot_init_nocb_percpu_data(struct rcu_data *rdp);

>  static void rcu_spawn_cpu_nocb_kthread(int cpu);

>  static void __init rcu_spawn_nocb_kthreads(void);

> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h

> index 7e291ce0a1d6..8ca41b3fe4f9 100644

> --- a/kernel/rcu/tree_plugin.h

> +++ b/kernel/rcu/tree_plugin.h

> @@ -1631,7 +1631,7 @@ bool rcu_is_nocb_cpu(int cpu)

>   * Kick the GP kthread for this NOCB group.  Caller holds ->nocb_lock

>   * and this function releases it.

>   */

> -static void wake_nocb_gp(struct rcu_data *rdp, bool force,

> +static bool wake_nocb_gp(struct rcu_data *rdp, bool force,

>  			   unsigned long flags)

>  	__releases(rdp->nocb_lock)

>  {

> @@ -1654,8 +1654,11 @@ static void wake_nocb_gp(struct rcu_data *rdp, bool force,

>  		trace_rcu_nocb_wake(rcu_state.name, rdp->cpu, TPS("DoWake"));

>  	}

>  	raw_spin_unlock_irqrestore(&rdp_gp->nocb_gp_lock, flags);

> -	if (needwake)

> +	if (needwake) {

>  		wake_up_process(rdp_gp->nocb_gp_kthread);

> +		return true;

> +	}

> +	return false;

>  }

>  

>  /*

> @@ -2155,17 +2158,19 @@ static int rcu_nocb_need_deferred_wakeup(struct rcu_data *rdp)

>  static void do_nocb_deferred_wakeup_common(struct rcu_data *rdp)

>  {

>  	unsigned long flags;

> +	bool ret;

>  	int ndw;

>  

>  	rcu_nocb_lock_irqsave(rdp, flags);

>  	if (!rcu_nocb_need_deferred_wakeup(rdp)) {

>  		rcu_nocb_unlock_irqrestore(rdp, flags);

> -		return;

> +		return false;

>  	}

>  	ndw = READ_ONCE(rdp->nocb_defer_wakeup);

>  	WRITE_ONCE(rdp->nocb_defer_wakeup, RCU_NOCB_WAKE_NOT);

> -	wake_nocb_gp(rdp, ndw == RCU_NOCB_WAKE_FORCE, flags);

> +	ret = wake_nocb_gp(rdp, ndw == RCU_NOCB_WAKE_FORCE, flags);

>  	trace_rcu_nocb_wake(rcu_state.name, rdp->cpu, TPS("DeferredWake"));

> +	return ret;

>  }

>  

>  /* Do a deferred wakeup of rcu_nocb_kthread() from a timer handler. */

> @@ -2181,10 +2186,12 @@ static void do_nocb_deferred_wakeup_timer(struct timer_list *t)

>   * This means we do an inexact common-case check.  Note that if

>   * we miss, ->nocb_timer will eventually clean things up.

>   */

> -static void do_nocb_deferred_wakeup(struct rcu_data *rdp)

> +static bool do_nocb_deferred_wakeup(struct rcu_data *rdp)

>  {

>  	if (rcu_nocb_need_deferred_wakeup(rdp))

> -		do_nocb_deferred_wakeup_common(rdp);

> +		return do_nocb_deferred_wakeup_common(rdp);

> +

> +	return false;

>  }

>  

>  void __init rcu_init_nohz(void)

> @@ -2518,8 +2525,9 @@ static int rcu_nocb_need_deferred_wakeup(struct rcu_data *rdp)

>  	return false;

>  }

>  

> -static void do_nocb_deferred_wakeup(struct rcu_data *rdp)

> +static bool do_nocb_deferred_wakeup(struct rcu_data *rdp)

>  {

> +	return false

>  }

>  

>  static void rcu_spawn_cpu_nocb_kthread(int cpu)
Frederic Weisbecker Jan. 5, 2021, 11:47 p.m. UTC | #4
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.
Peter Zijlstra Jan. 6, 2021, 10:33 a.m. UTC | #5
On Tue, Jan 05, 2021 at 01:57:22PM +0100, Frederic Weisbecker wrote:

> > Something like the below, combined with a fixup for all callers (which

> > the compiler will help us find thanks to __must_check).

> 

> Right, I just need to make sure that the wake up is local as the kthread

> awaken can be queued anywhere. But a simple need_resched() check after the

> wake up should be fine to get that.


Duh, yes. Clearly I'm having startup problems after the holidays ;-)
diff mbox series

Patch

diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index 305727ea0677..1af60dc50beb 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -109,15 +109,21 @@  void __cpuidle default_idle_call(void)
 		rcu_idle_enter();
 		lockdep_hardirqs_on(_THIS_IP_);
 
-		arch_cpu_idle();
+		/*
+		 * Last need_resched() check must come after rcu_idle_enter()
+		 * which may wake up RCU internal tasks.
+		 */
+		if (!need_resched()) {
+			arch_cpu_idle();
+			raw_local_irq_disable();
+		}
 
 		/*
-		 * OK, so IRQs are enabled here, but RCU needs them disabled to
-		 * turn itself back on.. funny thing is that disabling IRQs
-		 * will cause tracing, which needs RCU. Jump through hoops to
-		 * make it 'work'.
+		 * OK, so IRQs are enabled after arch_cpu_idle(), but RCU needs
+		 * them disabled to turn itself back on.. funny thing is that
+		 * disabling IRQs will cause tracing, which needs RCU. Jump through
+		 * hoops to make it 'work'.
 		 */
-		raw_local_irq_disable();
 		lockdep_hardirqs_off(_THIS_IP_);
 		rcu_idle_exit();
 		lockdep_hardirqs_on(_THIS_IP_);