From 93fd6de14156d8d9c8d47e6cbfd067ab2b02e399 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Fri, 7 Oct 2016 16:55:15 -0700 Subject: [PATCH] Fix fallback implementation of pg_atomic_write_u32(). I somehow had assumed that in the spinlock (in turn possibly using semaphores) based fallback atomics implementation 32 bit writes could be done without a lock. As far as the write goes that's correct, since postgres supports only platforms with single-copy atomicity for aligned 32bit writes. But writing without holding the spinlock breaks read-modify-write operations like pg_atomic_compare_exchange_u32(), since they'll potentially "miss" a concurrent write, which can't happen in actual hardware implementations. In 9.6+ when using the fallback atomics implementation this could lead to buffer header locks not being properly marked as released, and potentially some related state corruption. I don't see a related danger in 9.5 (earliest release with the API), because pg_atomic_write_u32() wasn't used in a concurrent manner there. The state variable of local buffers, before this change, were manipulated using pg_atomic_write_u32(), to avoid unnecessary synchronization overhead. As that'd not be the case anymore, introduce and use pg_atomic_unlocked_write_u32(), which does not correctly interact with RMW operations. This bug only caused issues when postgres is compiled on platforms without atomics support (i.e. no common new platform), or when compiled with --disable-atomics, which explains why this wasn't noticed in testing. Reported-By: Tom Lane Discussion: <14947.1475690465@sss.pgh.pa.us> Backpatch: 9.5-, where the atomic operations API was introduced. --- src/backend/port/atomics.c | 13 +++++++++++++ src/include/port/atomics.h | 25 +++++++++++++++++++++++-- src/include/port/atomics/fallback.h | 3 +++ src/include/port/atomics/generic.h | 9 +++++++++ 4 files changed, 48 insertions(+), 2 deletions(-) diff --git a/src/backend/port/atomics.c b/src/backend/port/atomics.c index 439b3c18c7..aa15df5a47 100644 --- a/src/backend/port/atomics.c +++ b/src/backend/port/atomics.c @@ -112,6 +112,19 @@ pg_atomic_init_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val_) ptr->value = val_; } +void +pg_atomic_write_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val) +{ + /* + * One might think that an unlocked write doesn't need to acquire the + * spinlock, but one would be wrong. Even an unlocked write has to cause a + * concurrent pg_atomic_compare_exchange_u32() (et al) to fail. + */ + SpinLockAcquire((slock_t *) &ptr->sema); + ptr->value = val; + SpinLockRelease((slock_t *) &ptr->sema); +} + bool pg_atomic_compare_exchange_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 *expected, uint32 newval) diff --git a/src/include/port/atomics.h b/src/include/port/atomics.h index d94fea60f1..0b506fcbc0 100644 --- a/src/include/port/atomics.h +++ b/src/include/port/atomics.h @@ -297,10 +297,12 @@ pg_atomic_read_u32(volatile pg_atomic_uint32 *ptr) } /* - * pg_atomic_write_u32 - unlocked write to atomic variable. + * pg_atomic_write_u32 - write to atomic variable. * * The write is guaranteed to succeed as a whole, i.e. it's not possible to - * observe a partial write for any reader. + * observe a partial write for any reader. Note that this correctly interacts + * with pg_atomic_compare_exchange_u32, in contrast to + * pg_atomic_unlocked_write_u32(). * * No barrier semantics. */ @@ -312,6 +314,25 @@ pg_atomic_write_u32(volatile pg_atomic_uint32 *ptr, uint32 val) pg_atomic_write_u32_impl(ptr, val); } +/* + * pg_atomic_unlocked_write_u32 - unlocked write to atomic variable. + * + * The write is guaranteed to succeed as a whole, i.e. it's not possible to + * observe a partial write for any reader. But note that writing this way is + * not guaranteed to correctly interact with read-modify-write operations like + * pg_atomic_compare_exchange_u32. This should only be used in cases where + * minor performance regressions due to atomics emulation are unacceptable. + * + * No barrier semantics. + */ +static inline void +pg_atomic_unlocked_write_u32(volatile pg_atomic_uint32 *ptr, uint32 val) +{ + AssertPointerAlignment(ptr, 4); + + pg_atomic_unlocked_write_u32_impl(ptr, val); +} + /* * pg_atomic_exchange_u32 - exchange newval with current value * diff --git a/src/include/port/atomics/fallback.h b/src/include/port/atomics/fallback.h index 4e04f9758b..a1380034ba 100644 --- a/src/include/port/atomics/fallback.h +++ b/src/include/port/atomics/fallback.h @@ -135,6 +135,9 @@ pg_atomic_unlocked_test_flag_impl(volatile pg_atomic_flag *ptr) #define PG_HAVE_ATOMIC_INIT_U32 extern void pg_atomic_init_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val_); +#define PG_HAVE_ATOMIC_WRITE_U32 +extern void pg_atomic_write_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val); + #define PG_HAVE_ATOMIC_COMPARE_EXCHANGE_U32 extern bool pg_atomic_compare_exchange_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 *expected, uint32 newval); diff --git a/src/include/port/atomics/generic.h b/src/include/port/atomics/generic.h index bb31df3623..ec140e4a1e 100644 --- a/src/include/port/atomics/generic.h +++ b/src/include/port/atomics/generic.h @@ -60,6 +60,15 @@ pg_atomic_write_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val) } #endif +#ifndef PG_HAVE_ATOMIC_UNLOCKED_WRITE_U32 +#define PG_HAVE_ATOMIC_UNLOCKED_WRITE_U32 +static inline void +pg_atomic_unlocked_write_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val) +{ + ptr->value = val; +} +#endif + /* * provide fallback for test_and_set using atomic_exchange if available */