Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 18 Dec 2022 11:44:34 -0800
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     Joel Fernandes <joel@joelfernandes.org>
Cc:     "Zhang, Qiang1" <qiang1.zhang@intel.com>,
        "frederic@kernel.org" <frederic@kernel.org>,
        "quic_neeraju@quicinc.com" <quic_neeraju@quicinc.com>,
        "rcu@vger.kernel.org" <rcu@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] rcu: Fix opposite might_sleep() check in
 rcu_blocking_is_gp()
Message-ID: <20221218194434.GS4001@paulmck-ThinkPad-P17-Gen-1>
Reply-To: paulmck@kernel.org
References: <20221215035755.2820163-1-qiang1.zhang@intel.com>
 <20221217010345.GF4001@paulmck-ThinkPad-P17-Gen-1>
 <PH0PR11MB5880526CB255AFF91F0D76A2DAE79@PH0PR11MB5880.namprd11.prod.outlook.com>
 <PH0PR11MB588000890D537044BBAA209EDAE79@PH0PR11MB5880.namprd11.prod.outlook.com>
 <20221217051759.GK4001@paulmck-ThinkPad-P17-Gen-1>
 <Y550Z+MOq1IX3Wb4@google.com>
 <20221218180638.GR4001@paulmck-ThinkPad-P17-Gen-1>
 <CAEXW_YSy4MyUW55Umtt4LRfX_4Dhdv0h2O=n+Zbq2ijakwGnBw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAEXW_YSy4MyUW55Umtt4LRfX_4Dhdv0h2O=n+Zbq2ijakwGnBw@mail.gmail.com>
Precedence: bulk

On Sun, Dec 18, 2022 at 02:29:10PM -0500, Joel Fernandes wrote:
> 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;

If the scheduler is inactive (early boot with interrupts disabled),
we return here.

> > > -     might_sleep();  /* Check for RCU read-side critical section. */

We get here only if the scheduler has started, and even then only in
preemption-disabled kernels.

Or is you concern that the might_sleep() never gets invoked in kernels
with preemption enabled?  Fixing that would require a slightly different
patch, though.

Or should I have waited until tomorrow to respond to this email?  ;-)

							Thanx, Paul

> > >       preempt_disable();
> > >       /*
> > >        * If the rcu_state.n_online_cpus counter is equal to one,