Messages in this thread | | | Date | Tue, 7 Mar 2017 11:09:31 -0800 | From | "Paul E. McKenney" <> | Subject | Re: rcu: WARNING in rcu_seq_end |
| |
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; >
| |