Thread: Spinlocks and compiler/memory barriers

Hi,

I just spent 20+ hours debugging a elusive problem which only happened
under heavy concurrency. Slight changes to the code made it
disappear. In the end it turned out that gcc liked to move *one*
instruction across the SpinLockRelease() - and only sometimes. Unrelated
changes made the vanish.

The relevant code was:volatile LWLock *lock = l;
...dlist_push_head((dlist_head *) &lock->waiters, &MyProc->lwWaitLink);SpinLockRelease(&lock->mutex);

Now you could argue that it's my fault because of two things:
a) The code casts away the volatile from lock.
b) MyProc isn't volatile.

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.

I'd previously, in
http://www.postgresql.org/message-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.

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

Better ideas, other suggestions?

I'm now going to drink.

Andres

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

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

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

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

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

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
> http://www.postgresql.org/message-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

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

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

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

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

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

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

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

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

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

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

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

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

On 2014-06-30 11:38:31 -0400, Robert Haas wrote:
> On Mon, Jun 30, 2014 at 11:17 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> >> 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.

Yes.

Unrelated, but why haven't we defined it as if (TAS(dummy))
S_UNLOCK(dummy);? That's just about as strong and less of a performance
drain?
Hm, I guess platforms that do an unlocked test first would be
problematic :/

> But trying to use a spinlock
> acquire-release to shore up problems with the spinlock release
> implementation makes my head explode.

Well, it actually makes some sense. Nearly any TAS() implementation is
going to have some memory barrier semantics - so using a TAS() as
fallback makes sense. That's why we're relying on it for use in memory
barrier emulation after all.

Also note that barrier.h assumes that a S_LOCK/S_UNLOCK acts as a
compiler barrier - which really isn't guaranteed by the current contract
of s_lock.h. Although the tas() implementations look safe.

> >> 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;

Yes, that's what I think is needed.

> 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?

I'm fine with either - we're both going to flying pretty blind :/.

I think the way S_UNLOCK's release memory barrier semantics are fixed
might influence the way the compiler barrier issue can be solved. Fixing
the release semantics will involve going through all tas()
implementations and see whether the default release semantics are
ok. The ones with broken semantics will need to grow their own
S_UNLOCK. I am wondering if that commit shouldn't just remove the
default S_UNLOCK and move it to the tas() implementation sites.



Please don't forget that I started this thread because I found the
current implementation lacking because s_lock neither has sane memory
release nor compiler release semantics... So it's not surprising that I
want to solve both :)

Greetings,

Andres Freund

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

On Mon, Jun 30, 2014 at 12:20 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-06-30 11:38:31 -0400, Robert Haas wrote:
>> On Mon, Jun 30, 2014 at 11:17 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>> >> 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.
>
> Yes.
>
> Unrelated, but why haven't we defined it as if (TAS(dummy))
> S_UNLOCK(dummy);? That's just about as strong and less of a performance
> drain?
> Hm, I guess platforms that do an unlocked test first would be
> problematic :/

Yes; also, there's no requirement for S_LOCK() to be defined in terms
of TAS(), and thus no requirement for TAS() to exist at all.

>> But trying to use a spinlock
>> acquire-release to shore up problems with the spinlock release
>> implementation makes my head explode.
>
> Well, it actually makes some sense. Nearly any TAS() implementation is
> going to have some memory barrier semantics - so using a TAS() as
> fallback makes sense. That's why we're relying on it for use in memory
> barrier emulation after all.

As far as I know, all of our TAS() implementations prevent CPU
reordering in the acquire direction.  It is not clear that they
provide CPU-reordering guarantees adequate for the release direction,
even when paired with whatever S_UNLOCK() implementation they're mated
with.  And it's quite clear that many of them aren't adequate to
prevent compiler-reordering in any case.

> Also note that barrier.h assumes that a S_LOCK/S_UNLOCK acts as a
> compiler barrier - which really isn't guaranteed by the current contract
> of s_lock.h. Although the tas() implementations look safe.

Ugh, you're right.  That should really be moved out-of-line.

>> >> 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;
>
> Yes, that's what I think is needed.

OK, let's do it that way then.

>> 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?
>
> I'm fine with either - we're both going to flying pretty blind :/.
>
> I think the way S_UNLOCK's release memory barrier semantics are fixed
> might influence the way the compiler barrier issue can be solved.

I think I'm starting to understand the terminological confusion: to
me, a memory barrier means a fence against both the compiler and the
CPU.  I now think you're using it to mean a fence against the CPU, as
distinct from a fence against the compiler.  That had me really
confused in some previous replies.

> Fixing
> the release semantics will involve going through all tas()
> implementations and see whether the default release semantics are
> ok. The ones with broken semantics will need to grow their own
> S_UNLOCK. I am wondering if that commit shouldn't just remove the
> default S_UNLOCK and move it to the tas() implementation sites.

So, I think that here you are talking about CPU behavior rather than
compiler behavior.  Next paragraph is on that basis.

The implementations that don't currently have their own S_UNLOCK() are
i386, x86_64, Itanium, ARM without GCC atomics, S/390 zSeries, Sparc,
Linux m68k, VAX, m32r, SuperH, non-Linux m68k, Univel CC, HP/UX
non-GCC, Sun Studio, and WIN32_ONLY_COMPILER.  Because most of those
are older platforms, I'm betting that more of them than not are OK; I
think we should confine ourselves to trying to fix the ones we're sure
are wrong, which if I understand you correctly are ARM without GCC
atomics, Sparc, and MIPS.  I think it'd be better to just add copies
of S_UNLOCK to those three rather and NOT duplicate the definition in
the other 12 places.

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

On 2014-06-30 12:46:29 -0400, Robert Haas wrote:
> >> But trying to use a spinlock
> >> acquire-release to shore up problems with the spinlock release
> >> implementation makes my head explode.
> >
> > Well, it actually makes some sense. Nearly any TAS() implementation is
> > going to have some memory barrier semantics - so using a TAS() as
> > fallback makes sense. That's why we're relying on it for use in memory
> > barrier emulation after all.
> 
> As far as I know, all of our TAS() implementations prevent CPU
> reordering in the acquire direction.  It is not clear that they
> provide CPU-reordering guarantees adequate for the release direction,
> even when paired with whatever S_UNLOCK() implementation they're mated
> with.


> And it's quite clear that many of them aren't adequate to prevent
> compiler-reordering in any case.

I actually don't think there currently are tas() implementations that
aren't compiler barriers? Maybe UNIVEL/unixware. A bit of luck.

> > Also note that barrier.h assumes that a S_LOCK/S_UNLOCK acts as a
> > compiler barrier - which really isn't guaranteed by the current contract
> > of s_lock.h. Although the tas() implementations look safe.
> 
> Ugh, you're right.  That should really be moved out-of-line.

Good. Then we already loose the compile time interdependency from
barrier.h to s_lock.h - although the fallback will have a runtime
dependency.

> >> 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?
> >
> > I'm fine with either - we're both going to flying pretty blind :/.
> >
> > I think the way S_UNLOCK's release memory barrier semantics are fixed
> > might influence the way the compiler barrier issue can be solved.
> 
> I think I'm starting to understand the terminological confusion: to
> me, a memory barrier means a fence against both the compiler and the
> CPU.  I now think you're using it to mean a fence against the CPU, as
> distinct from a fence against the compiler.  That had me really
> confused in some previous replies.

Oh. Lets henceforth define 'fence' as the cache coherency thingy and
read/write/release/acquire/memory barrier as the combination?

> > Fixing
> > the release semantics will involve going through all tas()
> > implementations and see whether the default release semantics are
> > ok. The ones with broken semantics will need to grow their own
> > S_UNLOCK. I am wondering if that commit shouldn't just remove the
> > default S_UNLOCK and move it to the tas() implementation sites.
> 
> So, I think that here you are talking about CPU behavior rather than
> compiler behavior.  Next paragraph is on that basis.

Yes, I am.

> The implementations that don't currently have their own S_UNLOCK() are
> i386
> x86_64

TSO, thus fine.

> Itanium

As a special case volatile stores emit release/acquire fences unless
specific compiler flags are used. Thus safe.

> ARM without GCC atomics

Borked.

> S/390 zSeries

Strongly ordered.

> Sparc
> Sun Studio

Borked. At least in some setups.

> Linux m68k

At least linux doesn't support SMP for m68k, so cache coherency
shouldn't be a problem.

> VAX

I don't really know, but I don't care. The NetBSD people statements about VAX
SMP support didn't increase my concern for VAX SMP.

> m32r

No idea.

> SuperH

Not offially supported (as it's not in installation.sgml), don't care :)

> non-Linux m68k

Couldn't figure out if anybody supports SMP here. Doesn't look like it.

> Univel CC

Don't care.

> HP/UX non-GCC

Itanics volatile semantics should work.

> and WIN32_ONLY_COMPILER

Should be fine due to TSO on x86 and itanic volatiles.

> Because most of those
> are older platforms, I'm betting that more of them than not are OK; I
> think we should confine ourselves to trying to fix the ones we're sure
> are wrong

Sounds sane.

>, which if I understand you correctly are ARM without GCC
> atomics, Sparc, and MIPS.

I've to revise my statement on MIPS, it actually looks safe. I seem to
have missed that it has its own S_UNLOCK.

Do I see it correctly that !(defined(__mips__) && !defined(__sgi)) isn't
supported?

> I think it'd be better to just add copies
> of S_UNLOCK to those three rather and NOT duplicate the definition in
> the other 12 places.

Ok.

Greetings,

Andres Freund

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

On 2014-06-30 19:22:59 +0200, Andres Freund wrote:
> On 2014-06-30 12:46:29 -0400, Robert Haas wrote:
> >, which if I understand you correctly are ARM without GCC
> > atomics, Sparc, and MIPS.
>
> I've to revise my statement on MIPS, it actually looks safe. I seem to
> have missed that it has its own S_UNLOCK.

So, here's my first blind attempt at fixing these. Too tired to think
much more about it. Sparc's configurable cache coherency drives me
nuts. Linux apparently switched somewhere in 2011 from RMO (relaxed
memory order) to TSO (total store order), solaris always used TSO, but
the BSDs don't. Man.

Accordingly there's a somewhat ugly thingy attached. I don't think this
is really ready, but it's what I can come up today.

Greetings,

Andres Freund

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

On Mon, Jun 30, 2014 at 6:28 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-06-30 19:22:59 +0200, Andres Freund wrote:
>> On 2014-06-30 12:46:29 -0400, Robert Haas wrote:
>> >, which if I understand you correctly are ARM without GCC
>> > atomics, Sparc, and MIPS.
>>
>> I've to revise my statement on MIPS, it actually looks safe. I seem to
>> have missed that it has its own S_UNLOCK.
>
> So, here's my first blind attempt at fixing these. Too tired to think
> much more about it. Sparc's configurable cache coherency drives me
> nuts. Linux apparently switched somewhere in 2011 from RMO (relaxed
> memory order) to TSO (total store order), solaris always used TSO, but
> the BSDs don't. Man.
>
> Accordingly there's a somewhat ugly thingy attached. I don't think this
> is really ready, but it's what I can come up today.

You know, looking at this, I wonder if we shouldn't just remove
support for ARMv5 instead of making a blind stab at a fix.  I'm quite
in favor of doing what we can to support obscure architectures, but I
think this might be beyond what's reasonable.  First of all,
committing untested assembler code to our tree seems like an iffy idea
at best.  Secondly, even if someone is running on ARMv5, they'll be
fine as long as they're running a sufficiently new GCC, so we're not
really giving up much by dumping the hand-rolled code for that
platform.  The relevant GCC versions are several years old at this
point, and if we rely on them to get it right, that's really probably
a better bet than trying to do this blind.

Since we have a Sun Studio machine in the buildfarm, we shouldn't give
up on SPARC completely, but maybe we should only add the cases for
sparcv8+ and above?  That at least has some chance of getting tested.

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

On 2014-06-30 22:44:58 -0400, Robert Haas wrote:
> On Mon, Jun 30, 2014 at 6:28 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> > On 2014-06-30 19:22:59 +0200, Andres Freund wrote:
> >> On 2014-06-30 12:46:29 -0400, Robert Haas wrote:
> >> >, which if I understand you correctly are ARM without GCC
> >> > atomics, Sparc, and MIPS.
> >>
> >> I've to revise my statement on MIPS, it actually looks safe. I seem to
> >> have missed that it has its own S_UNLOCK.
> >
> > So, here's my first blind attempt at fixing these. Too tired to think
> > much more about it. Sparc's configurable cache coherency drives me
> > nuts. Linux apparently switched somewhere in 2011 from RMO (relaxed
> > memory order) to TSO (total store order), solaris always used TSO, but
> > the BSDs don't. Man.
> >
> > Accordingly there's a somewhat ugly thingy attached. I don't think this
> > is really ready, but it's what I can come up today.
> 
> You know, looking at this, I wonder if we shouldn't just remove
> support for ARMv5 instead of making a blind stab at a fix.

Well, I argued that way for a while ;). We don't even need to really
desupport it, but just say it's not supported for gcc < 4.4.

On the other hand, the swpb release thing isn't too complicated and just
the inverse of existing code.

> I'm quite
> in favor of doing what we can to support obscure architectures, but I
> think this might be beyond what's reasonable.

Yea, I felt like I was going mad doing it. Perhaps I should have
stopped.

> Since we have a Sun Studio machine in the buildfarm, we shouldn't give
> up on SPARC completely, but maybe we should only add the cases for
> sparcv8+ and above?  That at least has some chance of getting tested.

That we have code for sparcv7 is ridiculous btw. Sparcv8 (not 8+/9) is
from 1992. v7/sun4c been dropped from solaris 8 if I see that correctly.

I agree that v8 (in contrast to v8+ aka v9 in 32bit mode) support has to
go as well. I'd personally vote for backpatching a note to
installation.sgml saying that it's probably not working, and not do
anything else there. That means we also should replace the ldstub by cas
in the the gcc inline assembly - but we have buildfarm members for that,
so it's not too bad.

Greetings,

Andres Freund

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

On Tue, Jul 1, 2014 at 6:04 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>> You know, looking at this, I wonder if we shouldn't just remove
>> support for ARMv5 instead of making a blind stab at a fix.
>
> Well, I argued that way for a while ;). We don't even need to really
> desupport it, but just say it's not supported for gcc < 4.4.

Yeah, I didn't realize at the time that you were making that argument
that the existing code was thought to be broken on its own terms.
Removing probably-working code that we just can't test easily is, in
my mind, quite different from removing probably-broken code for which
we can't test a fix.  By the time PostgreSQL 9.5 is released, GCC 4.4
will be six years old, and telling people on an obscure platform that
few operating system distributions support that they can't use a
brand-new PostgeSQL with a seven-year-old compiler doesn't seem like a
serious problem, especially since the only alternative we can offer is
compiling against completely-untested code.

>> Since we have a Sun Studio machine in the buildfarm, we shouldn't give
>> up on SPARC completely, but maybe we should only add the cases for
>> sparcv8+ and above?  That at least has some chance of getting tested.
>
> That we have code for sparcv7 is ridiculous btw. Sparcv8 (not 8+/9) is
> from 1992. v7/sun4c been dropped from solaris 8 if I see that correctly.
>
> I agree that v8 (in contrast to v8+ aka v9 in 32bit mode) support has to
> go as well. I'd personally vote for backpatching a note to
> installation.sgml saying that it's probably not working, and not do
> anything else there. That means we also should replace the ldstub by cas
> in the the gcc inline assembly - but we have buildfarm members for that,
> so it's not too bad.

If you want to do that, it's fine with me.  What I would do is:

- Back-patch the addition of the sparcv8+ stuff all the way.  If
anyone's running anything older, let them complain...
- Remove the special case for MIPS without gcc intrinsics only in
master, leaving the back-branches broken.  If anyone cares, let them
complain...
- Nothing else.

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

On Tue, Jul 1, 2014 at 11:22 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Jul 1, 2014 at 6:04 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>>> You know, looking at this, I wonder if we shouldn't just remove
>>> support for ARMv5 instead of making a blind stab at a fix.
>>
>> Well, I argued that way for a while ;). We don't even need to really
>> desupport it, but just say it's not supported for gcc < 4.4.
>
> Yeah, I didn't realize at the time that you were making that argument
> that the existing code was thought to be broken on its own terms.
> Removing probably-working code that we just can't test easily is, in
> my mind, quite different from removing probably-broken code for which
> we can't test a fix.  By the time PostgreSQL 9.5 is released, GCC 4.4
> will be six years old, and telling people on an obscure platform that
> few operating system distributions support that they can't use a
> brand-new PostgeSQL with a seven-year-old compiler doesn't seem like a
> serious problem, especially since the only alternative we can offer is
> compiling against completely-untested code.

A few years back I ported the postresql client libraries and a few
other pieces of software (in particular subversion) to a lot of
obscure platforms (old sparc, hpux, irix, older aix, etc etc).
Getting a modern gcc working on those platforms (with the possible
exception of aix) is in many cases difficult or impossible.   So
requiring new gcc is exactly equivalent to desupporting.

merlin

On 2014-07-01 11:46:19 -0500, Merlin Moncure wrote:
> On Tue, Jul 1, 2014 at 11:22 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> > On Tue, Jul 1, 2014 at 6:04 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> >>> You know, looking at this, I wonder if we shouldn't just remove
> >>> support for ARMv5 instead of making a blind stab at a fix.
> >>
> >> Well, I argued that way for a while ;). We don't even need to really
> >> desupport it, but just say it's not supported for gcc < 4.4.
> >
> > Yeah, I didn't realize at the time that you were making that argument
> > that the existing code was thought to be broken on its own terms.
> > Removing probably-working code that we just can't test easily is, in
> > my mind, quite different from removing probably-broken code for which
> > we can't test a fix.  By the time PostgreSQL 9.5 is released, GCC 4.4
> > will be six years old, and telling people on an obscure platform that
> > few operating system distributions support that they can't use a
> > brand-new PostgeSQL with a seven-year-old compiler doesn't seem like a
> > serious problem, especially since the only alternative we can offer is
> > compiling against completely-untested code.
> 
> A few years back I ported the postresql client libraries and a few
> other pieces of software (in particular subversion) to a lot of
> obscure platforms (old sparc, hpux, irix, older aix, etc etc).
> Getting a modern gcc working on those platforms (with the possible
> exception of aix) is in many cases difficult or impossible.

Well, we're talking about a case where the current code is
*broken*. Subtly so. Where we have no way of testing potential fixes. If
you/somebody steps up to fix & test that, cool. But I don't see it as a
service to anyone to ship broken stuff.

> So requiring new gcc is exactly equivalent to desupporting.

Yea, right. The case we're talking about here is armv5 and some armv6's
(the current fallback inline assembly doesn't work on all v6s). If
postgres were indeed in use and updated on such a platform it'd be cross
compiled with near certainty.

The other case is sparcv7 and sparcv8 (not v8+). The former has been
dropped from solaris8, the latter from solaris9.

Greetings,

Andres Freund

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

On Tue, Jul 1, 2014 at 12:46 PM, Merlin Moncure <mmoncure@gmail.com> wrote:
> A few years back I ported the postresql client libraries and a few
> other pieces of software (in particular subversion) to a lot of
> obscure platforms (old sparc, hpux, irix, older aix, etc etc).
> Getting a modern gcc working on those platforms (with the possible
> exception of aix) is in many cases difficult or impossible.   So
> requiring new gcc is exactly equivalent to desupporting.

I took a look around to see which operating systems support which
hardware platforms.  It's a little hard to tell who is supporting
which operating systems, because the terminology is different on
different project web sites, and it's hard to tell which things are
actually ARMv5, as opposed to something else.  But it seems that
NetBSD is has by far the largest list of supported platforms, and it
appears that they compile their current releases with either gcc 4.5
or gcc 4.8, depending on the particular port:

http://www.netbsd.org/developers/features/

According to http://www.netbsd.org/ports/ the ports that run on some
form of ARM are acorn26, acorn32, cats, epoc32, evbarm, hpcarm,
iyonix, netwinder, shark, and zaurus.  epoc32 is not listed at all on
the features link above, but the others are all listed as using gcc
4.8.x.  Even if they were using gcc 4.5.x, though, that would be good
enough, and most platforms that are now on 4.8.x were on 4.5.x before
the 4.8.x import got done:

http://mail-index.netbsd.org/tech-userlevel/2014/02/18/msg008484.html

Apparently, the last holdout preventing removal of gcc 4.1.x from
NetBSD was the vax port, which has happily now been upgraded to 4.8.x.

Debian also supports ARMv5; actually, they support ARMv4t and higher:

https://wiki.debian.org/ArmEabiPort

Debian has shipped a sufficiently-new compiler for our purposes since
6.0 (squeeze), released in 2009:

https://packages.debian.org/squeeze/gcc

So it's clearly *possible* to get newer gcc versions running on ARMv5.
I will grant you that it may not be the easiest thing to do on an
existing installation.  But I don't think having us continue to ship
either known-broken code or completely-untested code is any better.
If someone needs to get PostgreSQL 9.5 running on ARMv5 using an older
gcc, they can test Andres's already-posted patch and, if it works, we
can commit it.  What I think doesn't make sense is to ship it untested
and claim we have support when that really might or might not be true.

Also, none of this would affect the PostgreSQL client libraries that
you are talking about.  s_lock.h is only used by the backend.

The bottom line is that I love supporting obscure platforms as much as
anyone here, and several other committers are already telling me that
I love it too much.  We've got to draw the line somewhere, and I think
refusing to ship newly-written code that we have exactly zero means of
testing is a pretty good place to draw it.

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

Robert Haas <robertmhaas@gmail.com> writes:
> The bottom line is that I love supporting obscure platforms as much as
> anyone here, and several other committers are already telling me that
> I love it too much.  We've got to draw the line somewhere, and I think
> refusing to ship newly-written code that we have exactly zero means of
> testing is a pretty good place to draw it.

I'm good with the concept of expecting anyone who complains about lack
of support for $platform to provide resources for testing/debugging PG
on that platform; and that pending arrival of such resources it's okay
to consider the platform desupported.  What concerns me here is what
level of support we could provide even with adequate resources.  That
is, are we going to be buying into a scenario where platforms with poor
atomics support take a significant performance hit compared to the
current state of affairs?  I don't think that would be good.

Another way of framing the problem is in response to Andres' upthread
comment that relying on emulated atomics makes things much easier to
reason about.  That may be true as far as correctness is concerned but
it's patently false for reasoning about performance.  This ties into
Robert's worry about how many different hardware performance profiles
we're going to have to concern ourselves with.

Basically the future that concerns me is that we perform well on x86_64
hardware (which I believe is pretty much all that any active developers
are using) and poorly on other hardware.  I don't want to end up there,
but I think the current direction of this patch pretty much guarantees
that outcome.
        regards, tom lane

On Tue, Jul 1, 2014 at 2:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> The bottom line is that I love supporting obscure platforms as much as
>> anyone here, and several other committers are already telling me that
>> I love it too much.  We've got to draw the line somewhere, and I think
>> refusing to ship newly-written code that we have exactly zero means of
>> testing is a pretty good place to draw it.
>
> I'm good with the concept of expecting anyone who complains about lack
> of support for $platform to provide resources for testing/debugging PG
> on that platform; and that pending arrival of such resources it's okay
> to consider the platform desupported.  What concerns me here is what
> level of support we could provide even with adequate resources.  That
> is, are we going to be buying into a scenario where platforms with poor
> atomics support take a significant performance hit compared to the
> current state of affairs?  I don't think that would be good.
>
> Another way of framing the problem is in response to Andres' upthread
> comment that relying on emulated atomics makes things much easier to
> reason about.  That may be true as far as correctness is concerned but
> it's patently false for reasoning about performance.  This ties into
> Robert's worry about how many different hardware performance profiles
> we're going to have to concern ourselves with.

I have to admit that my concerns in that area were ameliorated to a
significant degree by the performance results that Andres posted
yesterday.  On top of the atomics patch (which is not this thread),
he's got a rewritten lwlock patch that uses those atomics.  His
testing showed that, even when that patch uses the atomics emulation
layer instead of real atomics, it's still better than the status quo.
Maybe that won't be true for every patch that uses atomics, but it's
certainly good as far as it goes.

But I think the issue on this sub-thread is a different one
altogether: in studying s_lock.h, Andres found clear evidence that the
ARMv5 and Sun Studio spinlock implementations are in fact *buggy*.
Even if the programmer uses volatile pointers until they turn blue in
the face, nothing in that patch prevents the CPU from changing the
apparent order of execution such that instructions within the
spinlock-protected critical section appear to execute after lock
release.  That is no good.  It means that if somebody runs PostgreSQL
on those platforms and tries to do non-trivial things with it, it's
probably going to break.

Now, if we want, we can simply ignore that problem.  No actual users
have complained about it, and it's not entirely outside the bounds of
plausibility that Andres is wrong, and those implementations are not
buggy after all.  But I don't think he's wrong.  If we assume he's
right, then we've got two choices: we can either blindly install fixes
for those platforms that we can't test, or we can drop support.

> Basically the future that concerns me is that we perform well on x86_64
> hardware (which I believe is pretty much all that any active developers
> are using) and poorly on other hardware.  I don't want to end up there,
> but I think the current direction of this patch pretty much guarantees
> that outcome.

I think this concern is more germane to the atomics patch than what
we're talking about here, because what we're talking about here, at
the moment, is either attempting to repair, or just rip out
completely, S_UNLOCK() implementations that appear to be buggy.

I have access to several PPC64 systems, use them regularly for
benchmarking and performance-testing, and can provide access to one of
those to others who may need it for testing.  For that reason, I think
at least x64/amd64 and PPC64 will get tested regularly, at least as
long as IBM keeps that hardware on loan to us.  I previously had
access to an Itanium server and used that for a lot of testing during
the 9.2 cycle, but that eventually went away.  I think it would be
awfully nice if some hardware vendor (or interested community member)
could provide us with access to some top-quality SPARC, ARM, and maybe
zSeries machines for testing, but nobody's offered that I know of.  We
can't test hardware we don't have.

Despite my concerns about keeping the list of supported atomics short,
and I do have concerns in that area, I'm not really sure that we have
much choice but to go in that direction.  We can't accept a >5x
performance hit in the name of portability, and that's literally what
we're talking about in some cases.  I definitely want to think
carefully about how we proceed in this area but doing nothing doesn't
seem like an option.

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

Robert Haas <robertmhaas@gmail.com> writes:
> Despite my concerns about keeping the list of supported atomics short,
> and I do have concerns in that area, I'm not really sure that we have
> much choice but to go in that direction.  We can't accept a >5x
> performance hit in the name of portability, and that's literally what
> we're talking about in some cases.  I definitely want to think
> carefully about how we proceed in this area but doing nothing doesn't
> seem like an option.

To be clear, I'm not advocating doing nothing (and I don't think anyone
else is).  It's obvious based on Andres' results that we want to use
atomics on platforms where they're well-supported.  The argument is
around what we're going to do for other platforms.
        regards, tom lane

On Tue, Jul 1, 2014 at 4:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> Despite my concerns about keeping the list of supported atomics short,
>> and I do have concerns in that area, I'm not really sure that we have
>> much choice but to go in that direction.  We can't accept a >5x
>> performance hit in the name of portability, and that's literally what
>> we're talking about in some cases.  I definitely want to think
>> carefully about how we proceed in this area but doing nothing doesn't
>> seem like an option.
>
> To be clear, I'm not advocating doing nothing (and I don't think anyone
> else is).  It's obvious based on Andres' results that we want to use
> atomics on platforms where they're well-supported.  The argument is
> around what we're going to do for other platforms.

OK, but that still seems like the issue on the other thread, not
what's being discussed here.

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

On 01/07/14 11:04, Andres Freund wrote:

>> Since we have a Sun Studio machine in the buildfarm, we shouldn't give
>> up on SPARC completely, but maybe we should only add the cases for
>> sparcv8+ and above?  That at least has some chance of getting tested.
>
> That we have code for sparcv7 is ridiculous btw. Sparcv8 (not 8+/9) is
> from 1992. v7/sun4c been dropped from solaris 8 if I see that correctly.
>
> I agree that v8 (in contrast to v8+ aka v9 in 32bit mode) support has to
> go as well. I'd personally vote for backpatching a note to
> installation.sgml saying that it's probably not working, and not do
> anything else there. That means we also should replace the ldstub by cas
> in the the gcc inline assembly - but we have buildfarm members for that,
> so it's not too bad.

Being involved in QEMU SPARC development, I can tell you that patches 
are still actively being received for SPARCv8. The last set of CPU 
patches were related to fixing bugs in the LEON3, a 32-bit SPARC CPU 
which is available in hardened versions certified for extreme 
environments such as military and space. I'd hate to find out that they 
switched to another database because they couldn't upgrade PostgreSQL on 
the ISS ;)

Also if you're struggling for Sun buildfarm animals, recent versions of 
QEMU will quite happily install and run later versions of 32-bit Solaris 
over serial, and 2.0 even manages to give you a cgthree framebuffer for 
the full experience.


ATB,

Mark.

On 2014-07-01 23:21:07 +0100, Mark Cave-Ayland wrote:
> On 01/07/14 11:04, Andres Freund wrote:
> 
> >>Since we have a Sun Studio machine in the buildfarm, we shouldn't give
> >>up on SPARC completely, but maybe we should only add the cases for
> >>sparcv8+ and above?  That at least has some chance of getting tested.
> >
> >That we have code for sparcv7 is ridiculous btw. Sparcv8 (not 8+/9) is
> >from 1992. v7/sun4c been dropped from solaris 8 if I see that correctly.
> >
> >I agree that v8 (in contrast to v8+ aka v9 in 32bit mode) support has to
> >go as well. I'd personally vote for backpatching a note to
> >installation.sgml saying that it's probably not working, and not do
> >anything else there. That means we also should replace the ldstub by cas
> >in the the gcc inline assembly - but we have buildfarm members for that,
> >so it's not too bad.
> 
> Being involved in QEMU SPARC development, I can tell you that patches are
> still actively being received for SPARCv8. The last set of CPU patches were
> related to fixing bugs in the LEON3, a 32-bit SPARC CPU which is available
> in hardened versions certified for extreme environments such as military and
> space. I'd hate to find out that they switched to another database because
> they couldn't upgrade PostgreSQL on the ISS ;)

LEON3 isn't really sparcv8. It's more like sparcv8 with v9 stuff cherry
picked... It e.g. has CAS :)
Your point stands though - we should probably backpatch the v8 version
of my patch as well, since apparently LEON3 does *not* have membar.

But I don't think this is a rat race we can win. We can't keep up with
all the variants of sparc and even worse arm. We should add a intrinsics
based version for sparc as well. That'll work just fine on any recent
gcc.

> Also if you're struggling for Sun buildfarm animals, recent versions of QEMU
> will quite happily install and run later versions of 32-bit Solaris over
> serial, and 2.0 even manages to give you a cgthree framebuffer for the full
> experience.

Well. I have to admit I'm really not interested in investing that much
time in something I've no stake in. If postgres developers have to put
emulated machines to develop features something imo went seriously
wrong. That's more effort than at least I'm willing to spend.

Greetings,

Andres Freund

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

Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-07-01 23:21:07 +0100, Mark Cave-Ayland wrote:
>> Also if you're struggling for Sun buildfarm animals, recent versions of QEMU
>> will quite happily install and run later versions of 32-bit Solaris over
>> serial, and 2.0 even manages to give you a cgthree framebuffer for the full
>> experience.

> Well. I have to admit I'm really not interested in investing that much
> time in something I've no stake in. If postgres developers have to put
> emulated machines to develop features something imo went seriously
> wrong. That's more effort than at least I'm willing to spend.

Perhaps more to the point, I have no faith at all that an emulator will
mimic multiprocessor timing behavior to the level of detail needed to
tell whether memory-barrier-related logic works.  See the VAX discussion
just a couple days ago.
        regards, tom lane

On 2014-07-01 20:21:37 -0400, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > On 2014-07-01 23:21:07 +0100, Mark Cave-Ayland wrote:
> >> Also if you're struggling for Sun buildfarm animals, recent versions of QEMU
> >> will quite happily install and run later versions of 32-bit Solaris over
> >> serial, and 2.0 even manages to give you a cgthree framebuffer for the full
> >> experience.
> 
> > Well. I have to admit I'm really not interested in investing that much
> > time in something I've no stake in. If postgres developers have to put
> > emulated machines to develop features something imo went seriously
> > wrong. That's more effort than at least I'm willing to spend.
> 
> Perhaps more to the point, I have no faith at all that an emulator will
> mimic multiprocessor timing behavior to the level of detail needed to
> tell whether memory-barrier-related logic works.  See the VAX discussion
> just a couple days ago.

Well, it would allow us to see wether fixed stuff actually compiles and
runs - that's not nothing. The biggest problem with fixing stuff like
armv5, sparc8, whatever is that it requires adding stuff to our inline
assembly. It's easy to accidentally make it not compile, but
comparatively harder to make the behaviour even worse than before.

Greetings,

Andres Freund

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

On 02/07/14 08:36, Andres Freund wrote:

> On 2014-07-01 20:21:37 -0400, Tom Lane wrote:
>> Andres Freund <andres@2ndquadrant.com> writes:
>>> On 2014-07-01 23:21:07 +0100, Mark Cave-Ayland wrote:
>>>> Also if you're struggling for Sun buildfarm animals, recent versions of QEMU
>>>> will quite happily install and run later versions of 32-bit Solaris over
>>>> serial, and 2.0 even manages to give you a cgthree framebuffer for the full
>>>> experience.
>>
>>> Well. I have to admit I'm really not interested in investing that much
>>> time in something I've no stake in. If postgres developers have to put
>>> emulated machines to develop features something imo went seriously
>>> wrong. That's more effort than at least I'm willing to spend.
>>
>> Perhaps more to the point, I have no faith at all that an emulator will
>> mimic multiprocessor timing behavior to the level of detail needed to
>> tell whether memory-barrier-related logic works.  See the VAX discussion
>> just a couple days ago.
>
> Well, it would allow us to see wether fixed stuff actually compiles and
> runs - that's not nothing. The biggest problem with fixing stuff like
> armv5, sparc8, whatever is that it requires adding stuff to our inline
> assembly. It's easy to accidentally make it not compile, but
> comparatively harder to make the behaviour even worse than before.

The point I wanted to make was that there are certain applications for 
which SPARCv8 is still certified for particular military/aerospace use. 
While I don't use it myself, some people are still using it enough to 
want to contribute QEMU patches.

In terms of QEMU, the main reason for mentioning it was that if the 
consensus were to keep SPARCv8 support then this could be one possible 
way to provide basic buildfarm support (although as Tom rightly points 
out, there would need to be testing to build up confidence that the 
emulator does the right thing in a multiprocessor environment).


ATB,

Mark.

On Wed, Jul 2, 2014 at 4:06 AM, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> The point I wanted to make was that there are certain applications for which
> SPARCv8 is still certified for particular military/aerospace use. While I
> don't use it myself, some people are still using it enough to want to
> contribute QEMU patches.
>
> In terms of QEMU, the main reason for mentioning it was that if the
> consensus were to keep SPARCv8 support then this could be one possible way
> to provide basic buildfarm support (although as Tom rightly points out,
> there would need to be testing to build up confidence that the emulator does
> the right thing in a multiprocessor environment).

I think we're getting off-track here.  The PostgreSQL project is
always willing to accept new buildfarm machines.  In the absence of
buildfarm machines, we try not to go around randomly breaking things
that were previously working.  But the current situation is that we
have good reason to suspect that a couple of very old platforms are
subtly broken.  In that situation, I think removing the
thought-to-be-broken-code is the only sensible approach.

Now, we're not crossing any sort of Rubicon here.  If buildfarm
machines show up - or, hell, if ONE person with access to the hardware
OR a simulator shows up and can compile test EVEN ONCE that a patch to
re-add support compiles and that the regression tests pass - then we
can add support back in two shakes of a lamb's tail.  In the meantime,
leaving broken code in our tree is not a service to anyone.

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

On Tue, Jul 1, 2014 at 12:22 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> Since we have a Sun Studio machine in the buildfarm, we shouldn't give
>>> up on SPARC completely, but maybe we should only add the cases for
>>> sparcv8+ and above?  That at least has some chance of getting tested.
>>
>> That we have code for sparcv7 is ridiculous btw. Sparcv8 (not 8+/9) is
>> from 1992. v7/sun4c been dropped from solaris 8 if I see that correctly.
>>
>> I agree that v8 (in contrast to v8+ aka v9 in 32bit mode) support has to
>> go as well. I'd personally vote for backpatching a note to
>> installation.sgml saying that it's probably not working, and not do
>> anything else there. That means we also should replace the ldstub by cas
>> in the the gcc inline assembly - but we have buildfarm members for that,
>> so it's not too bad.
>
> If you want to do that, it's fine with me.  What I would do is:
>
> - Back-patch the addition of the sparcv8+ stuff all the way.  If
> anyone's running anything older, let them complain...
> - Remove the special case for MIPS without gcc intrinsics only in
> master, leaving the back-branches broken.  If anyone cares, let them
> complain...
> - Nothing else.

I've gone ahead and done the second of these things.

Andres, do you want to go take a stab at fixing the SPARC stuff?

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

On 2014-07-06 15:02:21 -0400, Robert Haas wrote:
> On Tue, Jul 1, 2014 at 12:22 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> >>> Since we have a Sun Studio machine in the buildfarm, we shouldn't give
> >>> up on SPARC completely, but maybe we should only add the cases for
> >>> sparcv8+ and above?  That at least has some chance of getting tested.
> >>
> >> That we have code for sparcv7 is ridiculous btw. Sparcv8 (not 8+/9) is
> >> from 1992. v7/sun4c been dropped from solaris 8 if I see that correctly.
> >>
> >> I agree that v8 (in contrast to v8+ aka v9 in 32bit mode) support has to
> >> go as well. I'd personally vote for backpatching a note to
> >> installation.sgml saying that it's probably not working, and not do
> >> anything else there. That means we also should replace the ldstub by cas
> >> in the the gcc inline assembly - but we have buildfarm members for that,
> >> so it's not too bad.
> >
> > If you want to do that, it's fine with me.  What I would do is:
> >
> > - Back-patch the addition of the sparcv8+ stuff all the way.  If
> > anyone's running anything older, let them complain...
> > - Remove the special case for MIPS without gcc intrinsics only in
> > master, leaving the back-branches broken.  If anyone cares, let them
> > complain...
> > - Nothing else.
> 
> I've gone ahead and done the second of these things.

Thanks.

> Andres, do you want to go take a stab at fixing the SPARC stuff?

Will do, will probably take me till thursday to come up with the brain
cycles.

Greetings,

Andres Freund

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

On Sun, Jul 6, 2014 at 3:12 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> > If you want to do that, it's fine with me.  What I would do is:
>> >
>> > - Back-patch the addition of the sparcv8+ stuff all the way.  If
>> > anyone's running anything older, let them complain...
>> > - Remove the special case for MIPS without gcc intrinsics only in
>> > master, leaving the back-branches broken.  If anyone cares, let them
>> > complain...
>> > - Nothing else.
>>
>> I've gone ahead and done the second of these things.
>
> Thanks.
>
>> Andres, do you want to go take a stab at fixing the SPARC stuff?
>
> Will do, will probably take me till thursday to come up with the brain
> cycles.

Ping?

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

On Tue, Aug 5, 2014 at 11:55 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Sun, Jul 6, 2014 at 3:12 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>>> > If you want to do that, it's fine with me.  What I would do is:
>>> >
>>> > - Back-patch the addition of the sparcv8+ stuff all the way.  If
>>> > anyone's running anything older, let them complain...
>>> > - Remove the special case for MIPS without gcc intrinsics only in
>>> > master, leaving the back-branches broken.  If anyone cares, let them
>>> > complain...
>>> > - Nothing else.
>>>
>>> I've gone ahead and done the second of these things.
>>
>> Thanks.
>>
>>> Andres, do you want to go take a stab at fixing the SPARC stuff?
>>
>> Will do, will probably take me till thursday to come up with the brain
>> cycles.
>
> Ping?

This has been pending for almost two months now and, at your request,
my patch to make spinlocks act as compiler barriers is waiting behind
it.  Can we please get this moving again soon, or can I commit that
patch and you can fix this when you get around to it?

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

On September 4, 2014 2:18:37 PM CEST, Robert Haas <robertmhaas@gmail.com> wrote:
>On Tue, Aug 5, 2014 at 11:55 AM, Robert Haas <robertmhaas@gmail.com>
>wrote:
>> On Sun, Jul 6, 2014 at 3:12 PM, Andres Freund
><andres@2ndquadrant.com> wrote:
>>>> > If you want to do that, it's fine with me.  What I would do is:
>>>> >
>>>> > - Back-patch the addition of the sparcv8+ stuff all the way.  If
>>>> > anyone's running anything older, let them complain...
>>>> > - Remove the special case for MIPS without gcc intrinsics only in
>>>> > master, leaving the back-branches broken.  If anyone cares, let
>them
>>>> > complain...
>>>> > - Nothing else.
>>>>
>>>> I've gone ahead and done the second of these things.
>>>
>>> Thanks.
>>>
>>>> Andres, do you want to go take a stab at fixing the SPARC stuff?
>>>
>>> Will do, will probably take me till thursday to come up with the
>brain
>>> cycles.
>>
>> Ping?
>
>This has been pending for almost two months now and, at your request,
>my patch to make spinlocks act as compiler barriers is waiting behind
>it.  Can we please get this moving again soon, or can I commit that
>patch and you can fix this when you get around to it?

Yes. I plan to push the patch this weekend. Sorry for the delay.

Andres

-- 
Please excuse brevity and formatting - I am writing this on my mobile phone.

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

On 2014-09-04 14:19:47 +0200, Andres Freund wrote:
> Yes. I plan to push the patch this weekend. Sorry for the delay.

I'm about to push this. Is it ok to first push it to master and only
backpatch once a couple buildfarm cycles haven't complained?

Greetings,

Andres Freund

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

On Mon, Sep 8, 2014 at 8:07 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-09-04 14:19:47 +0200, Andres Freund wrote:
>> Yes. I plan to push the patch this weekend. Sorry for the delay.
>
> I'm about to push this. Is it ok to first push it to master and only
> backpatch once a couple buildfarm cycles haven't complained?

That will have the disadvantage that src/tools/git_changelog will show
the commits separately instead of grouping them together; so it's
probably best not to make a practice of it.  But I think it's up to
your discretion how to handle it in any particular case.

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

Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-09-04 14:19:47 +0200, Andres Freund wrote:
>> Yes. I plan to push the patch this weekend. Sorry for the delay.

> I'm about to push this. Is it ok to first push it to master and only
> backpatch once a couple buildfarm cycles haven't complained?

It makes for a cleaner commit history if you push concurrently into
all the branches you intend to patch.  That also gives more buildfarm
runs, which seems like a good thing for this sort of patch.

That is, assuming that we ought to backpatch at all, which to my mind
is debatable.
        regards, tom lane

On Mon, Sep 8, 2014 at 10:07 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> It makes for a cleaner commit history if you push concurrently into
> all the branches you intend to patch.  That also gives more buildfarm
> runs, which seems like a good thing for this sort of patch.
>
> That is, assuming that we ought to backpatch at all, which to my mind
> is debatable.

We're not going to backpatch the main patch to make spinlock
primitives act as compiler barriers - or at least, I will object
loudly.

But what we're talking about here is a bug fix for Sparc.  And surely
we ought to back-patch that.

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

On Mon, Sep  8, 2014 at 10:08:04AM -0400, Robert Haas wrote:
> On Mon, Sep 8, 2014 at 8:07 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> > On 2014-09-04 14:19:47 +0200, Andres Freund wrote:
> >> Yes. I plan to push the patch this weekend. Sorry for the delay.
> >
> > I'm about to push this. Is it ok to first push it to master and only
> > backpatch once a couple buildfarm cycles haven't complained?
> 
> That will have the disadvantage that src/tools/git_changelog will show
> the commits separately instead of grouping them together; so it's
> probably best not to make a practice of it.  But I think it's up to
> your discretion how to handle it in any particular case.

Uh, git_changelog timespan check is 24 hours, so if the delay is less
then 24 hours, I think we are ok, e.g.:
# Might want to make this parameter user-settable.my $timestamp_slop = 24 * 60 * 60;


--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

Robert Haas <robertmhaas@gmail.com> writes:
> But what we're talking about here is a bug fix for Sparc.  And surely
> we ought to back-patch that.

Ah.  OK, no objection.
        regards, tom lane

On 2014-09-04 08:18:37 -0400, Robert Haas wrote:
> On Tue, Aug 5, 2014 at 11:55 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> > On Sun, Jul 6, 2014 at 3:12 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> >>> > If you want to do that, it's fine with me.  What I would do is:
> >>> >
> >>> > - Back-patch the addition of the sparcv8+ stuff all the way.  If
> >>> > anyone's running anything older, let them complain...
> >>> > - Remove the special case for MIPS without gcc intrinsics only in
> >>> > master, leaving the back-branches broken.  If anyone cares, let them
> >>> > complain...
> >>> > - Nothing else.
> >>>
> >>> I've gone ahead and done the second of these things.
> >>
> >> Thanks.
> >>
> >>> Andres, do you want to go take a stab at fixing the SPARC stuff?
> >>
> >> Will do, will probably take me till thursday to come up with the brain
> >> cycles.
> >
> > Ping?
> 
> This has been pending for almost two months now and, at your request,
> my patch to make spinlocks act as compiler barriers is waiting behind
> it.  Can we please get this moving again soon, or can I commit that
> patch and you can fix this when you get around to it?

I finally pushed this. And once more I seriously got pissed at the poor
overall worldwide state of documentation and continously changing
terminology around this.

Sorry for taking this long :(

Do you have a current version of your patch to make them compiler
barriers?

Andres Freund

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

On Mon, Sep 8, 2014 at 7:10 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> This has been pending for almost two months now and, at your request,
>> my patch to make spinlocks act as compiler barriers is waiting behind
>> it.  Can we please get this moving again soon, or can I commit that
>> patch and you can fix this when you get around to it?
>
> I finally pushed this. And once more I seriously got pissed at the poor
> overall worldwide state of documentation and continously changing
> terminology around this.

There does seem to be a deficit in that area.

> Sorry for taking this long :(
>
> Do you have a current version of your patch to make them compiler
> barriers?

I had forgotten that it needed an update.  Thanks for the reminder.  Here's v2.

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

On 2014-09-09 13:52:40 -0400, Robert Haas wrote:
> I had forgotten that it needed an update.  Thanks for the reminder.  Here's v2.

I've attached a incremental patch fixing minor gripes. Other than that I
think you can go ahead with this once the buildfarm accepts the sparc
fixes (man, those machines are slow. spoonbill apparently takes ~5h for
one run).

I've done a read through s_lock.h and the only remaining potential issue
that I see is that I have no idea if unixware's tas() is actually a safe
compiler barrier (it is a memory barrier). And I really, really can't
make myself care.

Greetings,

Andres Freund

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

On Tue, Sep 9, 2014 at 5:09 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-09-09 13:52:40 -0400, Robert Haas wrote:
>> I had forgotten that it needed an update.  Thanks for the reminder.  Here's v2.
>
> I've attached a incremental patch fixing minor gripes. Other than that I
> think you can go ahead with this once the buildfarm accepts the sparc
> fixes (man, those machines are slow. spoonbill apparently takes ~5h for
> one run).
>
> I've done a read through s_lock.h and the only remaining potential issue
> that I see is that I have no idea if unixware's tas() is actually a safe
> compiler barrier (it is a memory barrier). And I really, really can't
> make myself care.
 *    It is the responsibility of these macros to make sure that the compiler *    does not re-order accesses to shared
memoryto precede the actual lock
 
- *    acquisition, or follow the lock release.  Prior to PostgreSQL 9.5, this
- *    was the caller's responsibility, which meant that callers had to use
+ *    acquisition, or following the lock release.  Prior to PostgreSQL 9.5,
+ *  this was the caller's responsibility, which meant that callers had to use

AFAICS my version is right and your version is grammatically
incorrect. "re-order to proceed or follow" uses the same verb tense in
both branches of the or; "re-order to proceed or following" does not.

I agree that if there are problems on UnixWare, we can let anyone who
cares about UnixWare submit a patch to fix them.

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

On 2014-09-09 17:30:44 -0400, Robert Haas wrote:
> On Tue, Sep 9, 2014 at 5:09 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> > On 2014-09-09 13:52:40 -0400, Robert Haas wrote:
> >> I had forgotten that it needed an update.  Thanks for the reminder.  Here's v2.
> >
> > I've attached a incremental patch fixing minor gripes. Other than that I
> > think you can go ahead with this once the buildfarm accepts the sparc
> > fixes (man, those machines are slow. spoonbill apparently takes ~5h for
> > one run).
> >
> > I've done a read through s_lock.h and the only remaining potential issue
> > that I see is that I have no idea if unixware's tas() is actually a safe
> > compiler barrier (it is a memory barrier). And I really, really can't
> > make myself care.
> 
>   *    It is the responsibility of these macros to make sure that the compiler
>   *    does not re-order accesses to shared memory to precede the actual lock
> - *    acquisition, or follow the lock release.  Prior to PostgreSQL 9.5, this
> - *    was the caller's responsibility, which meant that callers had to use
> + *    acquisition, or following the lock release.  Prior to PostgreSQL 9.5,
> + *  this was the caller's responsibility, which meant that callers had to use
> 
> AFAICS my version is right and your version is grammatically
> incorrect. "re-order to proceed or follow" uses the same verb tense in
> both branches of the or; "re-order to proceed or following" does not.

Wasn't sure about that one. It read oddly to me, but then I'm not a
native speaker. And won't read the sentence often ;)

> I agree that if there are problems on UnixWare, we can let anyone who
> cares about UnixWare submit a patch to fix them.

Ok.

Greetings,

Andres Freund

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

On Tue, Sep 9, 2014 at 5:32 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-09-09 17:30:44 -0400, Robert Haas wrote:
>> On Tue, Sep 9, 2014 at 5:09 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> > On 2014-09-09 13:52:40 -0400, Robert Haas wrote:
>> >> I had forgotten that it needed an update.  Thanks for the reminder.  Here's v2.
>> >
>> > I've attached a incremental patch fixing minor gripes. Other than that I
>> > think you can go ahead with this once the buildfarm accepts the sparc
>> > fixes (man, those machines are slow. spoonbill apparently takes ~5h for
>> > one run).
>> >
>> > I've done a read through s_lock.h and the only remaining potential issue
>> > that I see is that I have no idea if unixware's tas() is actually a safe
>> > compiler barrier (it is a memory barrier). And I really, really can't
>> > make myself care.
>>
>>   *    It is the responsibility of these macros to make sure that the compiler
>>   *    does not re-order accesses to shared memory to precede the actual lock
>> - *    acquisition, or follow the lock release.  Prior to PostgreSQL 9.5, this
>> - *    was the caller's responsibility, which meant that callers had to use
>> + *    acquisition, or following the lock release.  Prior to PostgreSQL 9.5,
>> + *  this was the caller's responsibility, which meant that callers had to use
>>
>> AFAICS my version is right and your version is grammatically
>> incorrect. "re-order to proceed or follow" uses the same verb tense in
>> both branches of the or; "re-order to proceed or following" does not.
>
> Wasn't sure about that one. It read oddly to me, but then I'm not a
> native speaker. And won't read the sentence often ;)
>
>> I agree that if there are problems on UnixWare, we can let anyone who
>> cares about UnixWare submit a patch to fix them.
>
> Ok.

So, that's committed, then. I think we should pick something that uses
spinlocks and is likely to fail spectacularly if we haven't got this
totally right yet, and de-volatilize it.  And then watch to see what
turns red in the buildfarm and/or which users start screaming.  I'm
inclined to propose lwlock.c as a candidate, since that's very widely
used and a place where we know there's significant contention.

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

On 2014-09-09 17:54:03 -0400, Robert Haas wrote:
> So, that's committed, then.

Yay, finally.

> I think we should pick something that uses
> spinlocks and is likely to fail spectacularly if we haven't got this
> totally right yet, and de-volatilize it.  And then watch to see what
> turns red in the buildfarm and/or which users start screaming.

Good plan.

> I'm inclined to propose lwlock.c as a candidate, since that's very
> widely used and a place where we know there's significant contention.

I suggest, additionally possibly, GetSnapshotData(). It's surely one of
the hottest functions in postgres, and I've seen some performance
increases from de-volatilizing it. IIRC it requires one volatile cast in
one place to enforce that a variable is accessed only once. Not sure if
we want to add such volatile casts or use something like linux'
ACCESS_ONCE macros for some common types. Helps to grep for places
worthy of inspection.

Greetings,

Andres Freund

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

On Tue, Sep 9, 2014 at 6:00 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-09-09 17:54:03 -0400, Robert Haas wrote:
>> So, that's committed, then.
>
> Yay, finally.
>
>> I think we should pick something that uses
>> spinlocks and is likely to fail spectacularly if we haven't got this
>> totally right yet, and de-volatilize it.  And then watch to see what
>> turns red in the buildfarm and/or which users start screaming.
>
> Good plan.
>
>> I'm inclined to propose lwlock.c as a candidate, since that's very
>> widely used and a place where we know there's significant contention.
>
> I suggest, additionally possibly, GetSnapshotData(). It's surely one of
> the hottest functions in postgres, and I've seen some performance
> increases from de-volatilizing it. IIRC it requires one volatile cast in
> one place to enforce that a variable is accessed only once. Not sure if
> we want to add such volatile casts or use something like linux'
> ACCESS_ONCE macros for some common types. Helps to grep for places
> worthy of inspection.

GetSnapshotData() isn't quite to the point, because it's using a
volatile pointer, but not because of anything about spinlock
manipulation.  That would, perhaps, be a good test for barriers, but
not for this.

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

On 2014-09-09 17:54:03 -0400, Robert Haas wrote:
> So, that's committed, then. I think we should pick something that uses
> spinlocks and is likely to fail spectacularly if we haven't got this
> totally right yet, and de-volatilize it.  And then watch to see what
> turns red in the buildfarm and/or which users start screaming.  I'm
> inclined to propose lwlock.c as a candidate, since that's very widely
> used and a place where we know there's significant contention.

Did you consider removing the volatiles from bufmgr.c? There's lots of
volatiles in there and most of them don't seem to have been added in a
principled way. I'm looking at my old patch for lockless pin/unpin of
buffers and it'd look a lot cleaner without.

Greetings,

Andres Freund

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

On Wed, Sep 24, 2014 at 2:45 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-09-09 17:54:03 -0400, Robert Haas wrote:
>> So, that's committed, then. I think we should pick something that uses
>> spinlocks and is likely to fail spectacularly if we haven't got this
>> totally right yet, and de-volatilize it.  And then watch to see what
>> turns red in the buildfarm and/or which users start screaming.  I'm
>> inclined to propose lwlock.c as a candidate, since that's very widely
>> used and a place where we know there's significant contention.
>
> Did you consider removing the volatiles from bufmgr.c? There's lots of
> volatiles in there and most of them don't seem to have been added in a
> principled way. I'm looking at my old patch for lockless pin/unpin of
> buffers and it'd look a lot cleaner without.

I hadn't thought of it, but it sounds like a good idea.

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