Date: Sat, 24 Feb 2024 09:27:34 -0600
From: Nathan Bossart <nathandbossart@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: Jeff Davis <pgsql@j-davis.com>,
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	pgsql-hackers@postgresql.org
Subject: Re: locked reads for atomics
Message-ID: <20240224152734.GA1862419@nathanxps13>
References: <20231111024839.op2d2id33x777mll@alap3.anarazel.de>
 <20231111025529.GA1409540@nathanxps13>
 <20231127210030.GA140335@nathanxps13>
 <CALj2ACUOEx_s0DxJL57bsZxQM1+30p_1G9eCZdmsCz45QNprhg@mail.gmail.com>
 <f7519ac5437f1e9f1bd1bb5a4f30fad42eaa6ad7.camel@j-davis.com>
 <20240223161758.GA1697194@nathanxps13>
 <274e5925684e71b0df4f7c0f473b8ce6fc6c382b.camel@j-davis.com>
 <20240223193247.GA1698856@nathanxps13>
 <20240223205812.GA1804550@nathanxps13>
 <20240224013449.b7hhtq5t4qh6j7m7@alap3.anarazel.de>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="vkogqOf2sHV7VnPd"
Content-Disposition: inline
In-Reply-To: <20240224013449.b7hhtq5t4qh6j7m7@alap3.anarazel.de>
Archived-At: <https://www.postgresql.org/message-id/20240224152734.GA1862419%40nathanxps13>
Precedence: bulk


--vkogqOf2sHV7VnPd
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--vkogqOf2sHV7VnPd
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v4-0001-Introduce-atomic-read-write-functions-with-full-b.patch"

From 647eea91cc07e2d6a4d6634bcb933df245711fa0 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@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


--vkogqOf2sHV7VnPd
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v4-0002-Convert-archiver-s-force_dir_scan-variable-to-an-.patch"

From 60c529540ce32c98d9d8d088be09e73fde1080a5 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@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


--vkogqOf2sHV7VnPd--