sx locks and memory barriers

[Marius Nünnerich]

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?