Subject: Re: locked reads for atomics

From: Nathan Bossart <[email-masked]>
Date: Fri, 10 Nov 2023 20:55:29 -0600

On Fri, Nov 10, 2023 at 06:48:39PM -0800, Andres Freund wrote:
> Yes. We should optimize pg_atomic_exchange_u32() one of these days - it can be
> done *far* faster than a cmpxchg. When I was adding the atomic abstraction
> there was concern with utilizing too many different atomic instructions. I
> didn't really agree back then, but these days I really don't see a reason to
> not use a few more intrinsics.

I might give this a try, if for no other reason than it'd force me to
improve my mental model of this stuff.  :)

>> It refers to "unlocked writes," but this isn't
>> pg_atomic_unlocked_write_u32_impl().  The original thread for this comment
>> [0] doesn't offer any hints, either.  Does "unlocked" mean something
>> different here, such as "write without any barrier semantics?"
> 
> It's just about not using the spinlock. If we were to *not* use a spinlock
> here, we'd break pg_atomic_compare_exchange_u32(), because the
> spinlock-implementation of pg_atomic_compare_exchange_u32() needs to actually
> be able to rely on no concurrent changes to the value to happen.

Thanks for clarifying.  I thought it might've been hinting at something
beyond the compare/exchange implications.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 27 Nov 2023 15:00:30 -0600

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.)

>From 53f717e84a3aa912f9d89dce4be7f77d9bffb3c2 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
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 <[email-masked]>
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

---

From: "Li, Yong" <[email-masked]>
Date: Wed, 17 Jan 2024 03:48:43 +0000

> On Nov 28, 2023, at 05:00, Nathan Bossart <[email-masked]> wrote:
> 
> External Email
> 

Hi Nathan,

The patch looks good to me.

The patch adds two pairs of atomic functions that provide full-barrier semantics to atomic read/write operations. The patch also includes an example of how this new functions can be used to replace spin locks.

The patch applies cleanly to HEAD. “make check-world” also runs cleanly with no error.  I am moving it to Ready for Committers.

Regards,
Yong

---

From: Bharath Rupireddy <[email-masked]>
Date: Thu, 22 Feb 2024 12:58:37 +0530

> feedback.  I've also added a pg_write_membarrier_u* pair of functions that

There's some immediate use for reads/writes with barrier semantics -
https://www.postgresql.org/message-id/CALj2ACXrePj4E6ocKr-%2Bb%3DrjT-8yeMmHnEeWQP1bc-WXETfTVw%40mail.gmail.com.
Any plan for taking this forward?

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: Jeff Davis <[email-masked]>
Date: Thu, 22 Feb 2024 11:53:50 -0800

> There's some immediate use for reads/writes with barrier semantics -

Is this mainly a convenience for safety/readability? Or is it faster in
some cases than doing an atomic access with separate memory barriers?

Regards,
	Jeff Davis

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 23 Feb 2024 10:17:58 -0600

> some cases than doing an atomic access with separate memory barriers?

The former.  Besides the 0002 patch tracked here, there's at least one
other patch [0] that could probably use these new functions.  The idea is
to provide an easy way to remove spinlocks, etc. and use atomics for less
performance-sensitive stuff.  The implementations are intended to be
relatively inexpensive and might continue to improve in the future, but the
functions are primarily meant to help reason about correctness.

I don't mind prioritizing these patches, especially since there now seems
to be multiple patches waiting on it.  IIRC I was worried about not having
enough support for this change, but I might now have it.

[0] https://commitfest.postgresql.org/47/4330/

---

From: Jeff Davis <[email-masked]>
Date: Fri, 23 Feb 2024 10:25:00 -0800

On Fri, 2024-02-23 at 10:17 -0600, Nathan Bossart wrote:
> The idea is

To be clear:

  x = pg_atomic_[read|write]_membarrier_u64(&v);

is semantically equivalent to:

  pg_memory_barrier();
  x = pg_atomic_[read|write]_u64(&v);
  pg_memory_barrier();

?

If so, that does seem more convenient.

Regards,
	Jeff Davis

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 23 Feb 2024 13:32:47 -0600

> If so, that does seem more convenient.

I think that's about right.  The upthread feedback from Andres [0] provides
some additional context.

[0] https://postgr.es/m/20231110231150.fjm77gup2i7xu6hc%40alap3.anarazel.de

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 23 Feb 2024 14:58:12 -0600

Here is a v3 of the patch set with the first draft of the commit messages.
There are no code differences between v2 and v3.

>From f1441aca96f157c5557d9a961fe85902b510f293 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Fri, 23 Feb 2024 14:23:15 -0600
Subject: [PATCH v3 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         | 59 ++++++++++++++++++++++++++++++
 src/include/port/atomics/generic.h | 36 ++++++++++++++++++
 2 files changed, 95 insertions(+)

index bf151037f7..36a0ac9d85 100644
 
+/*
 
+/*
 
+static inline uint64
 
+static inline void
index c3b5f6d9a4..6113ab62a3 100644

>From c45a6aa1453c4b1bde59fb96ef93e2d94ac72a06 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Fri, 23 Feb 2024 14:43:50 -0600
Subject: [PATCH v3 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(-)

index 9c18e4b3ef..a8ec76ab81 100644
 
 	/*

@@ -545,18 +542,12 @@ pgarch_readyXlog(char *xlog)
 	char		XLogArchiveStatusDir[MAXPGPATH];
 	DIR		   *rldir;
 	struct dirent *rlde;
-	bool		force_dir_scan;
 
 	/*
 
 	/*
@@ -707,9 +698,7 @@ ready_file_comparator(Datum a, Datum b, void *arg)
 void
 PgArchForceDirScan(void)
 {
 
 /*
-- 
2.25.1

---

From: Andres Freund <[email-masked]>
Date: Fri, 23 Feb 2024 17:30:26 -0800

Hi,

> If so, that does seem more convenient.

Kinda. Semantically I think that's correct, however it doesn't commonly make
sense to have both those memory barriers, so you wouldn't really write code
like that and thus comparing on the basis of convenience doesn't quite seem
right.

Rather than convenience, I think performance and simplicity are better
arguments. If you're going to execute a read and then a memory barrier, it's
going to be faster to just do a single atomic operation. And it's a bit
simpler to analyze on which "side" of the read/write the barrier is needed.

Greetings,

Andres Freund

---

From: Andres Freund <[email-masked]>
Date: Fri, 23 Feb 2024 17:34:49 -0800

Hi,

> + * 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.

Greetings,

Andres Freund

---

From: Nathan Bossart <[email-masked]>
Date: Sat, 24 Feb 2024 09:27:34 -0600

> data structure with the shared buffers case.

I removed the reference to pg_atomic_unlocked_write_u32().

>From 647eea91cc07e2d6a4d6634bcb933df245711fa0 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Fri, 23 Feb 2024 14:23:15 -0600
Subject: [PATCH v4 1/2] Introduce atomic read/write functions with full
 barrier semantics.

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(+)

index bf151037f7..c3b14440cf 100644
 
+/*
+ * 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
@@ -438,6 +487,15 @@ pg_atomic_write_u64(volatile pg_atomic_uint64 *ptr, uint64 val)
 
+static inline void

>From 60c529540ce32c98d9d8d088be09e73fde1080a5 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
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.

Reviewed-by: Yong Li

 	/*

 
 	/*
 
 	/*
 
 /*
-- 
2.25.1

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 29 Feb 2024 10:24:57 -0600

Committed.  Thank you for reviewing!