Re: Problem with pg_atomic_compare_exchange_u64 at 32-bit platforms

Hi,

On 2020-05-20 10:32:18 +0300, Konstantin Knizhnik wrote:
> On 20.05.2020 08:10, Andres Freund wrote:
> > On May 19, 2020 8:05:00 PM PDT, Noah Misch <noah@leadboat.com> wrote:
> > > On Tue, May 19, 2020 at 04:07:29PM +0300, Konstantin Knizhnik wrote:
> > > > Definition of pg_atomic_compare_exchange_u64 requires alignment of
> > > expected
> > > > pointer on 8-byte boundary.
> > > >
> > > > pg_atomic_compare_exchange_u64(volatile pg_atomic_uint64 *ptr,
> > > >                                 uint64 *expected, uint64 newval)
> > > > {
> > > > #ifndef PG_HAVE_ATOMIC_U64_SIMULATION
> > > >      AssertPointerAlignment(ptr, 8);
> > > >      AssertPointerAlignment(expected, 8);
> > > > #endif
> > > >
> > > >
> > > > I wonder if there are platforms  where such restriction is actually
> > > needed.
> > >
> > > In general, sparc Linux does SIGBUS on unaligned access.  Other
> > > platforms
> > > function but suffer performance penalties.
> > Indeed. Cross cacheline atomics are e.g. really expensive on x86. Essentially requiring a full blown bus lock
iirc.
> >
> Please notice that here we talk about alignment not of atomic pointer
> itself, but of pointer to the expected value.

That wasn't particularly clear in your first email... In hindsight I
can see that you meant that, but I'm not surprised to not have
understood that the on the first read either.


> At Intel CMPXCHG instruction read and write expected value throw AX
> register.
> So alignment of pointer to expected value in pg_atomic_compare_exchange_u64
> is not needed in this case.

x86 also supports doing a CMPXCHG crossing a cacheline boundary, it's
just terrifyingly expensive...


I can imagine this being a problem on a 32bit platforms, but on 64bit
platforms, it seems only an insane platform ABI would only have 4 byte
alignment on 64bit integers. That'd cause so much unnecessarily split
cachlines... That's separate from the ISA actually supporting doing such
reads efficiently, of course.


But that still leaves the alignment check on expected to be too strong
on 32 bit platforms where 64bit alignment is only 4 bytes. I'm doubtful
that's it's a good idea to use a comparison value potentially split
across cachelines for an atomic operation that's potentially
contended. But also, I don't particularly care about 32 bit performance.

I think we should probably just drop the second assert down to
ALIGNOF_INT64. Would require a new configure stanza, but that's easy
enough to do. It's just adding
AC_CHECK_ALIGNOF(PG_INT64_TYPE)

Doing that change made me think about replace the conditional long long
int alignof logic in configure.in, and just unconditionally do the a
check for PG_INT64_TYPE, seems nicer. That made me look at Solution.pm
due to ALIGNOF_LONG_LONG, and it's interesting:
    # Every symbol in pg_config.h.in must be accounted for here.  Set
    # to undef if the symbol should not be defined.
    my %define = (
...
        ALIGNOF_LONG_LONG_INT      => 8,
...
        PG_INT64_TYPE       => 'long long int',

so currently our msvc build actually claims that the alignment
requirements are what the code tests. And that's not just since
pg_config.h is autogenerated, it was that way before too:

/* The alignment requirement of a `long long int'. */
#define ALIGNOF_LONG_LONG_INT 8
/* Define to the name of a signed 64-bit integer type. */
#define PG_INT64_TYPE long long int

and has been for a while.


> And my question was whether there are some platforms where implementation of
> compare-exchange 64-bit primitive
> requires stronger alignment of "expected" pointer than one enforced by
> original alignment rules for this platform.

IIRC there's a few older platforms that have single-copy-atomicity for 8
byte values, but do *not* have it for ones not aligned to 8 byte
platforms. Despite not having such an ABI alignment.

It's not impossible to come up with a case where that could matter (if
expected pointed into some shared memory that could be read by others),
but it's hard to take them serious.

Greetings,

Andres Freund