Re: [PATCH v6 1/4] rcu: Make call_rcu() lazy to save power

[Joel Fernandes]

On Mon, Sep 26, 2022 at 04:53:51PM -0700, Paul E. McKenney wrote:
> On Mon, Sep 26, 2022 at 07:33:17PM -0400, Joel Fernandes wrote:
> > 
> > 
> > > On Sep 26, 2022, at 6:37 PM, Paul E. McKenney <paulmck@xxxxxxxxxx> wrote:
> > > 
> > > On Mon, Sep 26, 2022 at 09:07:12PM +0000, Joel Fernandes wrote:
> > >> Hi Paul,
> > >> 
> > >> On Mon, Sep 26, 2022 at 10:42:40AM -0700, Paul E. McKenney wrote:
> > >> [..]
> > >>>>>>>> +        WRITE_ONCE(rdp->lazy_len, 0);
> > >>>>>>>> +    } else {
> > >>>>>>>> +        rcu_cblist_flush_enqueue(&rcl, &rdp->nocb_bypass, rhp);
> > >>>>>>>> +        WRITE_ONCE(rdp->lazy_len, 0);
> > >>>>>>> 
> > >>>>>>> This WRITE_ONCE() can be dropped out of the "if" statement, correct?
> > >>>>>> 
> > >>>>>> Yes will update.
> > >>>>> 
> > >>>>> Thank you!
> > >>>>> 
> > >>>>>>> If so, this could be an "if" statement with two statements in its "then"
> > >>>>>>> clause, no "else" clause, and two statements following the "if" statement.
> > >>>>>> 
> > >>>>>> I don’t think we can get rid of the else part but I’ll see what it looks like.
> > >>>>> 
> > >>>>> In the function header, s/rhp/rhp_in/, then:
> > >>>>> 
> > >>>>>    struct rcu_head *rhp = rhp_in;
> > >>>>> 
> > >>>>> And then:
> > >>>>> 
> > >>>>>    if (lazy && rhp) {
> > >>>>>        rcu_cblist_enqueue(&rdp->nocb_bypass, rhp);
> > >>>>>        rhp = NULL;
> > >>>> 
> > >>>> This enqueues on to the bypass list, where as if lazy && rhp, I want to queue
> > >>>> the new rhp on to the main cblist. So the pseudo code in my patch is:
> > >>>> 
> > >>>> if (lazy and rhp) then
> > >>>>    1. flush bypass CBs on to main list.
> > >>>>    2. queue new CB on to main list.
> > >>> 
> > >>> And the difference is here, correct?  I enqueue to the bypass list,
> > >>> which is then flushed (in order) to the main list.  In contrast, you
> > >>> flush the bypass list, then enqueue to the main list.  Either way,
> > >>> the callback referenced by rhp ends up at the end of ->cblist.
> > >>> 
> > >>> Or am I on the wrong branch of this "if" statement?
> > >> 
> > >> But we have to flush first, and then queue the new one. Otherwise wouldn't
> > >> the callbacks be invoked out of order? Or did I miss something?
> > > 
> > > I don't think so...
> > > 
> > > We want the new callback to be last, right?  One way to do that is to
> > > flush the bypass, then queue the new callback onto ->cblist.  Another way
> > > to do that is to enqueue the new callback onto the end of the bypass,
> > > then flush the bypass.  Why wouldn't these result in the same order?
> > 
> > Yes you are right, sorry. I was fixated on the main list. Both your snippet and my patch will be equivalent then. However I find your snippet a bit confusing, as in it is not immediately obvious - why would we queue something on to a list, if we were about to flush it. But any way, it does make it a clever piece of code in some sense and I am ok with doing it this way ;-)
> 
> As long as the ->cblist.len comes out with the right value.  ;-)

The ->cblist.len's value is not effected by your suggested change, because
the bypass list's length is already accounted into the ->cblist.len, and for
the new rhp, after rcu_nocb_do_flush_bypass() is called, it either ends up in
the bypass list (if it is !lazy) or on the main cblist (if its lazy). So
everything just works. Below is the change. If its OK with you though, I will
put it in a separate commit just to be extra safe, since the code before it
was well tested and I am still testing it.

Thanks.

---8<-----------------------

From: "Joel Fernandes (Google)" <joel@xxxxxxxxxxxxxxxxx>
Subject: [PATCH] rcu: Refactor code a bit in rcu_nocb_do_flush_bypass()

This consolidates the code a bit and makes it cleaner. Functionally it
is the same.

Reported-by: Paul E. McKenney <paulmck@xxxxxxxxxx>
Signed-off-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>
---
 kernel/rcu/tree_nocb.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
index d69d058a78f9..1fc704d102a3 100644
--- a/kernel/rcu/tree_nocb.h
+++ b/kernel/rcu/tree_nocb.h
@@ -327,10 +327,11 @@ static void wake_nocb_gp_defer(struct rcu_data *rdp, int waketype,
  *
  * Note that this function always returns true if rhp is NULL.
  */
-static bool rcu_nocb_do_flush_bypass(struct rcu_data *rdp, struct rcu_head *rhp,
+static bool rcu_nocb_do_flush_bypass(struct rcu_data *rdp, struct rcu_head *rhp_in,
 				     unsigned long j, unsigned long flush_flags)
 {
 	struct rcu_cblist rcl;
+	struct rcu_head *rhp = rhp_in;
 	bool lazy = flush_flags & FLUSH_BP_LAZY;
 
 	WARN_ON_ONCE(!rcu_rdp_is_offloaded(rdp));
@@ -347,16 +348,15 @@ static bool rcu_nocb_do_flush_bypass(struct rcu_data *rdp, struct rcu_head *rhp,
 	/*
 	 * If the new CB requested was a lazy one, queue it onto the main
 	 * ->cblist so that we can take advantage of the grace-period that will
-	 * happen regardless.
+	 * happen regardless. But queue it onto the bypass list first so that
+	 * the lazy CB is ordered with the existing CBs in the bypass list.
 	 */
 	if (lazy && rhp) {
-		rcu_cblist_flush_enqueue(&rcl, &rdp->nocb_bypass, NULL);
-		rcu_cblist_enqueue(&rcl, rhp);
-		WRITE_ONCE(rdp->lazy_len, 0);
-	} else {
-		rcu_cblist_flush_enqueue(&rcl, &rdp->nocb_bypass, rhp);
-		WRITE_ONCE(rdp->lazy_len, 0);
+		rcu_cblist_enqueue(&rdp->nocb_bypass, rhp);
+		rhp = NULL;
 	}
+	rcu_cblist_flush_enqueue(&rcl, &rdp->nocb_bypass, rhp);
+	WRITE_ONCE(rdp->lazy_len, 0);
 
 	rcu_segcblist_insert_pend_cbs(&rdp->cblist, &rcl);
 	WRITE_ONCE(rdp->nocb_bypass_first, j);
-- 
2.38.0.rc1.362.ged0d419d3c-goog