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