lkml.org 
[lkml]   [2022]   [Dec]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] rcu: Fix opposite might_sleep() check in rcu_blocking_is_gp()
On Sun, Dec 18, 2022 at 1:06 PM Paul E. McKenney <paulmck@kernel.org> wrote:
>
> On Sun, Dec 18, 2022 at 02:01:11AM +0000, Joel Fernandes wrote:
> > 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@intel.com>
> > > > >
> > > > >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 ?
>
> One advantage of its current placement would be if might_sleep() ever
> unconditionally checks for interrupts being disabled.
>
> I don't believe that might_sleep() will do that any time soon given the
> likely fallout from code invoked at early boot as well as from runtime,
> but why be in the way of that additional diagnostic check?

If I understand the current code, might_sleep() is invoked only if the
scheduler is INACTIVE from RCU perspective, and I don't think here are
reports of fall out. That is current code behavior.

Situation right now is: might_sleep() only if the state is INACTIVE.
Qiang's patch: might_sleep() only if the state is NOT INACTIVE.
My suggestion: might_sleep() regardless of the state.

Is there a reason my suggestion will not work? Apologies if I
misunderstood something.

thanks,

- Joel


>
> Thanx, Paul
>
> > 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,

\
 
 \ /
  Last update: 2022-12-18 20:30    [W:0.145 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site