better atomics
On Wed, Oct 16, 2013 at 12:52 PM, Andres Freund <andres@2ndquadrant.com> wrote:
Partially that only works because we sprinkle 'volatile's on lots of
places. That can actually hurt performance... it'd usually be something
like:
#define pg_atomic_load(uint32) (*(volatile uint32 *)(atomic))Even if not needed in some places because a variable is already
volatile, it's very helpful in pointing out what happens and where you
need to be careful.
That's fine with me.
* uint32 pg_atomic_exchange_u32(uint32 *ptr, uint32 val)
* bool pg_atomic_compare_exchange_u32(uint32 *ptr, uint32 *expected, uint32 newval)
* uint32 pg_atomic_fetch_add_u32(uint32 *ptr, uint32 add)
* uint32 pg_atomic_fetch_sub_u32(uint32 *ptr, uint32 add)
* uint32 pg_atomic_fetch_and_u32(uint32 *ptr, uint32 add)
* uint32 pg_atomic_fetch_or_u32(uint32 *ptr, uint32 add)Do we really need all of those? Compare-and-swap is clearly needed,
and fetch-and-add is also very useful. I'm not sure about the rest.
Not that I necessarily object to having them, but it will be a lot
more work.Well, _sub can clearly be implemented with _add generically. I find code
using _sub much easier to read than _add(-whatever), but that's maybe
just me.
Yeah, I wouldn't bother with _sub.
But _and, _or are really useful because they can be used to atomically
clear and set flag bits.
Until we have some concrete application that requires this
functionality for adequate performance, I'd be inclined not to bother.
I think we have bigger fish to fry, and (to extend my nautical
metaphor) trying to get this working everywhere might amount to trying
to boil the ocean.
* u64 variants of the above
* bool pg_atomic_test_set(void *ptr)
* void pg_atomic_clear(void *ptr)What are the intended semantics here?
It's basically TAS() without defining whether setting a value that needs
to be set is a 1 or a 0. Gcc provides this and I think we should make
our spinlock implementation use it if there is no special cased
implementation available.
I'll reserve judgement until I see the patch.
More general question: how do we move the ball down the field in this
area? I'm willing to do some of the legwork, but I doubt I can do all
of it, and I really think we need to make some progress here soon, as
it seems that you and I and Ants are all running into the same
problems in slightly different ways. What's our strategy for getting
something done here?That's a pretty good question.
I am willing to write the gcc implementation, plus the generic wrappers
and provide the infrastructure to override it per-platform. I won't be
able to do anything about non-linux, non-gcc platforms in that timeframe
though.I was thinking of something like:
include/storage/atomic.h
include/storage/atomic-gcc.h
include/storage/atomic-msvc.h
include/storage/atomic-acc-ia64.h
where atomic.h first has a list of #ifdefs including the specialized
files and then lots of #ifndef providing generic variants if not
already provided by the platorm specific file.
Seems like we might want to put some of this in one of the various
directories called "port", or maybe a new subdirectory of one of them.
This basically sounds sane, but I'm unwilling to commit to dropping
support for all obscure platforms we currently support unless there
are about 500 people +1ing the idea, so I think we need to think about
what happens when we don't have platform support for these primitives.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-10-16 14:30:34 -0400, Robert Haas wrote:
But _and, _or are really useful because they can be used to atomically
clear and set flag bits.Until we have some concrete application that requires this
functionality for adequate performance, I'd be inclined not to bother.
I think we have bigger fish to fry, and (to extend my nautical
metaphor) trying to get this working everywhere might amount to trying
to boil the ocean.
Well, I actually have a very, very preliminary patch using them in the
course of removing the spinlocks in PinBuffer() (via LockBufHdr()).
I think it's also going to be easier to add all required atomics at once
than having to go over several platforms in independent feature
patches adding atomics one by one.
I was thinking of something like:
include/storage/atomic.h
include/storage/atomic-gcc.h
include/storage/atomic-msvc.h
include/storage/atomic-acc-ia64.h
where atomic.h first has a list of #ifdefs including the specialized
files and then lots of #ifndef providing generic variants if not
already provided by the platorm specific file.
Seems like we might want to put some of this in one of the various
directories called "port", or maybe a new subdirectory of one of them.
Hm. I'd have to be src/include/port, right? Not sure if putting some
files there will be cleaner, but I don't mind it either.
This basically sounds sane, but I'm unwilling to commit to dropping
support for all obscure platforms we currently support unless there
are about 500 people +1ing the idea, so I think we need to think about
what happens when we don't have platform support for these primitives.
I think the introduction of the atomics really isn't much dependent on
the removal. The only thing I'd touch around platforms in that patch is
adding a generic fallback pg_atomic_test_and_set() to s_lock.h and
remove the special case usages of __sync_lock_test_and_set() (Arm64,
some 32 bit arms).
It's the patches that would like to rely on atomics that will have the
problem :(
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@2ndquadrant.com> writes:
On 2013-10-16 14:30:34 -0400, Robert Haas wrote:
But _and, _or are really useful because they can be used to atomically
clear and set flag bits.
Until we have some concrete application that requires this
functionality for adequate performance, I'd be inclined not to bother.
I think we have bigger fish to fry, and (to extend my nautical
metaphor) trying to get this working everywhere might amount to trying
to boil the ocean.
Well, I actually have a very, very preliminary patch using them in the
course of removing the spinlocks in PinBuffer() (via LockBufHdr()).
I think we need to be very, very wary of making our set of required
atomics any larger than it absolutely has to be. The more stuff we
require, the closer we're going to be to making PG a program that only
runs well on Intel. I am not comforted by the "gcc will provide good
implementations of the atomics everywhere" argument, because in fact
it won't. See for example the comment associated with our only existing
use of gcc atomics:
* On ARM, we use __sync_lock_test_and_set(int *, int) if available, and if
* not fall back on the SWPB instruction. SWPB does not work on ARMv6 or
* later, so the compiler builtin is preferred if available. Note also that
* the int-width variant of the builtin works on more chips than other widths.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
That's not a track record that should inspire much faith that complete
sets of atomics will exist elsewhere. What's more, we don't just need
atomics that *work*, we need atomics that are *fast*, else the whole
exercise turns into pessimization not improvement. The more atomic ops
we depend on, the more likely it is that some of them will involve kernel
support on some chips, destroying whatever performance improvement we
hoped to get.
... The only thing I'd touch around platforms in that patch is
adding a generic fallback pg_atomic_test_and_set() to s_lock.h and
remove the special case usages of __sync_lock_test_and_set() (Arm64,
some 32 bit arms).
Um, if we had a "generic" pg_atomic_test_and_set(), s_lock.h would be
about one line long. The above quote seems to me to be exactly the kind
of optimism that is not warranted in this area.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-10-16 22:39:07 +0200, Tom Lane wrote:
Andres Freund <andres@2ndquadrant.com> writes:
On 2013-10-16 14:30:34 -0400, Robert Haas wrote:
But _and, _or are really useful because they can be used to atomically
clear and set flag bits.Until we have some concrete application that requires this
functionality for adequate performance, I'd be inclined not to bother.
I think we have bigger fish to fry, and (to extend my nautical
metaphor) trying to get this working everywhere might amount to trying
to boil the ocean.Well, I actually have a very, very preliminary patch using them in the
course of removing the spinlocks in PinBuffer() (via LockBufHdr()).I think we need to be very, very wary of making our set of required
atomics any larger than it absolutely has to be. The more stuff we
require, the closer we're going to be to making PG a program that only
runs well on Intel.
Well, essentially I am advocating to support basically three operations:
* compare and swap
* atomic add (and by that sub)
* test and set
The other operations are just porcelain around that.
With compare and swap both the others can be easily implemented if
neccessary.
Note that e.g. linux - running on all platforms we're talking about but
VAX - exensively and unconditionally uses atomic operations widely. So I
think we don't have to be too afraid about non-intel performance here.
I am not comforted by the "gcc will provide good
implementations of the atomics everywhere" argument, because in fact
it won't. See for example the comment associated with our only existing
use of gcc atomics:* On ARM, we use __sync_lock_test_and_set(int *, int) if available, and if
* not fall back on the SWPB instruction. SWPB does not work on ARMv6 or
* later, so the compiler builtin is preferred if available. Note also that
* the int-width variant of the builtin works on more chips than other widths.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^That's not a track record that should inspire much faith that complete
sets of atomics will exist elsewhere. What's more, we don't just need
atomics that *work*, we need atomics that are *fast*, else the whole
exercise turns into pessimization not improvement. The more atomic ops
we depend on, the more likely it is that some of them will involve kernel
support on some chips, destroying whatever performance improvement we
hoped to get.
Hm. I can't really follow. We *prefer* to use __sync_lock_test_and_set
in contrast to our own open-coded implementation, right? And the comment
about some hardware only supporting "int-width" matches that I only want to
require u32 atomics support, right?
I completely agree that we cannot rely on 8byte math or similar (16byte
cmpxchg) to be supported by all platforms. That indeed would require
kernel fallbacks on e.g. ARM.
... The only thing I'd touch around platforms in that patch is
adding a generic fallback pg_atomic_test_and_set() to s_lock.h and
remove the special case usages of __sync_lock_test_and_set() (Arm64,
some 32 bit arms).Um, if we had a "generic" pg_atomic_test_and_set(), s_lock.h would be
about one line long. The above quote seems to me to be exactly the kind
of optimism that is not warranted in this area.
Well, I am somewhat hesitant to change s_lock for more platforms than
necessary. So I proposed to restructure it in a way that leaves all
existing implementations in place that do not already rely on
__sync_lock_test_and_set().
There's also SPIN_DELAY btw, which I don't see any widely provided
intrinsics for.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi,
This is the first real submission of how I think our atomics
architecture should look like. It's definitely a good bit from complete,
please view it from that POV!
The most important thing I need at this point is feedback whether the
API in general looks ok and what should be changed, before I spend the
time to implement it nicely everywhere.
The API is loosely modeled to match C11's atomics
http://en.cppreference.com/w/c/atomic library, at least to the degree
that the latter could be used to implement pg's atomics.
Please note:
* Only gcc's implementation is tested. I'd be really surprised if it
compiled on anything else.
* I've currently replaced nearly all of s_lock.h to use parts of the
atomics API. We might rather use it's contents to implement atomics
'flags' on some of the wierder platforms out there.
* I've integrated barrier.h into the atomics code. It's pretty much a
required part, and compiler/arch specific so that seems to make sense.
* 'autoreconf' needs to be run after applying the patchset. I got some
problems with running autoconf 2.63 here, using 2.69 makes the diff to
big.
Questions:
* What do you like/dislike about the API (storage/atomics.h)
* decide whether it's ok to rely on inline functions or whether we need
to provide out-of-line versions. I think we should just bite the
bullet and require support. Some very old arches might need to live with
warnings. Patch 01 tries to silence them on HP-UX.
* decide whether we want to keep the semaphore fallback for
spinlocks. I strongly vote for removing it. The patchset provides
compiler based default implementations for atomics, so it's unlikely
to be helpful when bringing up a new platform.
TODO:
* actually test on other platforms that new gcc
* We should probably move most of the (documentation) content of
s_lock.h somewhere else.
* Integrate the spinlocks based fallback for platforms that only provide
something like TAS().
Comments?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Nov 15, 2013 at 3:04 PM, Andres Freund <andres@2ndquadrant.com> wrote:
Questions:
* What do you like/dislike about the API (storage/atomics.h)
* decide whether it's ok to rely on inline functions or whether we need
to provide out-of-line versions. I think we should just bite the
bullet and require support. Some very old arches might need to live with
warnings. Patch 01 tries to silence them on HP-UX.
* decide whether we want to keep the semaphore fallback for
spinlocks. I strongly vote for removing it. The patchset provides
compiler based default implementations for atomics, so it's unlikely
to be helpful when bringing up a new platform.
On point #2, I don't personally know of any systems that I care about
where inlining isn't supported. However, we've gone to quite a bit of
trouble relatively recently to keep things working for platforms where
that is the case, so I feel the need for an obligatory disclaimer: I
will not commit any patch that moves the wood in that area unless
there is massive consensus that this is an acceptable way forward.
Similarly for point #3: Heikki not long ago went to the trouble of
unbreaking --disable-spinlocks, which suggests that there is at least
some interest in continuing to be able to run in that mode. I'm
perfectly OK with the decision that we don't care about that any more,
but I will not be the one to make that decision, and I think it
requires a greater-than-normal level of consensus.
On point #1, I dunno. It looks like a lot of rearrangement to me, and
I'm not really sure what the final form of it is intended to be. I
think it desperately needs a README explaining what the point of all
this is and how to add support for a new platform or compiler if yours
doesn't work.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
On point #2, I don't personally know of any systems that I care about
where inlining isn't supported. However, we've gone to quite a bit of
trouble relatively recently to keep things working for platforms where
that is the case, so I feel the need for an obligatory disclaimer: I
will not commit any patch that moves the wood in that area unless
there is massive consensus that this is an acceptable way forward.
Similarly for point #3: Heikki not long ago went to the trouble of
unbreaking --disable-spinlocks, which suggests that there is at least
some interest in continuing to be able to run in that mode. I'm
perfectly OK with the decision that we don't care about that any more,
but I will not be the one to make that decision, and I think it
requires a greater-than-normal level of consensus.
Hm. Now that I think about it, isn't Peter proposing to break systems
without working "inline" over here?
/messages/by-id/1384257026.8059.5.camel@vanquo.pezone.net
Maybe that patch needs a bit more discussion. I tend to agree that
significantly moving our portability goalposts shouldn't be undertaken
lightly. On the other hand, it is 2013, and so it seems not unreasonable
to reconsider choices last made more than a dozen years ago.
Here's an idea that might serve as a useful touchstone for making choices:
compiler inadequacies are easier to fix than hardware/OS inadequacies.
Even a dozen years ago, people could get gcc running on any platform of
interest, and I seriously doubt that's less true today than back then.
So if anyone complains "my compiler doesn't support 'const'", we could
reasonably reply "so install gcc". On the other hand, if the complaint is
"my hardware doesn't support 64-bit CAS", it's not reasonable to tell them
to buy a new server.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 11/19/13, 9:57 AM, Tom Lane wrote:
Hm. Now that I think about it, isn't Peter proposing to break systems
without working "inline" over here?
/messages/by-id/1384257026.8059.5.camel@vanquo.pezone.net
No, that's about const, volatile, #, and memcmp.
I don't have an informed opinion about requiring inline support
(although it would surely be nice).
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-11-19 09:57:20 -0500, Tom Lane wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On point #2, I don't personally know of any systems that I care about
where inlining isn't supported. However, we've gone to quite a bit of
trouble relatively recently to keep things working for platforms where
that is the case, so I feel the need for an obligatory disclaimer: I
will not commit any patch that moves the wood in that area unless
there is massive consensus that this is an acceptable way forward.
Similarly for point #3: Heikki not long ago went to the trouble of
unbreaking --disable-spinlocks, which suggests that there is at least
some interest in continuing to be able to run in that mode. I'm
perfectly OK with the decision that we don't care about that any more,
but I will not be the one to make that decision, and I think it
requires a greater-than-normal level of consensus.Hm. Now that I think about it, isn't Peter proposing to break systems
without working "inline" over here?
/messages/by-id/1384257026.8059.5.camel@vanquo.pezone.net
Hm. Those don't directly seem to affect our inline tests. Our test is
PGAC_C_INLINE which itself uses AC_C_INLINE and then tests that
unreferenced inlines don't generate warnings.
Maybe that patch needs a bit more discussion. I tend to agree that
significantly moving our portability goalposts shouldn't be undertaken
lightly. On the other hand, it is 2013, and so it seems not unreasonable
to reconsider choices last made more than a dozen years ago.
I don't think there's even platforms out there that don't support const
inlines, just some that unnecessarily generate warnings. But since
builds on those platforms are already littered with warnings, that
doesn't seem something we need to majorly be concerned with.
The only animal we have that doesn't support quiet inlines today is
HP-UX/ac++, and I think - as in patch 1 in the series - we might be able
to simply suppress the warning there.
Here's an idea that might serve as a useful touchstone for making choices:
compiler inadequacies are easier to fix than hardware/OS inadequacies.
Even a dozen years ago, people could get gcc running on any platform of
interest, and I seriously doubt that's less true today than back then.
So if anyone complains "my compiler doesn't support 'const'", we could
reasonably reply "so install gcc".
Agreed.
On the other hand, if the complaint is
"my hardware doesn't support 64-bit CAS", it's not reasonable to tell them
to buy a new server.
Agreed. I've am even wondering about 32bit CAS since it's not actually
that hard to emulate it using spinlocks. Certainly less work than
arguing about removing stone-age platforms ;)
There's no fundamental issue with continuing to support semaphore based
spinlocks I am just wondering about the point of supporting that
configuration since it will always yield horrible performance.
The only fundamental thing that I don't immediately see how we can
support is the spinlock based memory barrier since that introduces a
circularity (atomics need barrier, barrier needs spinlocks, spinlock
needs atomics).
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Nov 19, 2013 at 9:57 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On point #2, I don't personally know of any systems that I care about
where inlining isn't supported. However, we've gone to quite a bit of
trouble relatively recently to keep things working for platforms where
that is the case, so I feel the need for an obligatory disclaimer: I
will not commit any patch that moves the wood in that area unless
there is massive consensus that this is an acceptable way forward.
Similarly for point #3: Heikki not long ago went to the trouble of
unbreaking --disable-spinlocks, which suggests that there is at least
some interest in continuing to be able to run in that mode. I'm
perfectly OK with the decision that we don't care about that any more,
but I will not be the one to make that decision, and I think it
requires a greater-than-normal level of consensus.Hm. Now that I think about it, isn't Peter proposing to break systems
without working "inline" over here?
/messages/by-id/1384257026.8059.5.camel@vanquo.pezone.netMaybe that patch needs a bit more discussion. I tend to agree that
significantly moving our portability goalposts shouldn't be undertaken
lightly. On the other hand, it is 2013, and so it seems not unreasonable
to reconsider choices last made more than a dozen years ago.
Sure. inline isn't C89, and to date, we've been quite rigorous about
enforcing C89-compliance, so I'm going to try not to be the guy that
casually breaks that. On the other hand, I haven't personally seen
any systems that don't have inline in a very long time. The first
systems I did development on were not even C89; they were K&R, and you
had to use old-style function declarations. It's probably safe to say
that they didn't support inline, either, though I don't recall trying
it. But the last time I used a system like that was probably in the
early nineties, and it's been a long time since then. However, I
upgrade my hardware about every 2 years, so I'm not the best guy to
say what the oldest stuff people still care about is.
Here's an idea that might serve as a useful touchstone for making choices:
compiler inadequacies are easier to fix than hardware/OS inadequacies.
Even a dozen years ago, people could get gcc running on any platform of
interest, and I seriously doubt that's less true today than back then.
So if anyone complains "my compiler doesn't support 'const'", we could
reasonably reply "so install gcc". On the other hand, if the complaint is
"my hardware doesn't support 64-bit CAS", it's not reasonable to tell them
to buy a new server.
I generally agree with that principle, but I'd offer the caveat that
some people do still have valid reasons for wanting to use non-GCC
compilers. I have been told that HP's aCC produces better results on
Itanium than gcc, for example. Still, I think we could probably make
a list of no more than half-a-dozen compilers that we really care
about - the popular open source ones (is there anything we need to
care about other than gcc and clang?) and the ones supported by large
vendors (IBM's ICC and HP's aCC being the ones that I know about) and
adopt features that are supported by everything on that list.
Another point is that, some releases ago, we began requiring working
64-bit arithmetic as per commit
d15cb38dec01fcc8d882706a3fa493517597c76b. We might want to go through
our list of nominally-supported platforms and see whether any of them
would fail to compile for that reason. Based on the commit message
for the aforementioned commit, such platforms haven't been viable for
PostgreSQL since 8.3, so we're not losing anything by canning them.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Nov 19, 2013 at 10:16 AM, Andres Freund <andres@2ndquadrant.com> wrote:
On the other hand, if the complaint is
"my hardware doesn't support 64-bit CAS", it's not reasonable to tell them
to buy a new server.Agreed. I've am even wondering about 32bit CAS since it's not actually
that hard to emulate it using spinlocks. Certainly less work than
arguing about removing stone-age platforms ;)There's no fundamental issue with continuing to support semaphore based
spinlocks I am just wondering about the point of supporting that
configuration since it will always yield horrible performance.The only fundamental thing that I don't immediately see how we can
support is the spinlock based memory barrier since that introduces a
circularity (atomics need barrier, barrier needs spinlocks, spinlock
needs atomics).
We've been pretty much assuming for a long time that calling a
function in another translation unit acts as a compiler barrier.
There's a lot of code that isn't actually safe against global
optimization; we assume, for example, that memory accesses can't move
over an LWLockAcquire(), but that's just using spinlocks internally,
and those aren't guaranteed to be compiler barriers, per previous
discussion. So one idea for a compiler barrier is just to define a
function call pg_compiler_barrier() in a file by itself, and make that
the fallback implementation. That will of course fail if someone uses
a globally optimizing compiler, but I think it'd be OK to say that if
you want to do that, you'd better have a real barrier implementation.
Right now, it's probably unsafe regardless.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-11-19 09:12:42 -0500, Robert Haas wrote:
On point #1, I dunno. It looks like a lot of rearrangement to me, and
I'm not really sure what the final form of it is intended to be.
Understandable. I am not that sure what parts we want to rearange
either. We very well could leave barrier.h and s_lock.h untouched and
just maintain the atomics stuff in parallel and switch over at some
later point.
I've mainly switched over s_lock.h to a) get some good coverage of the
atomics code b) make sure the api works fine for it. We could add the
atomics.h based implementation as a fallback for the cases in which no
native implementation is available.
I think it desperately needs a README explaining what the point of all
this is and how to add support for a new platform or compiler if yours
doesn't work.
Ok, I'll work on that. It would be useful to get feedback on the
"frontend" API in atomics.h though. If people are unhappy with the API
it might very well change how we can implement the API on different
architectures.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Nov 19, 2013 at 10:26 AM, Andres Freund <andres@2ndquadrant.com> wrote:
On 2013-11-19 09:12:42 -0500, Robert Haas wrote:
On point #1, I dunno. It looks like a lot of rearrangement to me, and
I'm not really sure what the final form of it is intended to be.Understandable. I am not that sure what parts we want to rearange
either. We very well could leave barrier.h and s_lock.h untouched and
just maintain the atomics stuff in parallel and switch over at some
later point.
I've mainly switched over s_lock.h to a) get some good coverage of the
atomics code b) make sure the api works fine for it. We could add the
atomics.h based implementation as a fallback for the cases in which no
native implementation is available.I think it desperately needs a README explaining what the point of all
this is and how to add support for a new platform or compiler if yours
doesn't work.Ok, I'll work on that. It would be useful to get feedback on the
"frontend" API in atomics.h though. If people are unhappy with the API
it might very well change how we can implement the API on different
architectures.
Sure, but if people can't understand the API, then it's hard to decide
whether to be unhappy with it.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Peter Eisentraut <peter_e@gmx.net> writes:
On 11/19/13, 9:57 AM, Tom Lane wrote:
Hm. Now that I think about it, isn't Peter proposing to break systems
without working "inline" over here?
/messages/by-id/1384257026.8059.5.camel@vanquo.pezone.net
No, that's about const, volatile, #, and memcmp.
Ah, sorry, not enough caffeine absorbed yet. Still, we should stop
to think about whether this represents an undesirable move of the
portability goalposts. The first three of these are certainly
compiler issues, and I personally don't have a problem with blowing
off compilers that still haven't managed to implement all of C89 :-(.
I'm not clear on which systems had the memcmp issue --- do we have
the full story on that?
I don't have an informed opinion about requiring inline support
(although it would surely be nice).
inline is C99, and we've generally resisted requiring C99 features.
Maybe it's time to move that goalpost, and maybe not.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-11-19 10:23:57 -0500, Robert Haas wrote:
The only fundamental thing that I don't immediately see how we can
support is the spinlock based memory barrier since that introduces a
circularity (atomics need barrier, barrier needs spinlocks, spinlock
needs atomics).We've been pretty much assuming for a long time that calling a
function in another translation unit acts as a compiler barrier.
There's a lot of code that isn't actually safe against global
optimization; we assume, for example, that memory accesses can't move
over an LWLockAcquire(), but that's just using spinlocks internally,
and those aren't guaranteed to be compiler barriers, per previous
discussion. So one idea for a compiler barrier is just to define a
function call pg_compiler_barrier() in a file by itself, and make that
the fallback implementation. That will of course fail if someone uses
a globally optimizing compiler, but I think it'd be OK to say that if
you want to do that, you'd better have a real barrier implementation.
That works for compiler, but not for memory barriers :/
Right now, it's probably unsafe regardless.
Yes, I have pretty little trust in the current state. Both from the
infrastructure perspective (spinlocks, barriers) as from individual
pieces of code. To a good part we're probably primarily protected by
x86's black magic and the fact that everyone with sufficient concurrency
to see problems uses x86.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Nov 19, 2013 at 10:31 AM, Andres Freund <andres@2ndquadrant.com> wrote:
On 2013-11-19 10:23:57 -0500, Robert Haas wrote:
The only fundamental thing that I don't immediately see how we can
support is the spinlock based memory barrier since that introduces a
circularity (atomics need barrier, barrier needs spinlocks, spinlock
needs atomics).We've been pretty much assuming for a long time that calling a
function in another translation unit acts as a compiler barrier.
There's a lot of code that isn't actually safe against global
optimization; we assume, for example, that memory accesses can't move
over an LWLockAcquire(), but that's just using spinlocks internally,
and those aren't guaranteed to be compiler barriers, per previous
discussion. So one idea for a compiler barrier is just to define a
function call pg_compiler_barrier() in a file by itself, and make that
the fallback implementation. That will of course fail if someone uses
a globally optimizing compiler, but I think it'd be OK to say that if
you want to do that, you'd better have a real barrier implementation.That works for compiler, but not for memory barriers :/
True, but we already assume that a spinlock is a memory barrier minus
a compiler barrier. So if you have a working compiler barrier, you
ought to be able to fix spinlocks to be memory barriers. And then, if
you need a memory barrier for some other purpose, you can always fall
back to acquiring and releasing a spinlock.
Maybe that's too contorted.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-11-19 10:30:24 -0500, Tom Lane wrote:
I don't have an informed opinion about requiring inline support
(although it would surely be nice).inline is C99, and we've generally resisted requiring C99 features.
Maybe it's time to move that goalpost, and maybe not.
But it's a part of C99 that was very widely implemented before, so even
if we don't want to rely on C99 in its entirety, relying on inline
support is realistic.
I think, independent from atomics, the readability & maintainability win
by relying on inline functions instead of long macros, potentially with
multiple eval hazards, or contortions like ILIST_INCLUDE_DEFINITIONS is
significant.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@2ndquadrant.com> writes:
The only animal we have that doesn't support quiet inlines today is
HP-UX/ac++, and I think - as in patch 1 in the series - we might be able
to simply suppress the warning there.
Or just not worry about it, if it's only a warning? Or does the warning
mean code bloat (lots of useless copies of the inline function)?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-11-19 10:37:35 -0500, Tom Lane wrote:
Andres Freund <andres@2ndquadrant.com> writes:
The only animal we have that doesn't support quiet inlines today is
HP-UX/ac++, and I think - as in patch 1 in the series - we might be able
to simply suppress the warning there.Or just not worry about it, if it's only a warning? Or does the warning
mean code bloat (lots of useless copies of the inline function)?
I honestly have no idea whether it causes code bloat - I'd be surprised
if it did since it detects that they are unused, but I cannot rule it
out entirely.
The suggested patch - untested since I have no access to HP-UX - just
adds +W2177 to the compiler's commandline in template/hpux which
supposedly suppressed that warning.
I think removing the quiet inline test is a good idea, but that doesn't
preclude fixing the warnings at the same time.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi,
On 2013-11-19 10:27:57 -0500, Robert Haas wrote:
Ok, I'll work on that. It would be useful to get feedback on the
"frontend" API in atomics.h though. If people are unhappy with the API
it might very well change how we can implement the API on different
architectures.Sure, but if people can't understand the API, then it's hard to decide
whether to be unhappy with it.
Fair point, while there's already an explantory blurb ontop of atomics.h
and the semantics of the functions should all be documented, it doesn't
explain the API on a high level. I've expanded it a bit
The attached patches compile and make check successfully on linux x86,
amd64 and msvc x86 and amd64. This time and updated configure is
included. The majority of operations still rely on CAS emulation.
I've copied the introduction here for easier reading:
/*-------------------------------------------------------------------------
*
* atomics.h
* Generic atomic operations support.
*
* Hardware and compiler dependent functions for manipulating memory
* atomically and dealing with cache coherency. Used to implement locking
* facilities and other concurrency safe constructs.
*
* There's three data types which can be manipulated atomically:
*
* * pg_atomic_flag - supports atomic test/clear operations, useful for
* implementing spinlocks and similar constructs.
*
* * pg_atomic_uint32 - unsigned 32bit integer that can correctly manipulated
* by several processes at the same time.
*
* * pg_atomic_uint64 - optional 64bit variant of pg_atomic_uint32. Support
* can be tested by checking for PG_HAVE_64_BIT_ATOMICS.
*
* The values stored in theses cannot be accessed using the API provided in
* thise file, i.e. it is not possible to write statements like `var +=
* 10`. Instead, for this example, one would have to use
* `pg_atomic_fetch_add_u32(&var, 10)`.
*
* This restriction has several reasons: Primarily it doesn't allow non-atomic
* math to be performed on these values which wouldn't be concurrency
* safe. Secondly it allows to implement these types ontop of facilities -
* like C11's atomics - that don't provide direct access. Lastly it serves as
* a useful hint what code should be looked at more carefully because of
* concurrency concerns.
*
* To be useful they usually will need to be placed in memory shared between
* processes or threads, most frequently by embedding them in structs. Be
* careful to align atomic variables to their own size!
*
* Before variables of one these types can be used they needs to be
* initialized using the type's initialization function (pg_atomic_init_flag,
* pg_atomic_init_u32 and pg_atomic_init_u64) or in case of global
* pg_atomic_flag variables using the PG_ATOMIC_INIT_FLAG macro. These
* initializations have to be performed before any (possibly concurrent)
* access is possible, otherwise the results are undefined.
*
* For several mathematical operations two variants exist: One that returns
* the old value before the operation was performed, and one that that returns
* the new value. *_fetch_<op> variants will return the old value,
* *_<op>_fetch the new one.
*
*
* To bring up postgres on a platform/compiler at the very least
* either one of
* * pg_atomic_test_set_flag(), pg_atomic_init_flag(), pg_atomic_clear_flag()
* * pg_atomic_compare_exchange_u32()
* * pg_atomic_exchange_u32()
* and all of
* * pg_compiler_barrier(), pg_memory_barrier()a
* need to be implemented. There exist generic, hardware independent,
* implementations for several compilers which might be sufficient, although
* possibly not optimal, for a new platform.
*
* To add support for a new architecture review whether the compilers used on
* that platform already provide adequate support in the files included
* "Compiler specific" section below. If not either add an include for a new
* file adding generic support for your compiler there and/or add/update an
* architecture specific include in the "Architecture specific" section below.
*
* Operations that aren't implemented by either architecture or compiler
* specific code are implemented ontop of compare_exchange() loops or
* spinlocks.
*
* Use higher level functionality (lwlocks, spinlocks, heavyweight locks)
* whenever possible. Writing correct code using these facilities is hard.
*
* For an introduction to using memory barriers within the PostgreSQL backend,
* see src/backend/storage/lmgr/README.barrier
*
* Portions Copyright (c) 1996-2013, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
*
* src/include/storage/atomics.h
*
*-------------------------------------------------------------------------
*/
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
