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

On 2014-06-25 20:16:08 -0400, Robert Haas wrote:
> On Wed, Jun 25, 2014 at 4:36 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> > 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.
> 
> I think that's going to fall afoul of Tom's previously-articulated "no
> loops inside spinlocks" rule.  Most atomics, by nature, are
> loop-until-it-works.

Well, so is TAS itself :).

More seriously, I think we're not going to have much fun if we're making
up the rule that you can't do an atomic add/sub while a spinlock is
held. That just precludes to many use cases and will make the code much
harder to understand. I don't think we're going to end up having many
problems if we allow atomic read/add/sub/write in there.

> >> 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...
> 
> We can't *regress* somewhat-frequently used platforms, but that's
> different from saying we've got to support *new* facilities on those
> platforms.

Well, we want to use the new facilities somewhere. And it's not entirely
unfair to call it a regression if other os/compiler/arch combinations
speed up but yours don't.

> Essentially the entire buildfarm is running either gcc,
> some Microsoft compiler, or a compiler that's supports the same
> atomics intrinsics as gcc i.e. icc or clang.  Some of those compilers
> may be too old to support the atomics intrinsics, and there's one Sun
> Studio animal, but I don't know that we need to care about those
> things in this patch...

I think we should support those where it's easy and where we'll see the
breakage on the buildfarm. Sun studio's intrinsics are simple enough
that I don't think my usage of them will be too hard to fix.

> ...unless of course the atomics fallbacks in this patch are
> sufficiently sucky that anyone who ends up using those is going to be
> sad.  Then the bar is a lot higher.  But if that's the case then I
> wonder if we're really on the right course here.

I don't you'll notice it much on mostly nonconcurrent uses. But some of
those architectures/platforms do support larger NUMA like setups. And
for those it'd probably hurt for some workloads. And e.g. solaris is
still fairly common for such setups.

> > 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.
> 
> Hmm. gcc 4.6 is only just over a year old, so if pre-4.6
> implementations aren't that good, that's a pretty good argument for
> keeping our own implementations around.  :-(

The difference isn't that big. It's
return __atomic_compare_exchange_n(&ptr->value, expected, newval,                                  false,
__ATOMIC_SEQ_CST,__ATOMIC_SEQ_CST);
vs

bool    ret;
uint64  current;
current = __sync_val_compare_and_swap(&ptr->value, *expected, newval);
ret = current == *expected;
*expected = current;
return ret;

On x86 that's an additional cmp, mov and such. Useless pos because
cmpxchg already computes all that... (that's returned by the setz in my
patch). MSVC only supports the second form as well.

There's a fair number of < 4.2 gccs on the buildfarm as well though.

Greetings,

Andres Freund

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