Re: Spinlocks and compiler/memory barriers - Mailing list pgsql-hackers

On 2014-06-27 13:00:34 -0400, Robert Haas wrote:
> There are two separate issues here:
> 
> 1. SpinLockAcquire and SpinLockRelease are not guaranteed to be
> compiler barriers, so all relevant memory accesses in the critical
> section need to be done using volatile pointers.  Failing to do this
> is an easy mistake to make, and we've fixed numerous bugs of this type
> over the years (most recently, to my knowledge, in
> 4bc15a8bfbc7856bc3426dc9ab99567eebbb64d3).  Forcing SpinLockAcquire()
> and SpinLockRelease() to serve as compiler barriers would let us
> dispense with a whole lot of volatile calls and make writing future
> code correctly a lot easier.

And actually faster in some cases. I'm just playing around with some
bigger POWER machine and removing the volatiles in GetSnapshotData() +
some access forcing macro for accessing xmin is worth 1% or so.

> 2. Some of our implementations of SpinLockAcquire and/or
> SpinLockRelease, but in particular SpinLockRelease, may not actually
> provide the memory-ordering semantics which they are required to
> provide.

> I tend to think that we should try to think about these two problems
> somewhat separately.  As to #1, in the back-branches, I think further
> volatile-izing the LWLock* routines is probably the only realistic
> solution.

We could also decide not to do anything :/.

>  In master, I fully support moving the goalposts such that
> we require SpinLockAcquire() and SpinLockRelease() are compiler
> barriers.  Once we do this, I think we should go back and rip out all
> the places where we've used volatile-ized pointers to provide compiler
> ordering.  That way, if we haven't actually managed to provide
> compiler ordering everywhere, it's more likely that something will
> fall over and warn us about the problem; plus, that avoids keeping
> around a coding pattern which isn't actually the one we want people to
> copy.

+1

> However, I think your proposed S_UNLOCKED_UNLOCK() hack is
> plain ugly, probably cripplingly slow, and there's no guarantee that's
> even correct; see for example the comments about Itanium's tas
> implementation possibly being only an acquire barrier (blech).

Heh. I don't think it's worse than the current fallback barrier
implementation. The S_UNLOCKED_UNLOCK() thing was just to avoid
recursion when using a barrier in the spinlock used to implement
barriers...

> On gcc and icc, which account for lines 99 through 700 of spin.h, it
> should be simple and mechanical to use compiler intrinsics to make
> sure that every S_UNLOCK implementation includes a compiler barrier.
> However, lines 710 through 895 support non-gcc, non-icc compilers, and
> some of those we may not know how to implement a compiler barrier - in
> particular, Univel CC, the Tru64 Alpha compiler, HPPA, AIX, or Sun's
> compilers.  Except for Sun, we have no buildfarm support for those
> platforms, so we could consider just dropping support entirely

Both sun's and AIX's compilers can relatively easily be handled:
* Solaris has atomic.h with membar_enter() et al. Apparently since at least
solaris 9.
* XLC has __fence and __isync intrinsics.

There's been recent talk about AIX, including about AIX animal, so I'd
be hesitant to drop it. It's also still developed.

I'm obviously in favor of dropping Alpha. And I'm, unsurprisingly, all
for removing unixware support (which is what univel CC seems to be used
for after you dropped the univel port proper).

I think the only person that has used postgres on hppa in the last 5
years is Tom, so I guess he'll have to speak up about it. Tom?

> void fake_compiler_barrier(void) { }
> void (*fake_compiler_barrier_hook) = fake_compiler_barrier;
> #define pg_compiler_barrier() ((*fake_compiler_barrier_hook)())

But we can do that as a fallback. It's what HPPA's example spinlock
implementation does after all.

> Now, this doesn't remove the circular dependency between s_lock.h and
> barrier.h, because we still don't have a fallback method, other than
> acquiring and releasing a spinlock, of implementing a barrier that
> blocks both compiler reordering and CPU reordering.  But it is enough
> to solve problem #1, and doesn't require that we drop support for
> anything that works now.

I think we can move the fallback into a C function. Compared to the cost
of a tas/unlock that shouldn't be significant.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services