From 0743fd27802a9e6e3910957f0b2222dd15fbd00b Mon Sep 17 00:00:00 2001 From: Alexandre Felipe Date: Thu, 8 Oct 2026 20:52:27 +0100 Subject: [PATCH-v1 2/2] Using half-state CAS The state CAS operations are as follows bumgr.c: (low) MarkBufferDirty, PinBuffer, StrategyGetBuffer, freelist.c: (low)GetBufferFromRing, GetStrategyBuffer bufmgr.c: (high) BufferLockAttempt bufmgr.c: (full) SharedBufferBeginSetHintBits UnlockBufHdrExt has three cases 1. pg_atomic_fetch_and_u64 when set_bits = 0 and refcount_change = 0. 2. pg_atomic_compare_exchange_u64 when touching the high part 3. pg_atomic_compare_exchange_io when affecting only the low part. For the case 3 the high bits are not guaranteed to be updated. At the moment the only caller using the return value of UnlockBufHdrExt is TerminateBufferIO, and it only uses BM_PIN_COUNT_WAITER. --- src/backend/storage/buffer/bufmgr.c | 35 ++++++-- src/backend/storage/buffer/freelist.c | 20 ++++- src/include/storage/buf_internals.h | 110 +++++++++++++++++++++++--- 3 files changed, 144 insertions(+), 21 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 5c82865a084..39462aa4378 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -3192,6 +3192,9 @@ MarkBufferDirty(Buffer buffer) * TerminateBufferIO() relies on the spinlock. */ old_buf_state = pg_atomic_read_u64(&bufHdr->state); + + /* The following loop considers only the low half of the state */ + Assert(((BM_DIRTY | BM_LOCKED) & 0xffffffff00000000) == 0); for (;;) { if (old_buf_state & BM_LOCKED) @@ -3202,8 +3205,8 @@ MarkBufferDirty(Buffer buffer) Assert(BUF_STATE_GET_REFCOUNT(buf_state) > 0); buf_state |= BM_DIRTY; - if (pg_atomic_compare_exchange_u64(&bufHdr->state, &old_buf_state, - buf_state)) + if (pg_atomic_compare_exchange_u64_lo(&bufHdr->state, &old_buf_state, + buf_state)) break; } @@ -3310,6 +3313,14 @@ PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy, uint64 old_buf_state; old_buf_state = pg_atomic_read_u64(&buf->state); + + /* The following loop considers only the low-half of the state */ + Assert((( + BM_VALID | + BM_LOCKED | + BUF_REFCOUNT_MASK | + BUF_USAGECOUNT_MASK + ) & 0xffffffff00000000) == 0); for (;;) { if (unlikely(skip_if_not_valid && !(old_buf_state & BM_VALID))) @@ -3348,7 +3359,7 @@ PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy, buf_state += BUF_USAGECOUNT_ONE; } - if (pg_atomic_compare_exchange_u64(&buf->state, &old_buf_state, + if (pg_atomic_compare_exchange_u64_lo(&buf->state, &old_buf_state, buf_state)) { result = (buf_state & BM_VALID) != 0; @@ -6126,7 +6137,14 @@ BufferLockAttempt(BufferDesc *buf_hdr, BufferLockMode mode) */ old_state = pg_atomic_read_u64(&buf_hdr->state); - /* loop until we've determined whether we could acquire the lock or not */ + /* loop until we've determined whether we could acquire the lock or not. + * This loop considers only half of the state */ + Assert((( + BM_LOCK_VAL_EXCLUSIVE | + BM_LOCK_VAL_SHARED | + BM_LOCK_VAL_SHARE_EXCLUSIVE + ) & 0x00000000ffffffff) == 0 + ); while (true) { uint64 desired_state; @@ -6163,8 +6181,8 @@ BufferLockAttempt(BufferDesc *buf_hdr, BufferLockMode mode) * * Retry if the value changed since we last looked at it. */ - if (likely(pg_atomic_compare_exchange_u64(&buf_hdr->state, - &old_state, desired_state))) + if (likely(pg_atomic_compare_exchange_u64_hi(&buf_hdr->state, + &old_state, desired_state))) { if (lock_free) { @@ -7047,6 +7065,11 @@ SharedBufferBeginSetHintBits(Buffer buffer, BufferDesc *buf_hdr, uint64 *locksta /* new lock level */ desired_state += BM_LOCK_VAL_SHARE_EXCLUSIVE; + /* + * XXX: We could use pg_atomic_compare_exchange_u64_hi here. + * However, doesn't update BM_DIRTY and BM_PERMANENT, used by + * one of the callers. + */ if (likely(pg_atomic_compare_exchange_u64(&buf_hdr->state, &old_state, desired_state))) { diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c index fdb5bad7910..baab0093210 100644 --- a/src/backend/storage/buffer/freelist.c +++ b/src/backend/storage/buffer/freelist.c @@ -238,6 +238,12 @@ StrategyGetBuffer(BufferAccessStrategy strategy, uint64 *buf_state, bool *from_r /* Use the "clock sweep" algorithm to find a free buffer */ trycounter = NBuffers; + /* This loop only considers the low-half of the state */ + Assert((( + BM_LOCKED | + BUF_REFCOUNT_MASK | + BUF_USAGECOUNT_MASK + ) & 0xffffffff00000000) == 0); for (;;) { uint64 old_buf_state; @@ -287,7 +293,7 @@ StrategyGetBuffer(BufferAccessStrategy strategy, uint64 *buf_state, bool *from_r { local_buf_state -= BUF_USAGECOUNT_ONE; - if (pg_atomic_compare_exchange_u64(&buf->state, &old_buf_state, + if (pg_atomic_compare_exchange_u64_lo(&buf->state, &old_buf_state, local_buf_state)) { trycounter = NBuffers; @@ -299,7 +305,7 @@ StrategyGetBuffer(BufferAccessStrategy strategy, uint64 *buf_state, bool *from_r /* pin the buffer if the CAS succeeds */ local_buf_state += BUF_REFCOUNT_ONE; - if (pg_atomic_compare_exchange_u64(&buf->state, &old_buf_state, + if (pg_atomic_compare_exchange_u64_lo(&buf->state, &old_buf_state, local_buf_state)) { /* Found a usable buffer */ @@ -645,8 +651,14 @@ GetBufferFromRing(BufferAccessStrategy strategy, uint64 *buf_state) /* * Check whether the buffer can be used and pin it if so. Do this using a - * CAS loop, to avoid having to lock the buffer header. + * CAS loop on the low-half of the state, to avoid having to lock the + * buffer header. */ + Assert((( + BM_LOCKED | + BUF_REFCOUNT_MASK | + BUF_USAGECOUNT_MASK + ) & 0xffffffff00000000) == 0); old_buf_state = pg_atomic_read_u64(&buf->state); for (;;) { @@ -675,7 +687,7 @@ GetBufferFromRing(BufferAccessStrategy strategy, uint64 *buf_state) /* pin the buffer if the CAS succeeds */ local_buf_state += BUF_REFCOUNT_ONE; - if (pg_atomic_compare_exchange_u64(&buf->state, &old_buf_state, + if (pg_atomic_compare_exchange_u64_lo(&buf->state, &old_buf_state, local_buf_state)) { *buf_state = local_buf_state; diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h index e4ff5619b79..500fe6c8d2c 100644 --- a/src/include/storage/buf_internals.h +++ b/src/include/storage/buf_internals.h @@ -31,18 +31,79 @@ #include "utils/resowner.h" /* - * Buffer state is a single 64-bit variable where following data is combined. + * Buffer state is a single 64-bit variable, [63:0], where the following + * data is combined. Bit 0 is the least significant bit. Bits [63:54] are + * unused. * * State of the buffer itself (in order): - * - 18 bits refcount - * - 4 bits usage count - * - 12 bits of flags - * - 18 bits share-lock count - * - 1 bit share-exclusive locked - * - 1 bit exclusive locked + * ------------------ LOW HALF ------------------- + * [17: 0] refcount (18 bits) + * [21:18] usage count ( 4 bits) + * [33:22] flags (12 bits) + * [ 22] BM_LOCKED + * [ 23] BM_DIRTY + * [ 24] BM_VALID + * [ 25] BM_TAG_VALID + * [ 26] BM_IO_IN_PROGRESS + * [ 27] BM_IO_ERROR + * [ 28] unused + * [ 29] BM_PIN_COUNT_WAITER + * [ 30] BM_CHECKPOINT_NEEDED + * [ 31] BM_PERMANENT + * ------------------ HIGH HALF ------------------ + * [ 32] BM_LOCK_HAS_WAITERS + * [ 33] BM_LOCK_WAKE_IN_PROGRESS + * [51:34] share-lock count (18 bits) + * [ 52] share-exclusive locked ( 1 bit) + * [ 53] exclusive locked ( 1 bit) + * [63:54] unused (10 bits) * * Combining these values allows to perform some operations without locking - * the buffer header, by modifying them together with a CAS loop. + * the buffer header, by modifying them together with a CAS loop. A 32-bit + * atomic is enough when every bit of the update sits in one half. An + * update that touches both halves needs a 64-bit atomic. + * + * 32-bit atomic, low half [31:0]: + * + * - Pinning: raise the refcount [17:0] and, when appropriate, the usage + * count [21:18], but only while BM_LOCKED [22] is clear (PinBuffer). + * Seeing the header unlocked and raising the refcount is one step, so a + * buffer is not pinned while its tag is being replaced. + * + * - Unpinning: subtract from the refcount [17:0] even while the header lock + * [22] is held, and observe BM_PIN_COUNT_WAITER [29] in that same value, + * so the last other pin can wake a cleanup-lock waiter (UnpinBuffer). + * + * - Clock-sweep victim selection and ring-buffer reuse: if the refcount + * [17:0] is zero (and, for a ring buffer, the usage count [21:18] is at + * most one) and the header is unlocked [22], either pin the buffer, + * setting refcount [17:0] to 1 if usagecount [21:18] is 0, or decrease + * usagecount [21:18] (StrategyGetBuffer, GetBufferFromRing). + * + * - Setting BM_DIRTY [23], retrying while BM_LOCKED [22] is set, so the + * flag is not set across TerminateBufferIO clearing it (MarkBufferDirty). + * + * - Publishing BM_PIN_COUNT_WAITER [29] with an atomic OR while the header + * lock [22] is held, which leaves a concurrent refcount [17:0] decrement + * intact (LockBufferForCleanup). + * + * - Finishing I/O: clear BM_IO_IN_PROGRESS [26], possibly BM_DIRTY [23], + * and drop the I/O pin [17:0] together (TerminateBufferIO). + * + * 32-bit atomic, high half [63:32]: + * + * - Acquiring, upgrading a share lock [51:34] to share-exclusive [52], and + * releasing the content lock [51:34], [52], or [53] (BufferLockAttempt, + * SharedBufferBeginSetHintBits, BufferLockUnlock). + * + * 64-bit atomic, both halves: + * + * - Releasing the content lock ([51:34], [52], or [53]) and the pin [17:0] + * in one subtraction (UnlockReleaseBuffer). + * + * - Releasing the header lock [22] while setting or clearing flags [33:22] + * and adjusting the refcount [17:0] (UnlockBufHdrExt). Flags [33:32] + * are the high-half bits of that field. * * The definition of buffer state components is below. */ @@ -472,12 +533,31 @@ UnlockBufHdr(BufferDesc *desc) * * Note that this approach would not trivially work for usagecount, since we * need to cap the usagecount at BM_MAX_USAGE_COUNT. + * + * When set_bits and unset_bits are both in the low half, this is a 32-bit + * CAS of bits [31:0]. BM_LOCKED and the refcount are in that half too. The + * returned high half is then whatever old_buf_state already held, which can + * lag a concurrent content-lock update. */ static inline uint64 UnlockBufHdrExt(BufferDesc *desc, uint64 old_buf_state, uint64 set_bits, uint64 unset_bits, int refcount_change) { + bool low_only; + + if (set_bits == 0 && refcount_change == 0) + { + /* + * No bits are being set and the refcount stays unchanged. + * The update can be performed with an atomic AND. + */ + return pg_atomic_fetch_and_u64(&desc->state, ~(unset_bits | BM_LOCKED)); + } + + /* Refcount and BM_LOCKED are in [31:0]. Flags [33:32] are not. */ + low_only = ((set_bits | unset_bits) >> 32) == 0; + for (;;) { uint64 buf_state = old_buf_state; @@ -495,11 +575,19 @@ UnlockBufHdrExt(BufferDesc *desc, uint64 old_buf_state, if (refcount_change != 0) buf_state += BUF_REFCOUNT_ONE * refcount_change; - if (pg_atomic_compare_exchange_u64(&desc->state, &old_buf_state, - buf_state)) + if (low_only) { - return old_buf_state; + /* + * The update hits only the low half, compare exchange + * can ignore the high half. + */ + if (pg_atomic_compare_exchange_u64_lo(&desc->state, &old_buf_state, + buf_state)) + return old_buf_state; } + else if (pg_atomic_compare_exchange_u64(&desc->state, &old_buf_state, + buf_state)) + return old_buf_state; } } -- 2.53.0