Re: Throwing away unnecessary spin-locks

Mutatis mutandis.

At this stage I am happy if it compiles everywhere

On Thu, Oct 1, 2026 at 9:50 PM Alexandre Felipe <
o(dot)alexandre(dot)felipe(at)gmail(dot)com> wrote:

>
>
> On Thu, Oct 1, 2026 at 9:08 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
>> Hi,
>>
>> On 2026-10-01 20:50:35 +0100, Alexandre Felipe wrote:
>> > Please try, if you want
>> > $ grep -A 2 -rn SpinLockAcquire src/backend | grep SpinLockRelease -B 2
>> >
>> > Story
>> > =====
>> > A common pattern that I have been repeatedly warned against is using
>> > spin-locks unnecessarily. I was surprised to see in checkpointer.c
>> > FirstCallSinceLastCheckpoint.
>> >
>> > int new_done;
>> > SpinLockAcquire(&CheckpointerShmem->ckpt_lck);
>> > new_done = CheckpointerShmem->ckpt_done;
>> > SpinLockRelease(&CheckpointerShmem->ckpt_lck);
>> >
>> > I think could certainly be replaced by something like
>> >
>> > + pg_compiler_barrier()
>> > + new_done = CheckpointerShmem->ckpt_done;
>> > + pg_compiler_barrier()
>>
>> That's not a correct transformation. A compiler barrier does not guarantee
>> cache coherency. You actually have to use correct memory barrier
>> pairings. It
>> might suffice to use a read memory barrier in this case, but obviously the
>> easiest transformation is to use full memory barriers on both sides.
>>
>
> Like this (sometimes I miss C++ templates)
>
>
> #define SLOCK_DEFINE_SCALAR_ACCESSORS(typename) \
>
> static inline typename \
>
> slock_read_barrier_##typename(volatile slock_t *lock, volatile typename
> *p) \
>
> { \
>
> typename val; \
>
> \
>
> (void) lock; \
>
> AssertPointerAlignment(p, alignof(typename)); \
>
> val = *p; \
>
> pg_read_barrier(); \
>
> return val; \
>
> } \
>
> static inline void \
>
> slock_write_barrier_##typename(volatile slock_t *lock, volatile typename
> *p, typename v) \
>
> { \
>
> (void) lock; \
>
> AssertPointerAlignment(p, alignof(typename)); \
>
> *p = v; \
>
> pg_write_barrier(); \
>
>
> }
>
> Or on both sides?
>
> The main reason for not having changed all of these over is that you
>> actually
>> have to carefully think about the changes...
>>
>
> Or thinking once about conditions that make them possible and apply some
> transformation.
>
>
>> > Grepping the codebase we get 105 matches, in 26 files.
>> >
>> > One interesting case is xlog.c that uses the lock to protect 64-bit
>> > assignments I saw conversations about pg_atomic_u64 for that, but then
>> in
>> > 32-bit platforms we get this weird 3-field structure everywhere.
>>
>> What are you referring to with "weird 3-field structure"?
>>
>
>
> typedef struct pg_atomic_uint64
> {
> int sema;
> volatile uint64 value;
> } pg_atomic_uint64;
>
> Sorry it is not declared as 3 field, I was thinking
> struct {int sema; volatile uint32 v1; volatile uint32 v2; }
>
> Regards,
> Alexandre
>