Re: better atomics - v0.5 - Mailing list pgsql-hackers

Hi,

On 2014-06-25 15:54:37 -0400, Robert Haas wrote:
> - The new argument to s_init_lock_sema() isn't used.

It's used when atomics fallback to spinlocks which fall back to
semaphores. c.f. atomics.c.

Since it better be legal to manipulate a atomic variable while holding a
spinlock we cannot simply use an arbitrary spinlock as backing for
atomics. That'd possibly cause us to wait on ourselves or cause
deadlocks.

> - Are the regression tests in regress.c likely to catch anything?
> Really?

They already cought several bugs. If the operations were immediately
widely used they probably wouldn't be necessary.

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

Uh. that's a rebase screweup. Should entirely be gone.

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

Hm. Good point. It should protect against that.

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

I don't mind moving it somewhere else and it's easy enough to change.

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

We could forgo it for pg_read_barrier() et al, but for the actual
atomics it's useful because it allows to put common checks in the !impl
routines.

> - Should SpinlockSemaInit() be assigning SpinlockSemas() to a local
> variable instead of re-executing it every time?  Granted, the compiler
> might inline this somehow, but...

I sure hope it will, but still a good idea.

> More generally, I'm really wondering if you're making the
> compare-and-swap implementation a lot more complicated than it needs
> to be.

Possible.

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

The discussion so far seemed pretty clear that we can't regress somewhat
frequently used platforms. And dropping support for all but msvc and gcc
would end up doing that. We're going to have to do the legword for the
most common platforms... Note that I didn't use assembler for those, but
relied on intrinsics...

> I suspect this patch could be about 20% of its current size and give
> us everything that we need.

I think the only way to do that would be to create a s_lock.h like
maze. Which imo is a utterly horrible idea. I'm pretty sure there'll be
more assembler implementations for 9.5 and unless we think ahead of how
to structure that we'll create something bad.
I also think that a year or so down the road we're going to support more
operations. E.g. optional support for a 16byte CAS can allow for some
awesome things when you want to do lockless stuff on a 64bit platform.

I think there's chances for reducing the size by merging i386/amd64,
that looked better earlier than it does now (due to the addition of the
inline assembler).

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

I think we should entirely get rid of s_lock.h. It's an unmaintainable
mess. That's what I'd done in an earlier version of the patch, but you'd
argued against it. I think you're right in that it shouldn't be done in
the same patch and possibly not even in the same cycle, but I think we
better do it sometime not too far away.
I e.g. don't see how we can guarantee sane barrier semantics with the
current implementation.

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

I added the x86 inline assembler because a fair number of buildfarm
animals use ancient gcc's and because I could easily test it. It's also
more efficient for gcc < 4.6. I'm not wedded to keeping it.

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

Happy to separate that out. It's also very useful to test code btw...

Greetings,

Andres Freund

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