Re: better atomics - v0.5

On Wed, Jun 25, 2014 at 1:14 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> [sorry for the second copy Robert]
>
> Attached is a new version of the atomic operations patch. Lots has
> changed since the last post:
>
> * gcc, msvc work. acc, xlc, sunpro have blindly written support which
> should be relatively easy to fix up. All try to implement TAS, 32 bit
> atomic add, 32 bit compare exchange; some do it for 64bit as well.
>
> * x86 gcc inline assembly means that we support every gcc version on x86
> >= i486; even when the __sync APIs aren't available yet.
>
> * 'inline' support isn't required anymore. We fall back to
> ATOMICS_INCLUDE_DEFINITIONS/STATIC_IF_INLINE etc. trickery.
>
> * When the current platform doesn't have support for atomic operations a
> spinlock backed implementation is used. This can be forced using
> --disable-atomics.
>
> That even works when semaphores are used to implement spinlocks (a
> separate array is used there to avoid nesting problems). It contrast
> to an earlier implementation this even works when atomics are mapped
> to different addresses in individual processes (think dsm).
>
> * s_lock.h falls back to the atomics.h provided APIs iff it doesn't have
> native support for the current platform. This can be forced by
> defining USE_ATOMICS_BASED_SPINLOCKS. Due to generic compiler
> intrinsics based implementations that should make it easier to bring
> up postgres on different platfomrs.
>
> * I tried to improve the documentation of the facilities in
> src/include/storage/atomics.h. Including documentation of the various
> barrier semantics.
>
> * There's tests in regress.c that get call via a SQL function from the
> regression tests.
>
> * Lots of other details changed, but that's the major pieces.

Review:

- The changes to spin.c include unnecessary whitespace adjustments.
- The new argument to s_init_lock_sema() isn't used.
- "on top" is two words, not one ("ontop").
- The changes to pg_config_manual.h add a superfluous blank line.
- Are the regression tests in regress.c likely to catch anything? Really?
- The patch adds a test for PGAC_HAVE_GCC__SYNC_INT32_ATOMICS, which
seems to be testing for __builtin_constant_p. I don't see that being
used in the patch anywhere, though; and also, why doesn't the naming
match?
- s_lock.h adds some fallback code to use atomics to define TAS on
platforms where GCC supports atomics but where we do not have a
working TAS implementation. However, this supposed fallback code
defines HAS_TEST_AND_SET unconditionally, so I think it will get used
even if we don't have (or have disabled via configure) atomics.
- I completely loathe the file layout you've proposed in
src/include/storage. If we're going to have separate #include files
for each architecture (and I'd rather we didn't go that route, because
it's messy and unlike what we have now), then shouldn't that stuff go
in src/include/port or a new directory src/include/port/atomics?
Polluting src/include/storage with a whole bunch of files called
atomics-whatever.h strikes me as completely ugly, and there's a lot of
duplicate code in there.
- What's the point of defining pg_read_barrier() to be
pg_read_barrier_impl() and pg_write_barrier() to be
pg_write_barrier_impl() and so forth? That seems like unnecessary
notation.
- Should SpinlockSemaInit() be assigning SpinlockSemas() to a local
variable instead of re-executing it every time? Granted, the compiler
might inline this somehow, but...

More generally, I'm really wondering if you're making the
compare-and-swap implementation a lot more complicated than it needs
to be. How much would we lose if we supported compiler intrinsics on
versions of GCC and MSVC that have it and left everything else to
future patches? I suspect this patch could be about 20% of its
current size and give us everything that we need. I've previously
opposed discarding s_lock.h on the grounds that it's extremely well
battle-tested, and if we change it, we might introduce subtle bugs
that are dependent on GCC versions and such. But now that compiler
intrinsics are relatively common, I don't really see any reason for us
to provide our own assembler versions of *new* primitives we want to
use. As long as we have some kind of wrapper functions or macros, we
retain the *option* to add workarounds for compiler bugs or lack of
compiler support on platforms, but it seems an awful lot simpler to me
to start by assuming that that the compiler will DTRT, and only roll
our own if that proves to be necessary. It's not like our hand-rolled
implementations couldn't have bugs - which is different from s_lock.h,
where we have evidence that the exact coding in that file does or did
work on those platforms.

Similarly, I would rip out - or separate into a separate patch - the
code that tries to use atomic operations to implement TAS if we don't
already have an implementation in s_lock.h. That might be worth
doing, if there are any common architectures that don't already have
TAS, or to simplify porting to new platforms, but it seems like a
separate thing from what this patch is mainly about, which is enabling
the use of CAS and related operations on platforms that support them.

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