MIME-Version: 1.0
References: 
 <CAPpHfdudejddHLLw=Uk_7pZ=1g2SZXfwubSgmWUhJ7fBUtR9rg@mail.gmail.com>
 <2muwyx6a5vojkg7iegknhnkcch3lfxptsxk7icwuh7szkvvu2y@vc3ukkfvnu6i>
 <CAPpHfdvucbQ9w7_eJYefUUp6w-WZUes+SRfq+GKfhukhs7kLqg@mail.gmail.com>
 <oc4iicdkwyhdf5o5vbwsl7jdlqnds37xtf27wuxvhy3abxoo6i@4ek3xp5j6niy>
In-Reply-To: <oc4iicdkwyhdf5o5vbwsl7jdlqnds37xtf27wuxvhy3abxoo6i@4ek3xp5j6niy>
From: Alexander Korotkov <aekorotkov@gmail.com>
Date: Fri, 7 Mar 2025 19:45:55 +0200
Message-ID: 
 <CAPpHfdszGqwPc0tG7xy8oQ937PfTGD6mFTjgEdrMvy2f7HSzWw@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/CAPpHfdszGqwPc0tG7xy8oQ937PfTGD6mFTjgEdrMvy2f7HSzWw%40mail.gmail.com>
Precedence: bulk

On Fri, Mar 7, 2025 at 7:07=E2=80=AFPM Andres Freund <andres@anarazel.de> w=
rote:
> On 2025-03-07 18:39:42 +0200, Alexander Korotkov wrote:
> > On Fri, Mar 7, 2025 at 6:02=E2=80=AFPM Andres Freund <andres@anarazel.d=
e> wrote:
> > >
> > > On 2025-03-07 17:47:08 +0200, Alexander Korotkov wrote:
> > > > While investigating a bug in the patch to get rid of WALBufMappingL=
ock, I
> > > > found that the surrounding pg_atomic_compare_exchange_u64() fixes t=
he
> > > > problem for me.
> > >
> > > That sounds more likely to be due to slowing down things enough to ma=
ke 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 based
> > > > 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, newva=
l, false,
> > > >                                        __ATOMIC_SEQ_CST, __ATOMIC_S=
EQ_CST);
> > > > }
> > > >
> > > > According to the docs __ATOMIC_SEQ_CST enforces total ordering with=
 *all
> > > > other __ATOMIC_SEQ_CST operations*.  It only says about other
> > > > __ATOMIC_SEQ_CST operations; nothing is said about regular reads/wr=
ites.
> > > > This sounds quite far from our comment, promising full barrier sema=
ntics,
> > > > which, in my understanding, means the equivalent of
> > > > both pg_read_barrier()/pg_write_barrier(), which should barrier *al=
l other
> > > > read/writes*.
> > >
> > > A memory barrier in one process/thread always needs be paired with a =
barrier
> > > in another process/thread. It's not enough to have a memory barrier o=
n one
> > > 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?
>
> Yes, if it's paired with another barrier on the other side. The regular
> read/write operations are basically protected transitively, due to
>
> a) An acquire barrier preventing non-atomic reads/writes in the same thre=
ad
>    from appearing to have been moved to before the barrier
>
> b) A release barrier preventing non-atomic reads/writes in the same threa=
d
>    from appearing to have been moved to after the barrier
>
> c) The other thread being guaranteed a) and b) for the other threads'
>    non-atomic reads/writes depending on the type of barrier
>
> d) The atomic value itself being guaranteed to be, well, atomic
>
>
> > > > 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/stl=
xr
> > > > provides one-way barriers.  Read/writes after ldaxr will be observe=
d after,
> > > > and read/writes before stlxr will be observed before.  So, a pair o=
f these
> > > > instructions provides a full memory barrier.  However, if we don't =
observe
> > > > the expected value, only ldaxr gets executed.  So, an unsuccessful
> > > > pg_atomic_compare_exchange_u64() attempt will leave us with a one-w=
ay
> > > > 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)
>
> Thinking more about it I wonder if the behaviour of not doing a release i=
n
> case the atomic fails *might* arguably actually be correct - if the
> compare-exchange fails, there's nothing that the non-atomic values could =
be
> ordered against.

There is another successful compare-exchange executed by a concurrent
process to get ordered against.

------
Regards,
Alexander Korotkov
Supabase