Re: Throwing away unnecessary spin-locks

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