Spinlocks and compiler/memory barriers

Andres Freund <andres@2ndquadrant.com> writes:

I think we should rework things so that we fall back to
pg_write_barrier(), (*((volatile slock_t *) (lock)) = 0) instead of what
we have right now.

Surely it had better be a read barrier as well? And S_LOCK the same?

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2014-06-26 14:13:07 -0700, Tom Lane wrote:

Andres Freund <andres@2ndquadrant.com> writes:

I think we should rework things so that we fall back to
pg_write_barrier(), (*((volatile slock_t *) (lock)) = 0) instead of what
we have right now.

Surely it had better be a read barrier as well?

I don't immediately see why it has to be read barrier? Hoisting a load
from after the release into the locked area of code should be safe? Note
that 'bad' reads can only happen for variables which aren't protected by
the spinlock since the S_LOCK needs to have acquire semantics and no
other process can modify protected variables concurrently.
The important thing is that all modifications that have been done inside
the spinlock are visible to other backends and that no writes are moved
outside the protected are.

And S_LOCK the same?

It better be a read barrier, yes. I haven't checked yet, but I assume
that pretty much all TAS/tas implementation already guarantee that. I
think if not we'd seen problems. Well, at least on platforms that
receive testing under concurrent circumstances :/

Greetings,

Andres Freund

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andres Freund <andres@2ndquadrant.com> writes:

On 2014-06-26 14:13:07 -0700, Tom Lane wrote:

Surely it had better be a read barrier as well?

I don't immediately see why it has to be read barrier? Hoisting a load
from after the release into the locked area of code should be safe?

No doubt, but delaying a read till after the unlocking write would
certainly not be safe.

AFAICT, README.barrier completely fails to define what we think the
semantics of pg_read_barrier and pg_write_barrier actually are, so if
you believe that a write barrier prevents reordering of reads relative to
writes, you'd better propose some new text for that file. It certainly
doesn't say that today.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

On 2014-06-26 23:01:10 +0200, Andres Freund wrote:

I think we should rework things so that we fall back to
pg_write_barrier(), (*((volatile slock_t *) (lock)) = 0) instead of what
we have right now.
That'd require to make barrier.h independent from s_lock.h, but I think
that'd be a pretty clear improvement. One open question is what happens
with the SpinlockRelease() when barriers are implemented using spinlocks
and we need a barrier for the SpinlockRelease().

Too tired to think about this any further today. Here's my current state
of this:
* barrier.h's spinlock implementation moved to s_lock.c, loosing the
s_lock.h include. That requires a S_UNLOCK definition which doesn't
again use a barrier. I've coined it S_UNLOCKED_UNLOCK
* s_lock.h now includes barrier.h to implement the generic S_UNLOCK
safely.
* gcc x86-64 had a superflous "cc" clobber. Likely copied from the 32bit
version which does additional operations.
* PPC was missing a compiler barrier
* alpha was missing a "cc" clobber.
* mips was missing a compiler barrier and a "cc" clobber
* I have no idea how to fix pa-risc's S_UNLOCK for !gcc. The referenced
spinlock paper calls a external function to avoid reordering.

Greetings,

Andres Freund

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

On 2014-06-26 15:40:11 -0700, Tom Lane wrote:

Andres Freund <andres@2ndquadrant.com> writes:

On 2014-06-26 14:13:07 -0700, Tom Lane wrote:

Surely it had better be a read barrier as well?

I don't immediately see why it has to be read barrier? Hoisting a load
from after the release into the locked area of code should be safe?

No doubt, but delaying a read till after the unlocking write would
certainly not be safe.

Right. What we actually want for locking is what several systems
(e.g. C11, linux) coin acquire/release barriers.
Not sure whether we should introduce a separate set of acquire/release
macros, or "promote" our barriers to have stronger guarantees than the
name implies.

The definition as I understand it is:

A acquire barrier implies that:
* memory operations from after the barrier cannot appear to have
happened before the barrier
* but: memory operations from *before* the barrier are *not* guaranteed to be
finished

A finished release barrier implies:
* stores from before the barrier cannot be moved past
* loads from before the barrier cannot be moved past
* but: reads from *after* the barrier might occur *before* the barrier.

I believe that all our current barrier definitions (except maybe alpha
which I haven't bothered to check thoroughly) satisfy those
constraints. That's primarily because we don't have support for all that
many platforms and use full memory barriers for read/write barriers in
several places.

Greetings,

Andres Freund

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Jun 26, 2014 at 5:01 PM, Andres Freund <andres@2ndquadrant.com> wrote:

But a) isn't really avoidable because it'll otherwise generate compiler
warnings and b) is done that way all over the tree. E.g. lwlock.c has
several copies of (note the nonvolatility of proc):
volatile LWLock *lock = l;
PGPROC *proc = MyProc;
...
proc->lwWaiting = true;
proc->lwWaitMode = LW_WAIT_UNTIL_FREE;
proc->lwWaitLink = NULL;

/* waiters are added to the front of the queue */
proc->lwWaitLink = lock->head;
if (lock->head == NULL)
lock->tail = proc;
lock->head = proc;

/* Can release the mutex now */
SpinLockRelease(&lock->mutex);
There's nothing forcing the compiler to not move any of the proc->*
assignments past the SpinLockRelease(). And indeed in my case it was
actually the store to lwWaitLink that was moved across the lock.

I think it's just pure luck that there's no active bug (that we know of)
caused by this. I wouldn't be surprised if some dubious behaviour we've
seen would be caused by similar issues.

Now, we can fix this and similar cases by more gratuitous use of
volatile. But for one we're never going to find all cases. For another
it won't help *at all* for architectures with looser CPU level memory
ordering guarantees.
I think we finally need to bite the bullet and make all S_UNLOCKs
compiler/write barriers.

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.

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. In particular, ...

I'd previously, in
/messages/by-id/20130920151110.GA8508@awork2.anarazel.de,
gone through the list of S_UNLOCKs and found several that were
lacking. Most prominently the default S_UNLOCK is just
#define S_UNLOCK(lock) (*((volatile slock_t *) (lock)) = 0)
which allows the compiler to move non volatile access across and does
nothing for CPU level cache coherency.

...this default implementation of S_UNLOCK() is pretty sketchy. Even
on a platform that enforces reads in program order and writes in
program order, this is still unsafe because a read within the critical
section might get postponed until after this write. Now, x86 happens
to have an additional constraint, which is that it can reorder loads
before stores but not stores before loads; so that coding happens to
provide release semantics. But that need not be true on every
architecture, and the trend seems to be toward weaker memory ordering.
As you pointed out to me on chat, the non-intrinsics based ARM
implementation brokenly relies on the default S_UNLOCK(), which
clearly isn't adequate.

Now, in terms of solving these problems:

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. 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. 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).

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, but
I'd be inclined to do something cheesy and hope it works:

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

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.

A more radical step would be simply desupport those architectures
instead of trying to create a fake compiler barrier for them. It
would be worth it to me to drop support for Univel CC, Alpha, and HPPA
to get this problem fixed, but we might have to do something about
AIX, and I'm sure we'd have to do something about Sun Studio.

Now, #2 is a different problem. I'm not sure there's a better option
than fixing whatever bugs exist there on a case-by-case basis.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Jun 26, 2014 at 6:40 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@2ndquadrant.com> writes:

On 2014-06-26 14:13:07 -0700, Tom Lane wrote:

Surely it had better be a read barrier as well?

I don't immediately see why it has to be read barrier? Hoisting a load
from after the release into the locked area of code should be safe?

No doubt, but delaying a read till after the unlocking write would
certainly not be safe.

AFAICT, README.barrier completely fails to define what we think the
semantics of pg_read_barrier and pg_write_barrier actually are, so if
you believe that a write barrier prevents reordering of reads relative to
writes, you'd better propose some new text for that file. It certainly
doesn't say that today.

The relevant text is in barrier.h

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2014-06-27 13:04:02 -0400, Robert Haas wrote:

On Thu, Jun 26, 2014 at 6:40 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@2ndquadrant.com> writes:

On 2014-06-26 14:13:07 -0700, Tom Lane wrote:

Surely it had better be a read barrier as well?

I don't immediately see why it has to be read barrier? Hoisting a load
from after the release into the locked area of code should be safe?

No doubt, but delaying a read till after the unlocking write would
certainly not be safe.

AFAICT, README.barrier completely fails to define what we think the
semantics of pg_read_barrier and pg_write_barrier actually are, so if
you believe that a write barrier prevents reordering of reads relative to
writes, you'd better propose some new text for that file. It certainly
doesn't say that today.

The relevant text is in barrier.h

Note that that definition of a write barrier is *not* sufficient for the
release of a lock... As I said elsewhere I think all the barrier
definitions, except maybe alpha, luckily seem to be strong enough for
that anyway.

Do we want to introduce acquire/release barriers? Or do we want to
redefine the current barriers to be strong enough for that?

Greetings,

Andres Freund

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Jun 27, 2014 at 2:04 PM, Andres Freund <andres@2ndquadrant.com> wrote:

On 2014-06-27 13:04:02 -0400, Robert Haas wrote:

On Thu, Jun 26, 2014 at 6:40 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@2ndquadrant.com> writes:

On 2014-06-26 14:13:07 -0700, Tom Lane wrote:

Surely it had better be a read barrier as well?

I don't immediately see why it has to be read barrier? Hoisting a load
from after the release into the locked area of code should be safe?

No doubt, but delaying a read till after the unlocking write would
certainly not be safe.

AFAICT, README.barrier completely fails to define what we think the
semantics of pg_read_barrier and pg_write_barrier actually are, so if
you believe that a write barrier prevents reordering of reads relative to
writes, you'd better propose some new text for that file. It certainly
doesn't say that today.

The relevant text is in barrier.h

Note that that definition of a write barrier is *not* sufficient for the
release of a lock... As I said elsewhere I think all the barrier
definitions, except maybe alpha, luckily seem to be strong enough for
that anyway.

Do we want to introduce acquire/release barriers? Or do we want to
redefine the current barriers to be strong enough for that?

Well, unless we're prepared to dump support for an awful lot of
platfomrs, trying to support acquire and release barriers on every
platform we support is a doomed effort. The definitions of the
barriers implemented by barrier.h are the same as the ones that Linux
has (minus read-barrier-depends), which I think is probably good
evidence that they are generally useful definitions. If we were going
to use any of those in s_lock.h, it'd have to be pg_memory_barrier(),
but I don't think making s_lock.h dependent on barrier.h is the way to
go. I think we should just adjust s_lock.h in a minimal way, using
inline assembler or tweaking the existing assembler or whatever.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2014-06-27 22:34:19 -0400, Robert Haas wrote:

On Fri, Jun 27, 2014 at 2:04 PM, Andres Freund <andres@2ndquadrant.com> wrote:

On 2014-06-27 13:04:02 -0400, Robert Haas wrote:

On Thu, Jun 26, 2014 at 6:40 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@2ndquadrant.com> writes:

On 2014-06-26 14:13:07 -0700, Tom Lane wrote:

Surely it had better be a read barrier as well?

I don't immediately see why it has to be read barrier? Hoisting a load
from after the release into the locked area of code should be safe?

No doubt, but delaying a read till after the unlocking write would
certainly not be safe.

AFAICT, README.barrier completely fails to define what we think the
semantics of pg_read_barrier and pg_write_barrier actually are, so if
you believe that a write barrier prevents reordering of reads relative to
writes, you'd better propose some new text for that file. It certainly
doesn't say that today.

The relevant text is in barrier.h

Note that that definition of a write barrier is *not* sufficient for the
release of a lock... As I said elsewhere I think all the barrier
definitions, except maybe alpha, luckily seem to be strong enough for
that anyway.

Do we want to introduce acquire/release barriers? Or do we want to
redefine the current barriers to be strong enough for that?

Well, unless we're prepared to dump support for an awful lot of
platfomrs, trying to support acquire and release barriers on every
platform we support is a doomed effort.

Hm? Just declare them to be as heavy as we need them? Already several
platforms fall back to more heavyweight operations than necessary?

The definitions of the
barriers implemented by barrier.h are the same as the ones that Linux
has (minus read-barrier-depends)

Linux has smb_load_acquire()/smp_store_release() for locks on all
platforms.

If we were going
to use any of those in s_lock.h, it'd have to be pg_memory_barrier(),
but I don't think making s_lock.h dependent on barrier.h is the way to
go. I think we should just adjust s_lock.h in a minimal way, using
inline assembler or tweaking the existing assembler or whatever.

Isn't that just going to be repeating the contents of barrier.h pretty
much again? How are you suggesting we deal with the generic S_UNLOCK
case without having a huge ifdef?
Why do we build an abstraction layer (barrier.h) and then not use it?

Greetings,

Andres Freund

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sat, Jun 28, 2014 at 4:31 AM, Andres Freund <andres@2ndquadrant.com> wrote:

Do we want to introduce acquire/release barriers? Or do we want to
redefine the current barriers to be strong enough for that?

Well, unless we're prepared to dump support for an awful lot of
platfomrs, trying to support acquire and release barriers on every
platform we support is a doomed effort.

Hm? Just declare them to be as heavy as we need them? Already several
platforms fall back to more heavyweight operations than necessary?

Can't we keep this simple for starters? Strength-reducing the
existing operations is yet a third problem, on top of the
already-existing problems of (1) making spinlock operations compiler
barriers and (2) fixing any buggy implementations. I'm explicitly
trying to avoid defining this in a way that means we need a Gigantic
Patch that Changes Everything.

The definitions of the
barriers implemented by barrier.h are the same as the ones that Linux
has (minus read-barrier-depends)

Linux has smb_load_acquire()/smp_store_release() for locks on all
platforms.

You mean "smp".

If we were going
to use any of those in s_lock.h, it'd have to be pg_memory_barrier(),
but I don't think making s_lock.h dependent on barrier.h is the way to
go. I think we should just adjust s_lock.h in a minimal way, using
inline assembler or tweaking the existing assembler or whatever.

Isn't that just going to be repeating the contents of barrier.h pretty
much again?

No, I think it's going to be *much* simpler than that. How about I
take a crack at this next week and then either (a) I'll see why it's a
bad idea and we can go from there or (b) you can review what I come up
with and tell me why it sucks?

How are you suggesting we deal with the generic S_UNLOCK
case without having a huge ifdef?
Why do we build an abstraction layer (barrier.h) and then not use it?

Because (1) the abstraction doesn't fit very well unless we do a lot
of additional work to build acquire and release barriers for every
platform we support and (2) I don't have much confidence that we can
depend on the spinlock fallback for barriers without completely
breaking obscure platforms, and I'd rather make a more minimal set of
changes.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2014-06-28 09:25:32 -0400, Robert Haas wrote:

On Sat, Jun 28, 2014 at 4:31 AM, Andres Freund <andres@2ndquadrant.com> wrote:

Do we want to introduce acquire/release barriers? Or do we want to
redefine the current barriers to be strong enough for that?

Well, unless we're prepared to dump support for an awful lot of
platfomrs, trying to support acquire and release barriers on every
platform we support is a doomed effort.

Hm? Just declare them to be as heavy as we need them? Already several
platforms fall back to more heavyweight operations than necessary?

Can't we keep this simple for starters? Strength-reducing the
existing operations is yet a third problem, on top of the
already-existing problems of (1) making spinlock operations compiler
barriers and (2) fixing any buggy implementations. I'm explicitly
trying to avoid defining this in a way that means we need a Gigantic
Patch that Changes Everything.

I actually mean that we can just define release barriers to be full
memory barriers for platforms where we don't want to think about it. Not
that we should weaken barriers.

No, I think it's going to be *much* simpler than that. How about I
take a crack at this next week and then either (a) I'll see why it's a
bad idea and we can go from there or (b) you can review what I come up
with and tell me why it sucks?

Ok. I think that's going in the wrong direction (duplication of
nontrivial knowledge), but maybe I'm taking a to 'purist' approach
here. Prove me wrong :)
You'll pick up the clobber changes from my patch?

How are you suggesting we deal with the generic S_UNLOCK
case without having a huge ifdef?
Why do we build an abstraction layer (barrier.h) and then not use it?

Because (1) the abstraction doesn't fit very well unless we do a lot
of additional work to build acquire and release barriers for every
platform we support and

Meh. Something like the (untested):
#if !defined(pg_release_barrier) && defined(pg_read_barrier) && defined(pg_write_barrier)
#define pg_release_barrier() do { pg_read_barrier(); pg_write_barrier();} while (0)
#elif !defined(pg_release_barrier)
#define pg_release_barrier() pg_memory_barrier()
#endif

before the fallback definition of pg_read/write_barrier should suffice?

(2) I don't have much confidence that we can
depend on the spinlock fallback for barriers without completely
breaking obscure platforms, and I'd rather make a more minimal set of
changes.

Well, it's the beginning of the cycle. And we're already depending on
barriers for correctness (and it's not getting less), so I don't really
see what avoiding barrier usage buys us except harder to find breakage?

Greetings,

Andres Freund

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2014-06-28 15:41:46 +0200, Andres Freund wrote:

On 2014-06-28 09:25:32 -0400, Robert Haas wrote:

No, I think it's going to be *much* simpler than that. How about I
take a crack at this next week and then either (a) I'll see why it's a
bad idea and we can go from there or (b) you can review what I come up
with and tell me why it sucks?

Ok. I think that's going in the wrong direction (duplication of
nontrivial knowledge), but maybe I'm taking a to 'purist' approach
here. Prove me wrong :)

What I forgot: I'm also pretty sure that the more lockless stuff we
introduce the more places are going to need acquire/release semantics...

Greetings,

Andres Freund

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sat, Jun 28, 2014 at 9:41 AM, Andres Freund <andres@2ndquadrant.com> wrote:

No, I think it's going to be *much* simpler than that. How about I
take a crack at this next week and then either (a) I'll see why it's a
bad idea and we can go from there or (b) you can review what I come up
with and tell me why it sucks?

Ok. I think that's going in the wrong direction (duplication of
nontrivial knowledge), but maybe I'm taking a to 'purist' approach
here. Prove me wrong :)

I'm not sure what you'll think of this, but see attached. I think
newer releases of Sun Studio support that GCC way of doing a barrier,
but I don't know how to test for that, so at the moment that uses the
fallback "put it in a function" approach, along with HPPA non-GCC and
Univel CC. Those things are obscure enough that I don't care about
making them less performance. I think AIX is actually OK as-is; if
_check_lock() is a compiler barrier but _clear_lock() is not, that
would be pretty odd.

How are you suggesting we deal with the generic S_UNLOCK
case without having a huge ifdef?
Why do we build an abstraction layer (barrier.h) and then not use it?

Because (1) the abstraction doesn't fit very well unless we do a lot
of additional work to build acquire and release barriers for every
platform we support and

Meh. Something like the (untested):
#if !defined(pg_release_barrier) && defined(pg_read_barrier) && defined(pg_write_barrier)
#define pg_release_barrier() do { pg_read_barrier(); pg_write_barrier();} while (0)
#elif !defined(pg_release_barrier)
#define pg_release_barrier() pg_memory_barrier()
#endif

before the fallback definition of pg_read/write_barrier should suffice?

That doesn't sound like a good idea. For example, on PPC, a read
barrier is lwsync, and so is a write barrier. That would also suck on
any architecture where either a read or write barrier gets implemented
as a full memory barrier, though I guess it might be smart enough to
optimize away most of the cost of the second of two barriers in
immediate succession.

I think if we want to use barrier.h as the foundation for this, we're
going to need to define a new set of things in there that have acquire
and release semantics, or just use full barriers for everything -
which would be wasteful on, e.g., x86. And I don't particularly see
the point in going to a lot of work to invent those new abstractions
everywhere when we can just tweak s_lock.h in place a bit and be done
with it. Making those files interdependent doesn't strike me as a
win.

(2) I don't have much confidence that we can
depend on the spinlock fallback for barriers without completely
breaking obscure platforms, and I'd rather make a more minimal set of
changes.

Well, it's the beginning of the cycle. And we're already depending on
barriers for correctness (and it's not getting less), so I don't really
see what avoiding barrier usage buys us except harder to find breakage?

If we use barriers to implement any facility other than spinlocks, I
have reasonable confidence that we won't break things more than they
already are broken today, because I think the fallback to
acquire-and-release a spinlock probably works, even though it's likely
terrible for performance. I have significantly less confidence that
trying to implement spinlocks in terms of barriers is going to be
reliable.

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

Hi,

On 2014-06-30 08:03:40 -0400, Robert Haas wrote:

On Sat, Jun 28, 2014 at 9:41 AM, Andres Freund <andres@2ndquadrant.com> wrote:

No, I think it's going to be *much* simpler than that. How about I
take a crack at this next week and then either (a) I'll see why it's a
bad idea and we can go from there or (b) you can review what I come up
with and tell me why it sucks?

Ok. I think that's going in the wrong direction (duplication of
nontrivial knowledge), but maybe I'm taking a to 'purist' approach
here. Prove me wrong :)

I'm not sure what you'll think of this, but see attached.

I think it continues in the tradition of making s_lock.h ever harder to
follow. But it's still better than what we have now from a correctness
perspective.

I think
newer releases of Sun Studio support that GCC way of doing a barrier,
but I don't know how to test for that, so at the moment that uses the
fallback "put it in a function" approach,

Sun studio's support for the gcc way is new (some update to sun studio 12), so that's
probably not sufficient.
#include <mbarrier.h> and __compiler_barrier()/__machine_rel_barrier()
should do the trick for spinlocks. That seems to have existed for
longer.

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.

along with HPPA non-GCC and
Univel CC. Those things are obscure enough that I don't care about
making them less performance.

Fine with me.

I think AIX is actually OK as-is; if _check_lock() is a compiler
barrier but _clear_lock() is not, that would be pretty odd.

Agreed.

How are you suggesting we deal with the generic S_UNLOCK
case without having a huge ifdef?
Why do we build an abstraction layer (barrier.h) and then not use it?

Because (1) the abstraction doesn't fit very well unless we do a lot
of additional work to build acquire and release barriers for every
platform we support and

Meh. Something like the (untested):
#if !defined(pg_release_barrier) && defined(pg_read_barrier) && defined(pg_write_barrier)
#define pg_release_barrier() do { pg_read_barrier(); pg_write_barrier();} while (0)
#elif !defined(pg_release_barrier)
#define pg_release_barrier() pg_memory_barrier()
#endif

before the fallback definition of pg_read/write_barrier should suffice?

That doesn't sound like a good idea. For example, on PPC, a read
barrier is lwsync, and so is a write barrier. That would also suck on
any architecture where either a read or write barrier gets implemented
as a full memory barrier, though I guess it might be smart enough to
optimize away most of the cost of the second of two barriers in
immediate succession.

Well, that's why I suggested only doing it if we haven't got a
pg_release_barrier() defined. And fallback to memory_barrier() directly
if read/write barriers are implemented using it so we don't have two
memory barriers in a row.

I think if we want to use barrier.h as the foundation for this, we're
going to need to define a new set of things in there that have acquire
and release semantics, or just use full barriers for everything -
which would be wasteful on, e.g., x86. And I don't particularly see
the point in going to a lot of work to invent those new abstractions
everywhere when we can just tweak s_lock.h in place a bit and be done
with it. Making those files interdependent doesn't strike me as a
win.

We'll need release semantics in more places than just s_lock.h. Anything
that acts like a lock without using spinlocks actually needs
acquire/release semantics. The lwlock patch only gets away with it
because the atomic operations implementation happen to act as acquire or
full memory barriers.

(2) I don't have much confidence that we can
depend on the spinlock fallback for barriers without completely
breaking obscure platforms, and I'd rather make a more minimal set of
changes.

Well, it's the beginning of the cycle. And we're already depending on
barriers for correctness (and it's not getting less), so I don't really
see what avoiding barrier usage buys us except harder to find breakage?

If we use barriers to implement any facility other than spinlocks, I
have reasonable confidence that we won't break things more than they
already are broken today, because I think the fallback to
acquire-and-release a spinlock probably works, even though it's likely
terrible for performance. I have significantly less confidence that
trying to implement spinlocks in terms of barriers is going to be
reliable.

So you believe we have a reliable barrier implementation - but you don't
believe it's reliable enough for spinlocks? If we'd *not* use the
barrier fallback for spinlock release if, and only if, it's implemented
via spinlocks, I don't see why we'd be worse off?

+#if !defined(S_UNLOCK)
+#if defined(__INTEL_COMPILER)
+#define S_UNLOCK(lock)	\
+	do { __memory_barrier(); *(lock) = 0; } while (0)
+#else

So, you're continuing to rely on the implicit acquire/release semantics
of volatiles on itanium and on the ordering guarantees for x86. Fair
enough. I checked and it seems to even be followed by gcc.

+#define S_UNLOCK(lock)	\
+	do { __asm__ __volatile__("" : : : "memory");  *(lock) = 0; } while (0)
+#endif
+#endif

This needs a comment that this implementation is only safe on strongly
ordered systems.

Causing a couple of problems:

1)
Afaics ARM will fall back to this for older gccs - and ARM is weakly
ordered. I think I'd just also use swpb to release the lock. Something
like
#define S_INIT_LOCK(lock) (*(lock)) = 0);

#define S_UNLOCK s_unlock
static __inline__ void
s_unlock(volatile slock_t *lock)
{
register slock_t _res = 0;

__asm__ __volatile__(
" swpb %0, %0, [%2] \n"
: "+r"(_res), "+m"(*lock)
: "r"(lock)
: "memory");
Assert(_res == 1); // lock should be held by us
}

2)
Afaics it's also wrong for sparc on linux (and probably the BSDs) where
relaxed memory ordering is used.

3)
Also looks wrong on mips which needs a full memory barrier.

@@ -826,6 +845,9 @@ spin_delay(void)
}
#endif

+#define S_UNLOCK(lock)	\
+	do { MemoryBarrier(); (*(lock)) = 0); } while (0)
+
#endif

Hm. Won't this end up being a full memory barrier on x86-64 even though
a compiler barrier would suffice on x86? In my measurements on linux
x86-64 that has a prety hefty performance penalty on NUMA systems.

-#define S_UNLOCK(lock)		(*((volatile slock_t *) (lock)) = 0)
+/*
+ * On most platforms, S_UNLOCK is essentially *(lock) = 0, but we can't just
+ * put that it into an inline macro, because the compiler might reorder
+ * instructions from the critical section to occur after the lock release.
+ * But since the compiler probably can't know what the external function
+ * s_unlock is doing, putting the same logic there should be adequate.
+ * Wherever possible, it's best not to rely on this default implementation,
+ * both because a sufficiently-smart globally optimizing compiler might be
+ * able to see through this charade, and perhaps more importantly because
+ * adding the cost of a function call to every spinlock release may hurt
+ * performance significantly.
+ */
+#define USE_DEFAULT_S_UNLOCK
+extern void s_unlock(volatile s_lock *lock);
+#define S_UNLOCK(lock)		s_unlock(lock)

I think this should also mention that the fallback only works for
strongly ordered systems.

Greetings,

Andres Freund

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Jun 30, 2014 at 9:26 AM, Andres Freund <andres@2ndquadrant.com> wrote:

I think it continues in the tradition of making s_lock.h ever harder to
follow. But it's still better than what we have now from a correctness
perspective.

Well, as you and I have discussed before, someday we probably need to
get rid of s_lock.h or at least refactor it heavily, but let's do one
thing at a time. I think we're eventually going to require every
platform that wants to run PostgreSQL to have working barriers and
atomics, and then we can rewrite s_lock.h into something much shorter
and cleaner, but I am opposed to doing that today, because even if we
don't care about people running obscure proprietary compilers on weird
platforms, there are still lots of people running older GCC versions.
For right now, I think the prudent thing to do is keep s_lock.h on
life support.

I think
newer releases of Sun Studio support that GCC way of doing a barrier,
but I don't know how to test for that, so at the moment that uses the
fallback "put it in a function" approach,

Sun studio's support for the gcc way is new (some update to sun studio 12), so that's
probably not sufficient.
#include <mbarrier.h> and __compiler_barrier()/__machine_rel_barrier()
should do the trick for spinlocks. That seems to have existed for
longer.

Can you either link to relevant documentation or be more specific
about what needs to be done here?

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.

So you believe we have a reliable barrier implementation - but you don't
believe it's reliable enough for spinlocks? If we'd *not* use the
barrier fallback for spinlock release if, and only if, it's implemented
via spinlocks, I don't see why we'd be worse off?

I can't parse this sentence because it's not clearly to me exactly
which part the "not" applies to, and I think we're talking past each
other a bit, too. Basically, every platform we support today has a
spinlock implementation that is supposed to prevent CPU reordering
across the acquire and release operations but might not prevent
compiler reordering. IOW, S_LOCK() should be a CPU acquire fence, and
S_UNLOCK() should be a CPU release fence. 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 is to make
NEW_S_UNLOCK(lock) = out of line function call + S_UNLOCK(lock), which
I think is strictly better. There's zip to guarantee that adding
S_LOCK(dummy) + S_UNLOCK(dummy) is doing anything we want in there,
and it's definitely going to be worse for performance.

The other thing that I don't like about your proposal has to do with
the fact that the support matrix for barrier.h and s_lock.h are not
identical. If s_lock.h is confusing to you today, making it further
intertwined with barrier.h is not going to make things better; at
least, that confuses the crap out of me. Perhaps the only good thing
to be said about this mess is that, right now, the dependency is in
just one direction: barrier.h depends on s_lock.h, and not the other
way around. At some future point we may well want to reverse the
direction of that dependency, but to do that we need barrier.h to have
a working barrier implementation for every platform that s_lock.h
supports. Maybe we'll accomplish that by adding to barrier.h and and
maybe we'll accomplish that by subtracting from s_lock.h but the short
path to getting this issue fixed is to be agnostic to that question.

1)
Afaics ARM will fall back to this for older gccs - and ARM is weakly
ordered. I think I'd just also use swpb to release the lock. Something
like
#define S_INIT_LOCK(lock) (*(lock)) = 0);

#define S_UNLOCK s_unlock
static __inline__ void
s_unlock(volatile slock_t *lock)
{
register slock_t _res = 0;

__asm__ __volatile__(
" swpb %0, %0, [%2] \n"
: "+r"(_res), "+m"(*lock)
: "r"(lock)
: "memory");
Assert(_res == 1); // lock should be held by us
}

2)
Afaics it's also wrong for sparc on linux (and probably the BSDs) where
relaxed memory ordering is used.

3)
Also looks wrong on mips which needs a full memory barrier.

You're mixing apples and oranges again. If the existing definitions
aren't CPU barriers, they're already broken, and we should fix that
and back-patch. 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.

@@ -826,6 +845,9 @@ spin_delay(void)
}
#endif

+#define S_UNLOCK(lock)       \
+     do { MemoryBarrier(); (*(lock)) = 0); } while (0)
+
#endif

Hm. Won't this end up being a full memory barrier on x86-64 even though
a compiler barrier would suffice on x86? In my measurements on linux
x86-64 that has a prety hefty performance penalty on NUMA systems.

Ah, crap, that should have been _ReadWriteBarrier().

I think this should also mention that the fallback only works for
strongly ordered systems.

I can revise the comments.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2014-06-30 10:05:44 -0400, Robert Haas wrote:

On Mon, Jun 30, 2014 at 9:26 AM, Andres Freund <andres@2ndquadrant.com> wrote:

I think it continues in the tradition of making s_lock.h ever harder to
follow. But it's still better than what we have now from a correctness
perspective.

Well, as you and I have discussed before, someday we probably need to
get rid of s_lock.h or at least refactor it heavily, but let's do one
thing at a time.

Well, that task gets harder by making it more complex...

(will answer separately about sun studio)

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.

So, I agree that we need to fix unlocks that aren't sufficiently strong
memory barriers, but it does get more urgent by not relying on volatile
inside criticial sections anymore.

Basically, every platform we support today has a
spinlock implementation that is supposed to prevent CPU reordering
across the acquire and release operations but might not prevent
compiler reordering. IOW, S_LOCK() should be a CPU acquire fence, and
S_UNLOCK() should be a CPU release fence.

Well, I think s_lock.h comes woefully short of that goal on several
platforms. Scary.

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.

To avoid issues with recursion the S_UNLOCK() used in the out of line
memory barrier implementation used a modified S_UNLOCK that doesn't
include a barrier.

My proposal is to make
NEW_S_UNLOCK(lock) = out of line function call + S_UNLOCK(lock), which
I think is strictly better. There's zip to guarantee that adding
S_LOCK(dummy) + S_UNLOCK(dummy) is doing anything we want in there,
and it's definitely going to be worse for performance.

Uh? At the very least doing a S_LOCK() guarantees that we're doing some
sort of memory barrier, which your's doesn't at all. That'd actually fix
the majority of platforms with borked release semantics because pretty
much all tas() implementations are full barriers.

The other thing that I don't like about your proposal has to do with
the fact that the support matrix for barrier.h and s_lock.h are not
identical. If s_lock.h is confusing to you today, making it further
intertwined with barrier.h is not going to make things better; at
least, that confuses the crap out of me.

Uh. It actually *removed* the confusing edge of the dependency. It's
rather confusing that memory barriers rely spinlocks and not the other
way round. I think we actually should do that unconditionally,
independent of any other changes. The only reasons barrier.h includes
s_lock.h is that dummy_spinlock is declared and that the memory barrier
is declared inline.

1)
Afaics ARM will fall back to this for older gccs - and ARM is weakly
ordered. I think I'd just also use swpb to release the lock. Something
like
#define S_INIT_LOCK(lock) (*(lock)) = 0);

#define S_UNLOCK s_unlock
static __inline__ void
s_unlock(volatile slock_t *lock)
{
register slock_t _res = 0;

__asm__ __volatile__(
" swpb %0, %0, [%2] \n"
: "+r"(_res), "+m"(*lock)
: "r"(lock)
: "memory");
Assert(_res == 1); // lock should be held by us
}

2)
Afaics it's also wrong for sparc on linux (and probably the BSDs) where
relaxed memory ordering is used.

3)
Also looks wrong on mips which needs a full memory barrier.

You're mixing apples and oranges again.

No, I'm not.

If the existing definitions
aren't CPU barriers, they're already broken, and we should fix that
and back-patch.

Yea, and that gets harder if we do it after master has changed
incompatibly. And, as explained above, we need to fix S_UNLOCK() to be a
memory barrier before we can remove volatiles - which is the goal of
your patch, no?

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.

@@ -826,6 +845,9 @@ spin_delay(void)
}
#endif

+#define S_UNLOCK(lock)       \
+     do { MemoryBarrier(); (*(lock)) = 0); } while (0)
+
#endif

Hm. Won't this end up being a full memory barrier on x86-64 even though
a compiler barrier would suffice on x86? In my measurements on linux
x86-64 that has a prety hefty performance penalty on NUMA systems.

Ah, crap, that should have been _ReadWriteBarrier().

I think that needs a
#include <intrin.h>
#pragma intrinsic(_ReadWriteBarrier)

Greetings,

Andres Freund

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Jun 30, 2014 at 11:17 AM, Andres Freund <andres@2ndquadrant.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers