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 10:06:38 -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: <20221218180638.GR4001@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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y550Z+MOq1IX3Wb4@google.com>
Precedence: bulk

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?

							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,