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:   Wed, 7 Sep 2022 11:40:14 +0200
From:   Frederic Weisbecker <frederic@kernel.org>
To:     Joel Fernandes <joel@joelfernandes.org>
Cc:     rcu@vger.kernel.org, linux-kernel@vger.kernel.org,
        rushikesh.s.kadam@intel.com, urezki@gmail.com,
        neeraj.iitr10@gmail.com, paulmck@kernel.org, rostedt@goodmis.org,
        vineeth@bitbyteword.org, boqun.feng@gmail.com
Subject: Re: [PATCH v5 06/18] rcu: Introduce call_rcu_lazy() API
 implementation
Message-ID: <20220907094014.GA193994@lothringen>
References: <20220901221720.1105021-1-joel@joelfernandes.org>
 <20220901221720.1105021-7-joel@joelfernandes.org>
 <20220902152132.GA115525@lothringen>
 <Yxa5Ch574cRZxRdo@google.com>
 <20220906151757.GA183806@lothringen>
 <Yxfggt1/cLBhUi/y@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Yxfggt1/cLBhUi/y@google.com>
Precedence: bulk

On Wed, Sep 07, 2022 at 12:06:26AM +0000, Joel Fernandes wrote:
> > > @@ -326,13 +372,20 @@ static bool rcu_nocb_do_flush_bypass(struct rcu_data *rdp, struct rcu_head *rhp,
> > >   * Note that this function always returns true if rhp is NULL.
> > >   */
> > >  static bool rcu_nocb_flush_bypass(struct rcu_data *rdp, struct rcu_head *rhp,
> > > -				  unsigned long j)
> > > +				  unsigned long j, unsigned long flush_flags)
> > >  {
> > > +	bool ret;
> > > +
> > >  	if (!rcu_rdp_is_offloaded(rdp))
> > >  		return true;
> > >  	rcu_lockdep_assert_cblist_protected(rdp);
> > >  	rcu_nocb_bypass_lock(rdp);
> > > -	return rcu_nocb_do_flush_bypass(rdp, rhp, j);
> > > +	ret = rcu_nocb_do_flush_bypass(rdp, rhp, j, flush_flags);
> > > +
> > > +	if (flush_flags & FLUSH_BP_WAKE)
> > > +		wake_nocb_gp(rdp, true);
> > 
> > Why the true above?
> > 
> > Also should we check if the wake up is really necessary (otherwise it means we
> > force a wake up for all rdp's from rcu_barrier())?
> > 
> >        was_alldone = rcu_segcblist_pend_cbs(&rdp->cblist);
> >        ret = rcu_nocb_do_flush_bypass(rdp, rhp, j, flush_flags);
> >        if (was_alldone && rcu_segcblist_pend_cbs(&rdp->cblist))
> >        	  wake_nocb_gp(rdp, false);
> 
> You mean something like the following right? Though I'm thinking if its
> better to call wake_nocb_gp() from tree.c in entrain() and let that handle
> the wake. That way, we can get rid of the extra FLUSH_BP flags as well and
> let the flush callers deal with the wakeups..

Ah yes that could make sense if only one caller cares.

> 
> Anyway, for testing this should be good...
> 
> ---8<-----------------------
> 
> diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
> index bd8f39ee2cd0..e3344c262672 100644
> --- a/kernel/rcu/tree_nocb.h
> +++ b/kernel/rcu/tree_nocb.h
> @@ -382,15 +382,19 @@ static bool rcu_nocb_flush_bypass(struct rcu_data *rdp, struct rcu_head *rhp,
>  				  unsigned long j, unsigned long flush_flags)
>  {
>  	bool ret;
> +	bool was_alldone;
>  
>  	if (!rcu_rdp_is_offloaded(rdp))
>  		return true;
>  	rcu_lockdep_assert_cblist_protected(rdp);
>  	rcu_nocb_bypass_lock(rdp);
> +	if (flush_flags & FLUSH_BP_WAKE)
> +		was_alldone = !rcu_segcblist_pend_cbs(&rdp->cblist);
> +

You can check that outside bypass lock (but you still need nocb_lock).

>  	ret = rcu_nocb_do_flush_bypass(rdp, rhp, j, flush_flags);
>  
> -	if (flush_flags & FLUSH_BP_WAKE)
> -		wake_nocb_gp(rdp, true);
> +	if (flush_flags & FLUSH_BP_WAKE && was_alldone)
> +		wake_nocb_gp(rdp, false);

That doesn't check if the bypass list was empty.

Thanks.