On Mon, Oct 03, 2022 at 07:33:24PM +0000, Joel Fernandes wrote:
> 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.
Having this as a separate simplification commit is fine by me.
And thank you for digging into this!
Thanx, Paul
> 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
>
