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,
