Re: Spinlocks and compiler/memory barriers

On Mon, Jun 30, 2014 at 11:17 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> > Solaris seems to run with TSO enabled for SPARC (whereas linux uses RMO,
>> > relaxed memory ordering), so it's probably fine to just use the compiler
>> > barrier.
>>
>> If it isn't, that's a change that has nothing to do with making
>> spinlock operations compiler barriers and everything to do with fixing
>> a bug in the existing implementation.
>
> Well, it actually has. If we start to remove volatiles from critical
> sections the compiler will schedule stores closer to the spinlock
> release and reads more freely - making externally visible ordering
> violations more likely. Especially on itanic.

Granted.

>> Now, we want to make these
>> operations compiler fences as well, and AIUI your proposal for that is
>> to make NEW_S_UNLOCK(lock) = out of line function call + S_LOCK(dummy)
>> + S_UNLOCK(dummy) + S_UNLOCK(lock).
>
> My proposal was to use barrier.h provided barriers as long as it
> provides a native implementation. If neither a compiler nor a memory
> barrier is provided my proposal was to fall back to the memory barrier
> emulation we already have, but move it out of line (to avoid reordering
> issues). The reason for doing so is that the release *has* to be a
> release barrier.

What do you mean by "the memory barrier emulation we already have"?
The only memory barrier emulation we have, to my knowledge, is a
spinlock acquire-release cycle. But trying to use a spinlock
acquire-release to shore up problems with the spinlock release
implementation makes my head explode.

>> On the other hand, the API contract change making
>> these into compiler barriers is a master-only change. I think for
>> purposes of this patch we should assume that the existing code is
>> sufficient to prevent CPU reordering and just focus on fixing compiler
>> ordering problems. Whatever needs to change on the CPU-reordering end
>> of things is a separate commit.
>
> I'm not arguing against that it should be a separate commit. Just that
> the proper memory barrier bit has to come first.

I feel like you're speaking somewhat imprecisely in an area where very
precise speech is needed to avoid confusion. If you're saying that
you think we should fix the S_UNLOCK() implementations that fail to
prevent CPU-ordering before we change all the S_UNLOCK()
implementations to prevent compiler-reordering, then that is fine with
me; otherwise, I don't understand what you're getting at here. Do you
want to propose a patch that does *only* that first part before we go
further here? Do you want me to try to put together such a patch
based on the details mentioned on this thread?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company