Feedback-ID: id4a34324:Fastmail
Date: Fri, 7 Mar 2025 11:02:33 -0500
From: Andres Freund <andres@anarazel.de>
To: Alexander Korotkov <aekorotkov@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: pg_atomic_compare_exchange_*() and memory barriers
Message-ID: <2muwyx6a5vojkg7iegknhnkcch3lfxptsxk7icwuh7szkvvu2y@vc3ukkfvnu6i>
References: 
 <CAPpHfdudejddHLLw=Uk_7pZ=1g2SZXfwubSgmWUhJ7fBUtR9rg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAPpHfdudejddHLLw=Uk_7pZ=1g2SZXfwubSgmWUhJ7fBUtR9rg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/2muwyx6a5vojkg7iegknhnkcch3lfxptsxk7icwuh7szkvvu2y%40vc3ukkfvnu6i>
Precedence: bulk

Hi,

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 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, newval, false,
>                                        __ATOMIC_SEQ_CST, __ATOMIC_SEQ_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/writes.
> This sounds quite far from our comment, promising full barrier semantics,
> which, in my understanding, means the equivalent of
> both pg_read_barrier()/pg_write_barrier(), which should barrier *all 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 on one
side but not the other.

__ATOMIC_SEQ_CST is used to implement the C11/C++11 barriers, and for those
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 both an
>  acquire operation and a release operation, plus a single total order exists
>  in which all threads observe all modifications in the same order (see
>  Sequentially-consistent ordering below).


>
> 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 after,
> and read/writes before stlxr will be observed before.  So, a pair of 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-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 have a couple of ideas on how to fix this problem.
> 1. Provide full barrier semantics for pg_atomic_compare_exchange_*().  In
> particular, for __atomic_compare_exchange_n(), we should probably
> use __ATOMIC_ACQ_REL for success and run an explicit memory barrier in the
> case of failure.

I don't follow - __ATOMIC_SEQ_CST is *stronger* than __ATOMIC_ACQ_REL.

Greetings,

Andres Freund