Here's a v2 of the patch set in which I've attempted to address all
feedback. I've also added a pg_write_membarrier_u* pair of functions that
provide an easy way to write to an atomic variable with full barrier
semantics. In the generic implementation, these are just aliases for an
atomic exchange.
0002 demonstrates how these functions might be used to eliminate the
arch_lck spinlock, which is only ever used for one boolean variable. My
hope is that the membarrier functions make eliminating spinlocks for
non-performance-sensitive code easy to reason about.
(We might be able to use a pg_atomic_flag instead for 0002, but that code
seems intended for a slightly different use-case and has more complicated
barrier semantics.)
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 53f717e84a3aa912f9d89dce4be7f77d9bffb3c2 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Fri, 24 Nov 2023 20:21:12 -0600
Subject: [PATCH v2 1/2] add membarrier operations
---
src/include/port/atomics.h | 59 ++++++++++++++++++++++++++++++
src/include/port/atomics/generic.h | 36 ++++++++++++++++++
2 files changed, 95 insertions(+)
diff --git a/src/include/port/atomics.h b/src/include/port/atomics.h
index bbff945eba..a889f0236e 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(). Note
+ * that this is less performant than pg_atomic_read_u32(), but 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,27 @@ 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() 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.
+ */
+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 +470,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 +488,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 95d99dd0be..9bcc3b48ce 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 baafbb3c1cbb1665be19e8d06172840bcb07c820 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Mon, 27 Nov 2023 12:47:06 -0600
Subject: [PATCH v2 2/2] replace force_dir_scan with atomics
---
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 46af349564..dccefb160a 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);
}
}
@@ -549,18 +546,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;
/*
@@ -711,9 +702,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
