sx locks and memory barriers

[John Baldwin]

On Tuesday 29 September 2009 4:26:56 pm Marius Nünnerich wrote:
> On Tue, Sep 29, 2009 at 21:15, Attilio Rao <attilio at freebsd.org> wrote:
> > 2009/9/29 John Baldwin <jhb at freebsd.org>:
> >> On Tuesday 29 September 2009 11:39:37 am Attilio Rao wrote:
> >>> 2009/9/25 Fabio Checconi <fabio at freebsd.org>:
> >>> > Hi all,
> >>> >  looking at sys/sx.h I have some troubles understanding this comment:
> >>> >
> >>> >  * A note about memory barriers.  Exclusive locks need to use the same
> >>> >  * memory barriers as mutexes: _acq when acquiring an exclusive lock
> >>> >  * and _rel when releasing an exclusive lock.  On the other side,
> >>> >  * shared lock needs to use an _acq barrier when acquiring the lock
> >>> >  * but, since they don't update any locked data, no memory barrier is
> >>> >  * needed when releasing a shared lock.
> >>> >
> >>> > In particular, I'm not understanding what prevents the following sequence
> >>> > from happening:
> >>> >
> >>> > CPU A                                   CPU B
> >>> >
> >>> > sx_slock(&data->lock);
> >>> >
> >>> > sx_sunlock(&data->lock);
> >>> >
> >>> > /* reordered after the unlock
> >>> >   by the cpu */
> >>> > if (data->buffer)
> >>> >                                        sx_xlock(&data->lock);
> >>> >                                        free(data->buffer);
> >>> >                                        data->buffer = NULL;
> >>> >                                        sx_xunlock(&data->lock);
> >>> >
> >>> >        a = *data->buffer;
> >>> >
> >>> > IOW, even if readers do not modify the data protected by the lock,
> >>> > without a release barrier a memory access may leak past the unlock (as
> >>> > the cpu won't notice any dependency between the unlock and the fetch,
> >>> > feeling free to reorder them), thus potentially racing with an exclusive
> >>> > writer accessing the data.
> >>> >
> >>> > On architectures where atomic ops serialize memory accesses this would
> >>> > never happen, otherwise the sequence above seems possible; am I missing
> >>> > something?
> >>>
> >>> I think your concerns are right, possibly we need this patch:
> >>> http://www.freebsd.org/~attilio/sxrw_unlockb.diff
> >>
> >> Actually, since you are only worried about reads, I think this should be
> >> an "acq" barrier rather than a "rel".  In some cases "acq" is cheaper, so we
> >> should prefer the cheapest barrier that provides what we need.  You would
> >> still need to keep some language about the memory barriers since using "acq"
> >> for shared unlocking is different from exclusive unlocking.
> >
> > Actually, I don't think that an acq barrier ensures enough protection
> > against the reordering of 'earlier' operation thus not fixing the
> > architecture ordering problem reported by Fabio. Also, I don't think
> > we just have to care about reads (or  I don't understand what you mean
> > here).
> > However, I'm not even sure that we have faster read barriers than the
> > write one. As long as it should be true in theory I don't think that's
> > what happen in practice.
> >
> >> The memory clobber is quite heavyweight.  It actually forces gcc to forget any
> >> cached memory items in registers and reload everything again.  What I really
> >> want is just a barrier to tell GCC to not reorder things.  If I read a value
> >> in the program before acquiring a lock it is in theory fine to keep that
> >> cached across the barrier.  However, there isn't a way to do this sort of
> >> thing with GCC currently.
> >
> > Yes, that's the only tool we have right now with GCC. I will try to
> > look for another way, but it sounds difficult to discover.
> 
> Even if we would have a mechanism to tell GCC to not reorder the
> instructions the CPU itself would still be free to reorder if there
> are no barriers. Or am I missing something?

No, the thing to do here for the second part is add "memory" clobbers to the
existing atomic ops with barriers.  It will still require barriers for them to
be enforced.

-- 
John Baldwin