Mutatis mutandis. At this stage I am happy if it compiles everywhere
On Thu, Oct 1, 2026 at 9:50 PM Alexandre Felipe < [email protected]> wrote: > > > On Thu, Oct 1, 2026 at 9:08 PM Andres Freund <[email protected]> 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 >
v1-0001-Spin-lock-protected-scalar-accessors.patch
Description: Binary data
v1-0002-replace-slock_-read-write-_.patch
Description: Binary data
