Re: [PATCH] rcu: Fix opposite might_sleep() check in rcu_blocking_is_gp()

[Joel Fernandes]

On Fri, Dec 16, 2022 at 09:17:59PM -0800, Paul E. McKenney wrote:
> On Sat, Dec 17, 2022 at 02:44:47AM +0000, Zhang, Qiang1 wrote:
> > 
> > On Thu, Dec 15, 2022 at 11:57:55AM +0800, Zqiang wrote:
> > > Currently, if the system is in the RCU_SCHEDULER_INACTIVE state, invoke
> > > synchronize_rcu_*() will implies a grace period and return directly,
> > > so there is no sleep action due to waiting for a grace period to end,
> > > but this might_sleep() check is the opposite. therefore, this commit
> > > puts might_sleep() check in the correct palce.
> > > 
> > > Signed-off-by: Zqiang <qiang1.zhang@xxxxxxxxx>
> > >
> > >Queued for testing and review, thank you!
> > >
> > >I was under the impression that might_sleep() did some lockdep-based
> > >checking, but I am unable to find it.  If there really is such checking,
> > >that would be a potential argument for leaving this code as it is.
> > >
> > >
> > >__might_sleep
> > >   __might_resched(file, line, 0)
> > >      rcu_sleep_check()
> > >
> > >Does it refer to this rcu_sleep_check() ?
> > >
> > >If so, when in the RCU_SCHEDULER_INACTIVE state,  the debug_lockdep_rcu_enabled() is always
> > >return false, so the RCU_LOCKDEP_WARN() also does not produce an actual warning.
> > 
> > and when the system_state == SYSTEM_BOOTING, we just did  rcu_sleep_check()  and then  return.
> 
> Very good, thank you!
> 
> Thoughts from others?

Please consider this as a best-effort comment that might be missing details:

The might_sleep() was added in 18fec7d8758d ("rcu: Improve synchronize_rcu()
diagnostics")

Since it is illegal to call a blocking API like synchronize_rcu() in a
non-preemptible section, is there any harm in just calling might_sleep()
uncomditionally in rcu_block_is_gp() ? I think it is a bit irrelevant if
synchronize_rcu() is called from a call path, before scheduler is
initialized, or after. The fact that it was even called from a
non-preemptible section is a red-flag, considering if such non-preemptible
section may call synchronize_rcu() API in the future, after full boot up,
even if rarely.

For this reason, IMHO there is still value in doing the might_sleep() check
unconditionally. Say if a common code path is invoked both before
RCU_SCHEDULER_INIT and *very rarely* after RCU_SCHEDULER_INIT.

Or is there more of a point in doing this check if scheduler is initialized
from RCU perspective ?

If not, I would do something like this:

---8<-----------------------

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 79aea7df4345..23c2303de9f4 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3435,11 +3435,12 @@ static int rcu_blocking_is_gp(void)
 {
 	int ret;
 
+	might_sleep();  /* Check for RCU read-side critical section. */
+
 	// Invoking preempt_model_*() too early gets a splat.
 	if (rcu_scheduler_active == RCU_SCHEDULER_INACTIVE ||
 	    preempt_model_full() || preempt_model_rt())
 		return rcu_scheduler_active == RCU_SCHEDULER_INACTIVE;
-	might_sleep();  /* Check for RCU read-side critical section. */
 	preempt_disable();
 	/*
 	 * If the rcu_state.n_online_cpus counter is equal to one,