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

["Paul E. McKenney" <paulmck@kernel.org>]

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?

> > > else
> > > 	1. flush bypass CBs on to main list
> > > 	2. queue new CB on to bypass list.
> > > 
> > > > 	}
> > > > 	rcu_cblist_flush_enqueue(&rcl, &rdp->nocb_bypass, rhp);
> > > > 	WRITE_ONCE(rdp->lazy_len, 0);
> > > > 
> > > > Or did I mess something up?
> > > 
> > > So the rcu_cblist_flush_enqueue() has to happen before the
> > > rcu_cblist_enqueue() to preserve the ordering of flushing into the main list,
> > > and queuing on to the main list for the "if". Where as in your snip, the
> > > order is reversed.
> > 
> > Did I pick the correct branch of the "if" statement above?  Or were you
> > instead talking about the "else" clause?
> > 
> > I would have been more worried about getting cblist->len right.
> 
> Hmm, I think my concern was more the ordering of callbacks, and moving the
> write to length should be Ok.

OK, sounds good to me!  ;-)

> > > If I consolidate it then, it looks like the following. However, it is a bit
> > > more unreadable. I could instead just take the WRITE_ONCE out of both if/else
> > > and move it to after the if/else, that would be cleanest. Does that sound
> > > good to you? Thanks!
> > 
> > Let's first figure out whether or not we are talking past one another.  ;-)
> 
> Haha yeah :-)

So were we?  ;-)

							Thanx, Paul