locked reads for atomics
Moving this to a new thread and adding it to the January commitfest.
On Thu, Nov 09, 2023 at 03:27:33PM -0600, Nathan Bossart wrote:
On Tue, Nov 07, 2023 at 04:58:16PM -0800, Andres Freund wrote:
However, even if there's likely some other implied memory barrier that we
could piggyback on, the patch much simpler to understand if it doesn't change
coherency rules. There's no way the overhead could matter.I wonder if it's worth providing a set of "locked read" functions. Those
could just do a compare/exchange with 0 in the generic implementation. For
patches like this one where the overhead really shouldn't matter, I'd
encourage folks to use those to make it easy to reason about correctness.
Concretely, like this.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
locked_atomic_reads_v1.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/include/port/atomics.h b/src/include/port/atomics.h
index bbff945eba..21a6edac3e 100644
--- a/src/include/port/atomics.h
+++ b/src/include/port/atomics.h
@@ -228,7 +228,8 @@ pg_atomic_init_u32(volatile pg_atomic_uint32 *ptr, uint32 val)
* The read is guaranteed to return a value as it has been written by this or
* another process at some point in the past. There's however no cache
* coherency interaction guaranteeing the value hasn't since been written to
- * again.
+ * again. Consider using pg_atomic_locked_read_u32() unless you have a strong
+ * reason (e.g., performance) to use unlocked reads.
*
* No barrier semantics.
*/
@@ -239,6 +240,24 @@ pg_atomic_read_u32(volatile pg_atomic_uint32 *ptr)
return pg_atomic_read_u32_impl(ptr);
}
+/*
+ * pg_atomic_read_u32 - locked read from atomic variable.
+ *
+ * This read is guaranteed to read the current value, but note that there's no
+ * guarantee that the value isn't updated between when this function returns
+ * and when you try to use it. Note that this is less performant than an
+ * unlocked read (i.e., pg_atomic_read_u32()), but it is generally much easier
+ * to reason about correctness with locked reads.
+ *
+ * Full barrier semantics.
+ */
+static inline uint32
+pg_atomic_locked_read_u32(volatile pg_atomic_uint32 *ptr)
+{
+ AssertPointerAlignment(ptr, 4);
+ return pg_atomic_locked_read_u32_impl(ptr);
+}
+
/*
* pg_atomic_write_u32 - write to atomic variable.
*
@@ -429,6 +448,15 @@ pg_atomic_read_u64(volatile pg_atomic_uint64 *ptr)
return pg_atomic_read_u64_impl(ptr);
}
+static inline uint64
+pg_atomic_locked_read_u64(volatile pg_atomic_uint64 *ptr)
+{
+#ifndef PG_HAVE_ATOMIC_U64_SIMULATION
+ AssertPointerAlignment(ptr, 8);
+#endif
+ return pg_atomic_locked_read_u64_impl(ptr);
+}
+
static inline void
pg_atomic_write_u64(volatile pg_atomic_uint64 *ptr, uint64 val)
{
diff --git a/src/include/port/atomics/generic.h b/src/include/port/atomics/generic.h
index cb5804adbf..5bb3aea9b7 100644
--- a/src/include/port/atomics/generic.h
+++ b/src/include/port/atomics/generic.h
@@ -49,6 +49,25 @@ pg_atomic_read_u32_impl(volatile pg_atomic_uint32 *ptr)
}
#endif
+#ifndef PG_HAVE_ATOMIC_LOCKED_READ_U32
+#define PG_HAVE_ATOMIC_LOCKED_READ_U32
+static inline uint32
+pg_atomic_locked_read_u32_impl(volatile pg_atomic_uint32 *ptr)
+{
+ uint32 old = 0;
+
+ /*
+ * In the generic implementation, locked reads are implemented as a
+ * compare/exchange with 0. That'll fail or succeed, but always return the
+ * most up-to-date value. It might also store a 0, but only if the
+ * previous value was also a zero, i.e., harmless.
+ */
+ pg_atomic_compare_exchange_u32_impl(ptr, &old, 0);
+
+ return old;
+}
+#endif
+
#ifndef PG_HAVE_ATOMIC_WRITE_U32
#define PG_HAVE_ATOMIC_WRITE_U32
static inline void
@@ -325,6 +344,25 @@ pg_atomic_read_u64_impl(volatile pg_atomic_uint64 *ptr)
#endif /* PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY && !PG_HAVE_ATOMIC_U64_SIMULATION */
#endif /* PG_HAVE_ATOMIC_READ_U64 */
+#ifndef PG_HAVE_ATOMIC_LOCKED_READ_U64
+#define PG_HAVE_ATOMIC_LOCKED_READ_U64
+static inline uint64
+pg_atomic_locked_read_u64_impl(volatile pg_atomic_uint64 *ptr)
+{
+ uint64 old = 0;
+
+ /*
+ * In the generic implementation, locked reads are implemented as a
+ * compare/exchange with 0. That'll fail or succeed, but always return the
+ * most up-to-date value. It might also store a 0, but only if the
+ * previous value was also a zero, i.e., harmless.
+ */
+ pg_atomic_compare_exchange_u64_impl(ptr, &old, 0);
+
+ return old;
+}
+#endif
+
#ifndef PG_HAVE_ATOMIC_INIT_U64
#define PG_HAVE_ATOMIC_INIT_U64
static inline void
I wonder if it's worth providing a set of "locked read" functions.
Most out-of-order machines include “read acquire” and “write release” which are pretty close to what you’re suggesting. With the current routines, we only have “read relaxed” and “write relaxed”. I think implementing acquire/release semantics is a very good idea,
I would also like to clarify the properties of atomics. One very important question: Are atomics also volatile? If so, the compiler has very limited ability to move them around. If not, it is difficult to tell when or where they will take place unless the surrounding code is peppered with barriers.
Import Notes
Resolved by subject fallback
On Fri, Nov 10, 2023 at 09:49:06PM +0000, John Morris wrote:
Most out-of-order machines include “read acquire” and “write release”
which are pretty close to what you’re suggesting. With the current
routines, we only have “read relaxed” and “write relaxed”. I think
implementing acquire/release semantics is a very good idea,
We do have both pg_atomic_write_u32() and pg_atomic_unlocked_write_u32()
(see commit b0779ab), but AFAICT those only differ in the fallback/spinlock
implementations. I suppose there could be an unlocked 64-bit write on
platforms that have 8-byte single-copy atomicity but still need to use the
fallback/spinlock implementation for some reason, but that might be a bit
of a stretch, and the use-cases might be few and far between...
I would also like to clarify the properties of atomics. One very
important question: Are atomics also volatile?
The PostgreSQL atomics support appears to ensure they are volatile.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Hi,
On 2023-11-10 14:51:28 -0600, Nathan Bossart wrote:
Moving this to a new thread and adding it to the January commitfest.
On Thu, Nov 09, 2023 at 03:27:33PM -0600, Nathan Bossart wrote:
On Tue, Nov 07, 2023 at 04:58:16PM -0800, Andres Freund wrote:
However, even if there's likely some other implied memory barrier that we
could piggyback on, the patch much simpler to understand if it doesn't change
coherency rules. There's no way the overhead could matter.I wonder if it's worth providing a set of "locked read" functions. Those
could just do a compare/exchange with 0 in the generic implementation. For
patches like this one where the overhead really shouldn't matter, I'd
encourage folks to use those to make it easy to reason about correctness.Concretely, like this.
I don't think "locked" is a good name - there's no locking. I think that'll
deter their use, because it will make it sound more expensive than they are.
pg_atomic_read_membarrier_u32()?
@@ -228,7 +228,8 @@ pg_atomic_init_u32(volatile pg_atomic_uint32 *ptr, uint32 val) * The read is guaranteed to return a value as it has been written by this or * another process at some point in the past. There's however no cache * coherency interaction guaranteeing the value hasn't since been written to - * again. + * again. Consider using pg_atomic_locked_read_u32() unless you have a strong + * reason (e.g., performance) to use unlocked reads.
I think that's too strong an endorsement. Often there will be no difference in
difficulty analysing correctness, because the barrier pairing / the
interaction with the surrounding code needs to be analyzed just as much.
* No barrier semantics.
*/
@@ -239,6 +240,24 @@ pg_atomic_read_u32(volatile pg_atomic_uint32 *ptr)
return pg_atomic_read_u32_impl(ptr);
}+/* + * pg_atomic_read_u32 - locked read from atomic variable.
Un-updated name...
+ * This read is guaranteed to read the current value,
It doesn't guarantee that *at all*. What it guarantees is solely that the
current CPU won't be doing something that could lead to reading an outdated
value. To actually ensure the value is up2date, the modifying side also needs
to have used a form of barrier (in the form of fetch_add, compare_exchange,
etc or an explicit barrier).
+#ifndef PG_HAVE_ATOMIC_LOCKED_READ_U32 +#define PG_HAVE_ATOMIC_LOCKED_READ_U32 +static inline uint32 +pg_atomic_locked_read_u32_impl(volatile pg_atomic_uint32 *ptr) +{ + uint32 old = 0; + + /* + * In the generic implementation, locked reads are implemented as a + * compare/exchange with 0. That'll fail or succeed, but always return the + * most up-to-date value. It might also store a 0, but only if the + * previous value was also a zero, i.e., harmless. + */ + pg_atomic_compare_exchange_u32_impl(ptr, &old, 0); + + return old; +} +#endif
I suspect implementing it with an atomic fetch_add of 0 would be faster...
Greetings,
Andres Freund
Hi,
On 2023-11-10 21:49:06 +0000, John Morris wrote:
I wonder if it's worth providing a set of "locked read" functions.
Most out-of-order machines include “read acquire” and “write release” which
are pretty close to what you’re suggesting.
Is that really true? It's IA64 lingo. X86 doesn't have them, while arm has
more granular barriers, they don't neatly map onto acquire/release either.
I don't think an acquire here would actually be equivalent to making this a
full barrier - an acquire barrier allows moving reads or stores from *before*
the barrier to be moved after the barrier. It just prevents the opposite.
And for proper use of acquire/release semantics we'd need to pair operations
much more closely. Right now we often rely on another preceding memory barrier
to ensure correct ordering, having to use paired operations everywhere would
lead to slower code.
I thoroughly dislike how strongly C++11/C11 prefer paired atomics *on the same
address* over "global" fences. It often leads to substantially slower
code. And they don't at all map neatly on hardware, where largely barrier
semantics are *not* tied to individual addresses. And the fence specification
is just about unreadable (although I think they did fix some of the worst
issues).
Greetings,
Andres Freund
On Fri, Nov 10, 2023 at 03:11:50PM -0800, Andres Freund wrote:
On 2023-11-10 14:51:28 -0600, Nathan Bossart wrote:
+ * This read is guaranteed to read the current value,
It doesn't guarantee that *at all*. What it guarantees is solely that the
current CPU won't be doing something that could lead to reading an outdated
value. To actually ensure the value is up2date, the modifying side also needs
to have used a form of barrier (in the form of fetch_add, compare_exchange,
etc or an explicit barrier).
Okay, I think I was missing that this doesn't work along with
pg_atomic_write_u32() because that doesn't have any barrier semantics
(probably because the spinlock version does). IIUC you'd want to use
pg_atomic_exchange_u32() to write the value instead, which seems to really
just be another compare/exchange under the hood.
Speaking of the spinlock implementation of pg_atomic_write_u32(), I've been
staring at this comment for a while and can't make sense of it:
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);
}
It refers to "unlocked writes," but this isn't
pg_atomic_unlocked_write_u32_impl(). The original thread for this comment
[0]: /messages/by-id/14947.1475690465@sss.pgh.pa.us
different here, such as "write without any barrier semantics?"
[0]: /messages/by-id/14947.1475690465@sss.pgh.pa.us
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Hi,
On 2023-11-10 20:38:13 -0600, Nathan Bossart wrote:
On Fri, Nov 10, 2023 at 03:11:50PM -0800, Andres Freund wrote:
On 2023-11-10 14:51:28 -0600, Nathan Bossart wrote:
+ * This read is guaranteed to read the current value,
It doesn't guarantee that *at all*. What it guarantees is solely that the
current CPU won't be doing something that could lead to reading an outdated
value. To actually ensure the value is up2date, the modifying side also needs
to have used a form of barrier (in the form of fetch_add, compare_exchange,
etc or an explicit barrier).Okay, I think I was missing that this doesn't work along with
pg_atomic_write_u32() because that doesn't have any barrier semantics
(probably because the spinlock version does). IIUC you'd want to use
pg_atomic_exchange_u32() to write the value instead, which seems to really
just be another compare/exchange under the hood.
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.
Speaking of the spinlock implementation of pg_atomic_write_u32(), I've been
staring at this comment for a while and can't make sense of it: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);
}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.
Greetings,
Andres Freund
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
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
Attachments:
v2-0001-add-membarrier-operations.patchtext/x-diff; charset=us-asciiDownload
From 53f717e84a3aa912f9d89dce4be7f77d9bffb3c2 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@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
v2-0002-replace-force_dir_scan-with-atomics.patchtext/x-diff; charset=us-asciiDownload
From baafbb3c1cbb1665be19e8d06172840bcb07c820 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@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
On Nov 28, 2023, at 05:00, Nathan Bossart <nathandbossart@gmail.com> wrote:
External Email
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
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
On Tue, Nov 28, 2023 at 2:30 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
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
There's some immediate use for reads/writes with barrier semantics -
/messages/by-id/CALj2ACXrePj4E6ocKr-+b=rjT-8yeMmHnEeWQP1bc-WXETfTVw@mail.gmail.com.
Any plan for taking this forward?
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Thu, 2024-02-22 at 12:58 +0530, Bharath Rupireddy wrote:
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
On Thu, Feb 22, 2024 at 11:53:50AM -0800, Jeff Davis wrote:
On Thu, 2024-02-22 at 12:58 +0530, Bharath Rupireddy wrote:
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?
The former. Besides the 0002 patch tracked here, there's at least one
other patch [0]https://commitfest.postgresql.org/47/4330/ 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/
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Fri, 2024-02-23 at 10:17 -0600, Nathan Bossart wrote:
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.
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
On Fri, Feb 23, 2024 at 10:25:00AM -0800, Jeff Davis wrote:
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.
I think that's about right. The upthread feedback from Andres [0]/messages/by-id/20231110231150.fjm77gup2i7xu6hc@alap3.anarazel.de provides
some additional context.
[0]: /messages/by-id/20231110231150.fjm77gup2i7xu6hc@alap3.anarazel.de
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
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.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
v3-0001-Introduce-atomic-read-write-functions-with-full-b.patchtext/x-diff; charset=us-asciiDownload
From f1441aca96f157c5557d9a961fe85902b510f293 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
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(+)
diff --git a/src/include/port/atomics.h b/src/include/port/atomics.h
index bf151037f7..36a0ac9d85 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 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
v3-0002-Convert-archiver-s-force_dir_scan-variable-to-an-.patchtext/x-diff; charset=us-asciiDownload
From c45a6aa1453c4b1bde59fb96ef93e2d94ac72a06 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
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(-)
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
Hi,
On 2024-02-23 10:25:00 -0800, Jeff Davis wrote:
On Fri, 2024-02-23 at 10:17 -0600, Nathan Bossart wrote:
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.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.
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
Hi,
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.
Greetings,
Andres Freund
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
Attachments:
v4-0001-Introduce-atomic-read-write-functions-with-full-b.patchtext/x-diff; charset=us-asciiDownload
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
v4-0002-Convert-archiver-s-force_dir_scan-variable-to-an-.patchtext/x-diff; charset=us-asciiDownload
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
Committed. Thank you for reviewing!
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com