On Fri, Feb 23, 2024 at 05:34:49PM -0800, Andres Freund wrote: > On 2024-02-23 14:58:12 -0600, Nathan Bossart wrote: >> +/* >> + * pg_atomic_write_membarrier_u32 - write with barrier semantics. >> + * >> + * The write is guaranteed to succeed as a whole, i.e., it's not possible to >> + * observe a partial write for any reader. Note that this correctly >> interacts >> + * with both pg_atomic_compare_exchange_u32() and >> + * pg_atomic_read_membarrier_u32(). While this may be less performant than >> + * pg_atomic_write_u32() and pg_atomic_unlocked_write_u32(), it may be >> easier >> + * to reason about correctness with this function in less >> performance-sensitive >> + * code. >> + * >> + * Full barrier semantics. >> + */ > > The callout to pg_atomic_unlocked_write_u32() is wrong. The reason to use > pg_atomic_unlocked_write_u32() is for variables where we do not ever want to > fall back to spinlocks/semaphores, because the underlying variable isn't > actually shared. In those cases using the other variants is a bug. The only > use of pg_atomic_unlocked_write_u32() is temp-table buffers which share the > data structure with the shared buffers case.
I removed the reference to pg_atomic_unlocked_write_u32(). -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
>From 647eea91cc07e2d6a4d6634bcb933df245711fa0 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nat...@postgresql.org> Date: Fri, 23 Feb 2024 14:23:15 -0600 Subject: [PATCH v4 1/2] Introduce atomic read/write functions with full barrier semantics. Writing correct code using atomic variables is often difficult due to the implied memory barrier semantics (or lack thereof) of the underlying operations. This commit introduces atomic read/write functions with full barrier semantics to ease this cognitive load in areas that are not performance critical. For example, some spinlocks protect a single value, and these new functions make it easy to convert the value to an atomic variable (thus eliminating the need for the spinlock) without modifying the barrier semantics previously provided by the spinlock. However, since these functions are less performant than the other atomic reads/writes, they are not suitable for every use-case. The base implementations for these new functions are atomic exchanges (for writes) and atomic fetch/adds with 0 (for reads). These implementations can be overwritten with better architecture- specific versions as they are discovered. This commit leaves converting existing code to use these new functions as a future exercise. Reviewed-by: Andres Freund, Yong Li, Jeff Davis Discussion: https://postgr.es/me/20231110205128.GB1315705%40nathanxps13 --- src/include/port/atomics.h | 58 ++++++++++++++++++++++++++++++ src/include/port/atomics/generic.h | 36 +++++++++++++++++++ 2 files changed, 94 insertions(+) diff --git a/src/include/port/atomics.h b/src/include/port/atomics.h index bf151037f7..c3b14440cf 100644 --- a/src/include/port/atomics.h +++ b/src/include/port/atomics.h @@ -239,6 +239,26 @@ pg_atomic_read_u32(volatile pg_atomic_uint32 *ptr) return pg_atomic_read_u32_impl(ptr); } +/* + * pg_atomic_read_membarrier_u32 - read with barrier semantics. + * + * This read is guaranteed to return the current value, provided that the value + * is only ever updated via operations with barrier semantics, such as + * pg_atomic_compare_exchange_u32() and pg_atomic_write_membarrier_u32(). + * While this may be less performant than pg_atomic_read_u32(), it may be + * easier to reason about correctness with this function in less performance- + * sensitive code. + * + * Full barrier semantics. + */ +static inline uint32 +pg_atomic_read_membarrier_u32(volatile pg_atomic_uint32 *ptr) +{ + AssertPointerAlignment(ptr, 4); + + return pg_atomic_read_membarrier_u32_impl(ptr); +} + /* * pg_atomic_write_u32 - write to atomic variable. * @@ -276,6 +296,26 @@ pg_atomic_unlocked_write_u32(volatile pg_atomic_uint32 *ptr, uint32 val) pg_atomic_unlocked_write_u32_impl(ptr, val); } +/* + * pg_atomic_write_membarrier_u32 - write with barrier semantics. + * + * The write is guaranteed to succeed as a whole, i.e., it's not possible to + * observe a partial write for any reader. Note that this correctly interacts + * with both pg_atomic_compare_exchange_u32() and + * pg_atomic_read_membarrier_u32(). While this may be less performant than + * pg_atomic_write_u32(), it may be easier to reason about correctness with + * this function in less performance-sensitive code. + * + * Full barrier semantics. + */ +static inline void +pg_atomic_write_membarrier_u32(volatile pg_atomic_uint32 *ptr, uint32 val) +{ + AssertPointerAlignment(ptr, 4); + + pg_atomic_write_membarrier_u32_impl(ptr, val); +} + /* * pg_atomic_exchange_u32 - exchange newval with current value * @@ -429,6 +469,15 @@ pg_atomic_read_u64(volatile pg_atomic_uint64 *ptr) return pg_atomic_read_u64_impl(ptr); } +static inline uint64 +pg_atomic_read_membarrier_u64(volatile pg_atomic_uint64 *ptr) +{ +#ifndef PG_HAVE_ATOMIC_U64_SIMULATION + AssertPointerAlignment(ptr, 8); +#endif + return pg_atomic_read_membarrier_u64_impl(ptr); +} + static inline void pg_atomic_write_u64(volatile pg_atomic_uint64 *ptr, uint64 val) { @@ -438,6 +487,15 @@ pg_atomic_write_u64(volatile pg_atomic_uint64 *ptr, uint64 val) pg_atomic_write_u64_impl(ptr, val); } +static inline void +pg_atomic_write_membarrier_u64(volatile pg_atomic_uint64 *ptr, uint64 val) +{ +#ifndef PG_HAVE_ATOMIC_U64_SIMULATION + AssertPointerAlignment(ptr, 8); +#endif + pg_atomic_write_membarrier_u64_impl(ptr, val); +} + static inline uint64 pg_atomic_exchange_u64(volatile pg_atomic_uint64 *ptr, uint64 newval) { diff --git a/src/include/port/atomics/generic.h b/src/include/port/atomics/generic.h index c3b5f6d9a4..6113ab62a3 100644 --- a/src/include/port/atomics/generic.h +++ b/src/include/port/atomics/generic.h @@ -243,6 +243,24 @@ pg_atomic_sub_fetch_u32_impl(volatile pg_atomic_uint32 *ptr, int32 sub_) } #endif +#if !defined(PG_HAVE_ATOMIC_READ_MEMBARRIER_U32) && defined(PG_HAVE_ATOMIC_FETCH_ADD_U32) +#define PG_HAVE_ATOMIC_READ_MEMBARRIER_U32 +static inline uint32 +pg_atomic_read_membarrier_u32_impl(volatile pg_atomic_uint32 *ptr) +{ + return pg_atomic_fetch_add_u32_impl(ptr, 0); +} +#endif + +#if !defined(PG_HAVE_ATOMIC_WRITE_MEMBARRIER_U32) && defined(PG_HAVE_ATOMIC_EXCHANGE_U32) +#define PG_HAVE_ATOMIC_WRITE_MEMBARRIER_U32 +static inline void +pg_atomic_write_membarrier_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val) +{ + (void) pg_atomic_exchange_u32_impl(ptr, val); +} +#endif + #if !defined(PG_HAVE_ATOMIC_EXCHANGE_U64) && defined(PG_HAVE_ATOMIC_COMPARE_EXCHANGE_U64) #define PG_HAVE_ATOMIC_EXCHANGE_U64 static inline uint64 @@ -399,3 +417,21 @@ pg_atomic_sub_fetch_u64_impl(volatile pg_atomic_uint64 *ptr, int64 sub_) return pg_atomic_fetch_sub_u64_impl(ptr, sub_) - sub_; } #endif + +#if !defined(PG_HAVE_ATOMIC_READ_MEMBARRIER_U64) && defined(PG_HAVE_ATOMIC_FETCH_ADD_U64) +#define PG_HAVE_ATOMIC_READ_MEMBARRIER_U64 +static inline uint64 +pg_atomic_read_membarrier_u64_impl(volatile pg_atomic_uint64 *ptr) +{ + return pg_atomic_fetch_add_u64_impl(ptr, 0); +} +#endif + +#if !defined(PG_HAVE_ATOMIC_WRITE_MEMBARRIER_U64) && defined(PG_HAVE_ATOMIC_EXCHANGE_U64) +#define PG_HAVE_ATOMIC_WRITE_MEMBARRIER_U64 +static inline void +pg_atomic_write_membarrier_u64_impl(volatile pg_atomic_uint64 *ptr, uint64 val) +{ + (void) pg_atomic_exchange_u64_impl(ptr, val); +} +#endif -- 2.25.1
>From 60c529540ce32c98d9d8d088be09e73fde1080a5 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nat...@postgresql.org> Date: Fri, 23 Feb 2024 14:43:50 -0600 Subject: [PATCH v4 2/2] Convert archiver's force_dir_scan variable to an atomic variable. Commit XXXXXXXXXX introduced new atomic read/write functions with full barrier semantics, which are intended to ease converting non- performance-critical code to use atomic variables. This commit demonstrates one such conversion. Reviewed-by: Yong Li Discussion: https://postgr.es/m/20231110205128.GB1315705%40nathanxps13 --- src/backend/postmaster/pgarch.c | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c index 9c18e4b3ef..a8ec76ab81 100644 --- a/src/backend/postmaster/pgarch.c +++ b/src/backend/postmaster/pgarch.c @@ -45,7 +45,6 @@ #include "storage/proc.h" #include "storage/procsignal.h" #include "storage/shmem.h" -#include "storage/spin.h" #include "utils/guc.h" #include "utils/memutils.h" #include "utils/ps_status.h" @@ -83,11 +82,9 @@ typedef struct PgArchData int pgprocno; /* pgprocno of archiver process */ /* - * Forces a directory scan in pgarch_readyXlog(). Protected by arch_lck. + * Forces a directory scan in pgarch_readyXlog(). */ - bool force_dir_scan; - - slock_t arch_lck; + pg_atomic_uint32 force_dir_scan; } PgArchData; char *XLogArchiveLibrary = ""; @@ -174,7 +171,7 @@ PgArchShmemInit(void) /* First time through, so initialize */ MemSet(PgArch, 0, PgArchShmemSize()); PgArch->pgprocno = INVALID_PGPROCNO; - SpinLockInit(&PgArch->arch_lck); + pg_atomic_init_u32(&PgArch->force_dir_scan, false); } } @@ -545,18 +542,12 @@ pgarch_readyXlog(char *xlog) char XLogArchiveStatusDir[MAXPGPATH]; DIR *rldir; struct dirent *rlde; - bool force_dir_scan; /* * If a directory scan was requested, clear the stored file names and * proceed. */ - SpinLockAcquire(&PgArch->arch_lck); - force_dir_scan = PgArch->force_dir_scan; - PgArch->force_dir_scan = false; - SpinLockRelease(&PgArch->arch_lck); - - if (force_dir_scan) + if (pg_atomic_exchange_u32(&PgArch->force_dir_scan, false)) arch_files->arch_files_size = 0; /* @@ -707,9 +698,7 @@ ready_file_comparator(Datum a, Datum b, void *arg) void PgArchForceDirScan(void) { - SpinLockAcquire(&PgArch->arch_lck); - PgArch->force_dir_scan = true; - SpinLockRelease(&PgArch->arch_lck); + pg_atomic_write_membarrier_u32(&PgArch->force_dir_scan, true); } /* -- 2.25.1
