On Tue, Mar 07, 2017 at 06:26:03PM -0800, Paul E. McKenney wrote:
> On Wed, Mar 08, 2017 at 09:39:13AM +0800, Boqun Feng wrote:
> > On Tue, Mar 07, 2017 at 03:31:54PM -0800, Paul E. McKenney wrote:
> > > On Wed, Mar 08, 2017 at 07:05:13AM +0800, Boqun Feng wrote:
> > > > On Tue, Mar 07, 2017 at 07:27:15AM -0800, Paul E. McKenney wrote:
> > > > > On Tue, Mar 07, 2017 at 03:43:42PM +0100, Dmitry Vyukov wrote:
> > > > > > On Tue, Mar 7, 2017 at 3:27 PM, Boqun Feng <boqun.f...@gmail.com>
> > > > > > wrote:
> > > > > > > On Tue, Mar 07, 2017 at 08:05:19AM +0100, Dmitry Vyukov 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
> > > >
> > > > The point is that smp_mb__before_atomic() + atomic_long_inc() will
> > > > guarantee a smp_mb() before or right along with the atomic operation,
> > > > and that's enough because rcu_seq_done() followed by a smp_mb() will
> > > > give it a acquire-like behavior.
> > >
> > > Given current architectures, true enough, from what I can see.
> > >
> > > However, let's take a look at atomic_ops.rst:
> > >
> > >
> > > If a caller requires memory barrier semantics around an atomic_t
> > > operation which does not return a value, a set of interfaces are
> > > defined which accomplish this::
> > >
> > > void smp_mb__before_atomic(void);
> > > void smp_mb__after_atomic(void);
> > >
> > > For example, smp_mb__before_atomic() can be used like so::
> > >
> > > obj->dead = 1;
> > > smp_mb__before_atomic();
> > > atomic_dec(&obj->ref_count);
> > >
> > > It makes sure that all memory operations preceding the atomic_dec()
> > > call are strongly ordered with respect to the atomic counter
> > > operation. In the above example, it guarantees that the assignment of
> > > "1" to obj->dead will be globally visible to other cpus before the
> > > atomic counter decrement.
> > >
> > > Without the explicit smp_mb__before_atomic() call, the
> > > implementation could legally allow the atomic counter update visible
> > > to other cpus before the "obj->dead = 1;" assignment.
> > >
> > > So the ordering is guaranteed against the atomic operation, not
> > > necessarily the stuff after it. But again, the implementations I know
> > > of do make the guarantee, hence my calling it a theoretical bug in the
> > > commit log.
> >
> > Fair enough ;-) It's me who misunderstood this part of document.
> >
> > However, the names of the barriers are smp_mb__{before,after}_atomic(),
> > so if they, semantically, only provide ordering for the corresponding
> > atomic ops rather than a full barrier, I would their names are
> > misleading ;-)
>
> Well, if you have both ordering before and after, then you have full
> ordering.
>
I mean the names of the barriers are *smp_mb*__before_atomic() and
*smp_mb*__after_atomic(), so it's natural to think they provide a
smp_mb() in some situations ;-)
> > > > > > smp_mb__after_atomic() there. But it would be way easier to
> > > > > > understand
> > > >
> > > > Adding smp_mb__after_atomic() would be pointless as it's the load of
> > > > ->expedited_sequence that we want to ensure having acquire behavior
> > > > rather than the atomic increment of @stat.
> > >
> > > Again, agreed given current code, but atomic_ops.rst doesn't guarantee
> > > ordering past the actual atomic operation itself.
> >
> > Neither does atomic_ops.rst guarantee the ordering between a load before
> > the atomic op and memory accesses after the atomic op, right? I.e.
> > atomic_ops.rst doesn't say no for reordering like this:
> >
> > r1 = READ_ONCE(a); ---------+
> > atomic_long_inc(b); |
> > smp_mb__after_atomic(); |
> > WRITE_ONCE(c); |
> > {r1 = READ_ONCE(a)} <-------+
> >
> > So it's still not an acquire for READ_ONCE(a), in our case "a" is
> > ->expedited_sequence.
> >
> > To me, we can either fix the atomic_ops.rst or, as I proposed, just
> > change smp_mb__before_atomic() to smp_mb().
>
> Or have both an smp_mb__before_atomic() and an smp_mb__after_atomic(),
> as is the usual approach when you need full ordering. ;-)
>
Yes ;-) It's just that "adding a barrier after one operation to provide
acquire semantic for another operation" looks weird to me.
Regards,
Boqun
> Thanx, Paul
>
> > Thoughts?
> >
> > Regards,
> > Boqun
> >
> > > Thanx, Paul
> > >
> > > > > > what's happens there and prove that it's correct, if we use
> > > > > > store_release/load_acquire.
> > > > >
> > > > > Fair point, how about the following?
> > > > >
> > > > > Thanx, Paul
> > > > >
> > > > > ------------------------------------------------------------------------
> > > > >
> > > > > commit 6fd8074f1976596898e39f5b7ea1755652533906
> > > > > Author: Paul E. McKenney <paul...@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 <dvyu...@google.com>
> > > > > Signed-off-by: Paul E. McKenney <paul...@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(); /* ^^^ */
> > > >
> > > > If we really care about future-proofing, I think it's more safe to
> > > > change smp_mb__before_atomic() to smp_mb() rather than adding
> > > > __after_atomic() barrier. Though I think both would be unnecessary ;-)
> > > >
> > > > Regards,
> > > > Boqun
> > > >
> > > > > return true;
> > > > > }
> > > > > return false;
> > > > >
> > >
> > >
>
>
signature.asc
Description: PGP signature
