MIME-Version: 1.0
References: 
 <CAPpHfdudejddHLLw=Uk_7pZ=1g2SZXfwubSgmWUhJ7fBUtR9rg@mail.gmail.com>
 <2muwyx6a5vojkg7iegknhnkcch3lfxptsxk7icwuh7szkvvu2y@vc3ukkfvnu6i>
In-Reply-To: <2muwyx6a5vojkg7iegknhnkcch3lfxptsxk7icwuh7szkvvu2y@vc3ukkfvnu6i>
From: Alexander Korotkov <aekorotkov@gmail.com>
Date: Fri, 7 Mar 2025 18:39:42 +0200
Message-ID: 
 <CAPpHfdvucbQ9w7_eJYefUUp6w-WZUes+SRfq+GKfhukhs7kLqg@mail.gmail.com>
Subject: Re: pg_atomic_compare_exchange_*() and memory barriers
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAPpHfdvucbQ9w7_eJYefUUp6w-WZUes%2BSRfq%2BGKfhukhs7kLqg%40mail.gmail.com>
Precedence: bulk

Hi, Andres.

Thank you for reply.

On Fri, Mar 7, 2025 at 6:02=E2=80=AFPM Andres Freund <andres@anarazel.de> w=
rote:
>
> On 2025-03-07 17:47:08 +0200, Alexander Korotkov wrote:
> > While investigating a bug in the patch to get rid of WALBufMappingLock,=
 I
> > found that the surrounding pg_atomic_compare_exchange_u64() fixes the
> > problem for me.
>
> That sounds more likely to be due to slowing down things enough to make a=
 race
> less likely to be hit. Or a compiler bug.
>
>
> > That doesn't feel right because, according to the
> > comments, both pg_atomic_compare_exchange_u32() and
> > pg_atomic_compare_exchange_u64() should provide full memory barrier
> > semantics.  So, I decided to investigate this further.
> >
> > In my case, the implementation of pg_atomic_compare_exchange_u64() is b=
ased
> > on __atomic_compare_exchange_n().
> >
> > static inline bool
> > pg_atomic_compare_exchange_u64_impl(volatile pg_atomic_uint64 *ptr,
> >                                     uint64 *expected, uint64 newval)
> > {
> >     AssertPointerAlignment(expected, 8);
> >     return __atomic_compare_exchange_n(&ptr->value, expected, newval, f=
alse,
> >                                        __ATOMIC_SEQ_CST, __ATOMIC_SEQ_C=
ST);
> > }
> >
> > According to the docs __ATOMIC_SEQ_CST enforces total ordering with *al=
l
> > other __ATOMIC_SEQ_CST operations*.  It only says about other
> > __ATOMIC_SEQ_CST operations; nothing is said about regular reads/writes=
.
> > This sounds quite far from our comment, promising full barrier semantic=
s,
> > which, in my understanding, means the equivalent of
> > both pg_read_barrier()/pg_write_barrier(), which should barrier *all ot=
her
> > read/writes*.
>
> A memory barrier in one process/thread always needs be paired with a barr=
ier
> in another process/thread. It's not enough to have a memory barrier on on=
e
> side but not the other.

Yes, there surely should be a memory barrier on another side.  But
does __ATOMIC_SEQ_CST works as a barrier for the regular read/write
operations on the same side?

> __ATOMIC_SEQ_CST is used to implement the C11/C++11 barriers, and for tho=
se
> it's more clearly formulated that that include acquire/release semantics.=
  The
> standard formulation is a bit more complicated IIRC, but here's the
> cppreference.com simplification:
>
> https://en.cppreference.com/w/c/atomic/memory_order
> >  A load operation with this memory order performs an acquire operation,=
 a
> >  store performs a release operation, and read-modify-write performs bot=
h an
> >  acquire operation and a release operation, plus a single total order e=
xists
> >  in which all threads observe all modifications in the same order (see
> >  Sequentially-consistent ordering below).


Thank you.  This is not yet clear for me.  I probably need to meditate
more on this :)

>
> > This function first checks if LSE instructions are present.  If so,
> > instruction LSE casal is used.  If not (in my case), the loop of
> > ldaxr/stlxr is used instead.  The documentation says both ldaxr/stlxr
> > provides one-way barriers.  Read/writes after ldaxr will be observed af=
ter,
> > and read/writes before stlxr will be observed before.  So, a pair of th=
ese
> > instructions provides a full memory barrier.  However, if we don't obse=
rve
> > the expected value, only ldaxr gets executed.  So, an unsuccessful
> > pg_atomic_compare_exchange_u64() attempt will leave us with a one-way
> > barrier, and that caused a bug in my case.
>
> That has to be a compiler bug.  We specify __ATOMIC_SEQ_CST for both
> success_memorder *and* failure_memorder.
>
> What compiler & version is this?

I've tried and got the same results with two compilers.
gcc (Ubuntu 13.3.0-6ubuntu2~24.04) 13.3.0
Ubuntu clang version 19.1.1 (1ubuntu1~24.04.2)

------
Regards,
Alexander Korotkov
Supabase