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:   Mon, 26 Sep 2022 10:45:25 -0700
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     Frederic Weisbecker <frederic@kernel.org>
Cc:     Joel Fernandes <joel@joelfernandes.org>, rcu@vger.kernel.org,
        linux-kernel@vger.kernel.org, rushikesh.s.kadam@intel.com,
        urezki@gmail.com, neeraj.iitr10@gmail.com, rostedt@goodmis.org
Subject: Re: [PATCH v6 1/4] rcu: Make call_rcu() lazy to save power
Message-ID: <20220926174525.GP4196@paulmck-ThinkPad-P17-Gen-1>
Reply-To: paulmck@kernel.org
References: <20220924224637.GA161871@lothringen>
 <19217A4C-7183-4D78-A714-FBFE7BB20742@joelfernandes.org>
 <20220925220936.GA182999@lothringen>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20220925220936.GA182999@lothringen>
Precedence: bulk

On Mon, Sep 26, 2022 at 12:09:36AM +0200, Frederic Weisbecker wrote:
> On Sat, Sep 24, 2022 at 07:28:16PM -0400, Joel Fernandes wrote:
> > > And then FLUSH_BP_WAKE is probably not needed anymore. 
> > 
> > It is needed as the API is in tree_nocb.h and we
> > have to have that handle the details of laziness
> > there rather than tree.c. We could add new apis
> > to get rid of flag but it’s cleaner (and Paul seemed
> > to be ok with it).
> 
> If the wake up is handled outside the flush function, as in the
> diff I just posted, there is no more user left of FLUSH_BP_WAKE, IIRC...

To get rid of FLUSH_BP_WAKE, we might need to pull some rcu_data fields
out from under #ifdef in order to allow them to be accessed by common
code.  Which might be a good tradeoff, as the size of rcu_data has not
been a concern.  Plus the increase in size would be quite small.

							Thanx, Paul

> > >> @@ -512,9 +598,16 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone,
> > >>    }
> > >>    // Need to actually to a wakeup.
> > >>    len = rcu_segcblist_n_cbs(&rdp->cblist);
> > >> +    bypass_len = rcu_cblist_n_cbs(&rdp->nocb_bypass);
> > >> +    lazy_len = READ_ONCE(rdp->lazy_len);
> > >>    if (was_alldone) {
> > >>        rdp->qlen_last_fqs_check = len;
> > >> -        if (!irqs_disabled_flags(flags)) {
> > >> +        // Only lazy CBs in bypass list
> > >> +        if (lazy_len && bypass_len == lazy_len) {
> > >> +            rcu_nocb_unlock_irqrestore(rdp, flags);
> > >> +            wake_nocb_gp_defer(rdp, RCU_NOCB_WAKE_LAZY,
> > >> +                       TPS("WakeLazy"));
> > > 
> > > I'm trying to think of a case where rcu_nocb_try_bypass() returns false
> > > (queue to regular list) but then call_rcu() -> __call_rcu_nocb_wake() ends up
> > > seeing a lazy bypass queue even though we are queueing a non-lazy callback
> > > (should have flushed in this case).
> > > 
> > > Looks like it shouldn't happen, even with concurrent (de-offloading) but just
> > > in case, can we add:
> > 
> > Yes I also feel this couldn’t happen because irq is
> > off and nocb lock is held throughout the calls to
> > the above 2 functions. Unless I missed the race
> > you’re describing?
> 
> At least I can't find any either...
> 
> > 
> > > 
> > >      WARN_ON_ONCE(lazy_len != len)
> > 
> > But this condition can be true even in normal
> > circumstances? len also contains DONE CBs
> > which are ready to be invoked. Or did I miss
> > something?
> 
> Duh, good point, nevermind then :-)
> 
> Thanks.
> 
> > 
> > Thanks,
> > 
> >   - Joel
> > 
> > > 
> > >> +        } else if (!irqs_disabled_flags(flags)) {
> > >>            /* ... if queue was empty ... */
> > >>            rcu_nocb_unlock_irqrestore(rdp, flags);
> > >>            wake_nocb_gp(rdp, false);
> > > 
> > > Thanks.