Re: rcu: WARNING in rcu_seq_end

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Tue, Mar 07, 2017 at 07:37:57PM +0100, Dmitry Vyukov wrote:
> On Tue, Mar 7, 2017 at 4:27 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> >> > [...]
> >> >> >>
> >> >> >> What is that mutex? And what locks/unlocks provide synchronization? I
> >> >> >> see that one uses exp_mutex and another -- exp_wake_mutex.
> >> >> >
> >> >> > Both of them.
> >> >> >
> >> >> > ->exp_mutex is acquired by the task requesting the grace period, and
> >> >> > the counter's first increment is done by that task under that mutex.
> >> >> > This task then schedules a workqueue, which drives forward the grace
> >> >> > period.  Upon grace-period completion, the workqueue handler does the
> >> >> > second increment (the one that your patch addressed).  The workqueue
> >> >> > handler then acquires ->exp_wake_mutex and wakes the task that holds
> >> >> > ->exp_mutex (along with all other tasks waiting for this grace period),
> >> >> > and that task releases ->exp_mutex, which allows the next grace period to
> >> >> > start (and the first increment for that next grace period to be carried
> >> >> > out under that lock).  The workqueue handler releases ->exp_wake_mutex
> >> >> > after finishing its wakeups.
> >> >>
> >> >> Then we need the following for the case when task requesting the grace
> >> >> period does not block, right?
> >> >
> >> > Won't be necessary I think, as the smp_mb() in rcu_seq_end() and the
> >> > smp_mb__before_atomic() in sync_exp_work_done() already provide the
> >> > required ordering, no?
> >>
> >> smp_mb() is probably fine, but smp_mb__before_atomic() is release not
> >> acquire. If we want to play that game, then I guess we also need
> >> smp_mb__after_atomic() there. But it would be way easier to understand
> >> what's happens there and prove that it's correct, if we use
> >> store_release/load_acquire.
> >
> > Fair point, how about the following?
> 
> I am not qualified enough to reason about these smp_mb__after_atomic.
> >From practical point of view there may be enough barriers in the
> resulting machine code already, but re formal semantics of adding
> smp_mb__after_atomic after an unrelated subsequent atomic RMW op I
> gave up. You must be the best candidate for this now :)

Unfortunately, there are code paths from sync_exp_work_done() that
have no memory barriers.  :-(

And I might be the best candidate, but this email thread has definitely
shown that I am not infallable, never mind that there was already
plenty of evidence on this particular point.  So thank you again for
your testing and review efforts!

                                                        Thanx, Paul

> > ------------------------------------------------------------------------
> >
> > commit 6fd8074f1976596898e39f5b7ea1755652533906
> > Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Date:   Tue Mar 7 07:21:23 2017 -0800
> >
> >     rcu: Add smp_mb__after_atomic() to sync_exp_work_done()
> >
> >     The sync_exp_work_done() function needs to fully order the counter-check
> >     operation against anything happening after the corresponding grace period.
> >     This is a theoretical bug, as all current architectures either provide
> >     full ordering for atomic operation on the one hand or implement,
> >     however, a little future-proofing is a good thing.  This commit
> >     therefore adds smp_mb__after_atomic() after the atomic_long_inc()
> >     in sync_exp_work_done().
> >
> >     Reported-by: Dmitry Vyukov <dvyukov@google.com>
> >     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> >
> > diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> > index 027e123d93c7..652071abd9b4 100644
> > --- a/kernel/rcu/tree_exp.h
> > +++ b/kernel/rcu/tree_exp.h
> > @@ -247,6 +247,7 @@ static bool sync_exp_work_done(struct rcu_state *rsp, atomic_long_t *stat,
> >                 /* Ensure test happens before caller kfree(). */
> >                 smp_mb__before_atomic(); /* ^^^ */
> >                 atomic_long_inc(stat);
> > +               smp_mb__after_atomic(); /* ^^^ */
> >                 return true;
> >         }
> >         return false;
>