agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@postgresql.org
Subject: Re: small cleanup for s_lock.h
Date: Fri, 25 Sep 2026 16:03:30 -0500
Message-ID: <arbhohGoRM2Phzm4@nathan> (raw)
In-Reply-To: <af0D6fhLfVmG2fr7@nathan>
References: <afkUeI7UhacZ5ZFm@nathan>
<369933.1777933007@sss.pgh.pa.us>
<afoWBgsygtkCNCRQ@nathan>
<532705.1778000169@sss.pgh.pa.us>
<afovdi61nybDRbC3@nathan>
<afz5FHWr6rOB7bsL@nathan>
<af0AKUfzjGDvQdBk@nathan>
<af0D6fhLfVmG2fr7@nathan>
I've committed v5-{0001,0002} and attached a rebased version of 0003.
Besides fixing some small things discovered during LLM review, I decided to
remove the restriction that only one of TAS or S_LOCK can be defined.
Having both doesn't break anything; we just use the platform's S_LOCK in
that case.
--
nathan
From fe366a26ea50a50f0e810a8dd51069285ceefcf9 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Fri, 25 Sep 2026 16:01:13 -0500
Subject: [PATCH v6 1/1] Better express platform requirements in s_lock.h.
Presently, s_lock.h insists that every platform provide TAS(), and
its header comment doesn't say which of the macros a new platform
actually needs to supply. The real contract is that a platform
must provide either S_LOCK() or a TAS() for the default S_LOCK() to
be built on.
To better express that, this commit moves the "no spinlock support"
error to where the default S_LOCK() is defined, compiles s_lock()
only when the default S_LOCK() is in use, and provides a default
TAS_SPIN() only when there is a TAS() to base it on. The direct
callers of s_lock() in the spinlock tests are adjusted to match.
The header comment is reworked accordingly, dropping the note about
pre-9.5 volatile requirements and the caution about TAS()
spuriously failing along the way. The latter was added by commit
7f60b81e1a for a long-unsupported platform, and it only matters to
callers outside this file, which commit 499abb0c0f disallowed.
Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/afkUeI7UhacZ5ZFm%40nathan
---
src/backend/storage/lmgr/s_lock.c | 4 ++-
src/include/storage/s_lock.h | 46 +++++++++++++++----------------
src/test/regress/regress.c | 2 +-
3 files changed, 26 insertions(+), 26 deletions(-)
diff --git a/src/backend/storage/lmgr/s_lock.c b/src/backend/storage/lmgr/s_lock.c
index 6df568eccb3..795aeed1028 100644
--- a/src/backend/storage/lmgr/s_lock.c
+++ b/src/backend/storage/lmgr/s_lock.c
@@ -91,6 +91,7 @@ s_lock_stuck(const char *file, int line, const char *func)
#endif
}
+#ifdef USE_DEFAULT_S_LOCK
/*
* s_lock(lock) - platform-independent portion of waiting for a spinlock.
*/
@@ -110,6 +111,7 @@ s_lock(volatile slock_t *lock, const char *file, int line, const char *func)
return delayStatus.delays;
}
+#endif
#ifdef USE_DEFAULT_S_UNLOCK
void
@@ -291,7 +293,7 @@ main()
printf(" if S_LOCK() and TAS() are working.\n");
fflush(stdout);
- s_lock(&test_lock.lock, __FILE__, __LINE__, __func__);
+ S_LOCK(&test_lock.lock);
printf("S_LOCK_TEST: failed, lock not locked\n");
return 1;
diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index 17edc058b74..7e049dfac18 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -44,23 +44,14 @@
* atomic test-and-set only when it appears free.
*
* TAS() and TAS_SPIN() are NOT part of the API, and should never be called
- * directly.
- *
- * CAUTION: on some platforms TAS() and/or TAS_SPIN() may sometimes report
- * failure to acquire a lock even when the lock is not locked. For example,
- * on Alpha TAS() will "fail" if interrupted. Therefore a retry loop must
- * always be used, even if you are certain the lock is free.
+ * directly. A platform must provide either S_LOCK() or a TAS() for the
+ * default S_LOCK() to be built on. Currently, all supported platforms do
+ * the latter, so that is probably the best place to start if adding a new
+ * one.
*
* It is the responsibility of these macros to make sure that the compiler
* does not re-order accesses to shared memory to precede the actual lock
- * acquisition, or follow the lock release. Prior to PostgreSQL 9.5, this
- * was the caller's responsibility, which meant that callers had to use
- * volatile-qualified pointers to refer to both the spinlock itself and the
- * shared data being accessed within the spinlocked critical section. This
- * was notationally awkward, easy to forget (and thus error-prone), and
- * prevented some useful compiler optimizations. For these reasons, we
- * now require that the macros themselves prevent compiler re-ordering,
- * so that the caller doesn't need to take special precautions.
+ * acquisition, or follow the lock release.
*
* On platforms with weak memory ordering, the TAS(), TAS_SPIN(), and
* S_UNLOCK() macros must further include hardware-level memory fence
@@ -72,7 +63,7 @@
*
* On most supported platforms, TAS() uses a tas() function written
* in assembly language to execute a hardware atomic-test-and-set
- * instruction. Equivalent OS-supplied mutex routines could be used too.
+ * instruction. Equivalent compiler intrinsics are another popular option.
*
*
* Portions Copyright (c) 1996-2026, PostgreSQL Global Development Group
@@ -642,19 +633,23 @@ spin_delay(void)
#endif /* !defined(TAS) */
-/* Blow up if we didn't have any way to do spinlocks */
-#ifndef TAS
-#error PostgreSQL does not have spinlock support on this platform. Please report this to pgsql-bugs@lists.postgresql.org.
-#endif
-
-
/*
* Default Definitions - override these above as needed.
*/
+/*
+ * Make sure S_LOCK is defined, either explicitly for the platform or via a TAS
+ * macro for the platform.
+ */
#if !defined(S_LOCK)
+#ifdef TAS
+#define USE_DEFAULT_S_LOCK
+extern int s_lock(volatile slock_t *lock, const char *file, int line, const char *func);
#define S_LOCK(lock) \
(TAS(lock) ? s_lock((lock), __FILE__, __LINE__, __func__) : 0)
+#else
+#error PostgreSQL does not have spinlock support on this platform. Please report this to pgsql-bugs@lists.postgresql.org.
+#endif /* TAS */
#endif /* S_LOCK */
#if !defined(S_UNLOCK)
@@ -687,15 +682,18 @@ extern void s_unlock(volatile slock_t *lock);
#define SPIN_DELAY() ((void) 0)
#endif /* SPIN_DELAY */
-#if !defined(TAS_SPIN)
+/*
+ * TAS_SPIN is only needed by the default S_LOCK's helper function (s_lock()),
+ * so we only provide a default when there is a TAS to base it on.
+ */
+#if !defined(TAS_SPIN) && defined(TAS)
#define TAS_SPIN(lock) TAS(lock)
-#endif /* TAS_SPIN */
+#endif /* ! TAS_SPIN && TAS */
/*
* Platform-independent out-of-line support routines
*/
-extern int s_lock(volatile slock_t *lock, const char *file, int line, const char *func);
/* Support for dynamic adjustment of spins_per_delay */
#define DEFAULT_SPINS_PER_DELAY 100
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index c72ee31cdce..3deb4ed7203 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -675,7 +675,7 @@ test_spinlock(void)
S_UNLOCK(&struct_w_lock.lock);
/* and that "contended" acquisition works */
- s_lock(&struct_w_lock.lock, "testfile", 17, "testfunc");
+ S_LOCK(&struct_w_lock.lock);
S_UNLOCK(&struct_w_lock.lock);
/*
--
2.55.0
Attachments:
[text/plain] v6-0001-Better-express-platform-requirements-in-s_lock.h.patch (6.4K, ../arbhohGoRM2Phzm4@nathan/2-v6-0001-Better-express-platform-requirements-in-s_lock.h.patch)
download | inline diff:
From fe366a26ea50a50f0e810a8dd51069285ceefcf9 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Fri, 25 Sep 2026 16:01:13 -0500
Subject: [PATCH v6 1/1] Better express platform requirements in s_lock.h.
Presently, s_lock.h insists that every platform provide TAS(), and
its header comment doesn't say which of the macros a new platform
actually needs to supply. The real contract is that a platform
must provide either S_LOCK() or a TAS() for the default S_LOCK() to
be built on.
To better express that, this commit moves the "no spinlock support"
error to where the default S_LOCK() is defined, compiles s_lock()
only when the default S_LOCK() is in use, and provides a default
TAS_SPIN() only when there is a TAS() to base it on. The direct
callers of s_lock() in the spinlock tests are adjusted to match.
The header comment is reworked accordingly, dropping the note about
pre-9.5 volatile requirements and the caution about TAS()
spuriously failing along the way. The latter was added by commit
7f60b81e1a for a long-unsupported platform, and it only matters to
callers outside this file, which commit 499abb0c0f disallowed.
Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/afkUeI7UhacZ5ZFm%40nathan
---
src/backend/storage/lmgr/s_lock.c | 4 ++-
src/include/storage/s_lock.h | 46 +++++++++++++++----------------
src/test/regress/regress.c | 2 +-
3 files changed, 26 insertions(+), 26 deletions(-)
diff --git a/src/backend/storage/lmgr/s_lock.c b/src/backend/storage/lmgr/s_lock.c
index 6df568eccb3..795aeed1028 100644
--- a/src/backend/storage/lmgr/s_lock.c
+++ b/src/backend/storage/lmgr/s_lock.c
@@ -91,6 +91,7 @@ s_lock_stuck(const char *file, int line, const char *func)
#endif
}
+#ifdef USE_DEFAULT_S_LOCK
/*
* s_lock(lock) - platform-independent portion of waiting for a spinlock.
*/
@@ -110,6 +111,7 @@ s_lock(volatile slock_t *lock, const char *file, int line, const char *func)
return delayStatus.delays;
}
+#endif
#ifdef USE_DEFAULT_S_UNLOCK
void
@@ -291,7 +293,7 @@ main()
printf(" if S_LOCK() and TAS() are working.\n");
fflush(stdout);
- s_lock(&test_lock.lock, __FILE__, __LINE__, __func__);
+ S_LOCK(&test_lock.lock);
printf("S_LOCK_TEST: failed, lock not locked\n");
return 1;
diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index 17edc058b74..7e049dfac18 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -44,23 +44,14 @@
* atomic test-and-set only when it appears free.
*
* TAS() and TAS_SPIN() are NOT part of the API, and should never be called
- * directly.
- *
- * CAUTION: on some platforms TAS() and/or TAS_SPIN() may sometimes report
- * failure to acquire a lock even when the lock is not locked. For example,
- * on Alpha TAS() will "fail" if interrupted. Therefore a retry loop must
- * always be used, even if you are certain the lock is free.
+ * directly. A platform must provide either S_LOCK() or a TAS() for the
+ * default S_LOCK() to be built on. Currently, all supported platforms do
+ * the latter, so that is probably the best place to start if adding a new
+ * one.
*
* It is the responsibility of these macros to make sure that the compiler
* does not re-order accesses to shared memory to precede the actual lock
- * acquisition, or follow the lock release. Prior to PostgreSQL 9.5, this
- * was the caller's responsibility, which meant that callers had to use
- * volatile-qualified pointers to refer to both the spinlock itself and the
- * shared data being accessed within the spinlocked critical section. This
- * was notationally awkward, easy to forget (and thus error-prone), and
- * prevented some useful compiler optimizations. For these reasons, we
- * now require that the macros themselves prevent compiler re-ordering,
- * so that the caller doesn't need to take special precautions.
+ * acquisition, or follow the lock release.
*
* On platforms with weak memory ordering, the TAS(), TAS_SPIN(), and
* S_UNLOCK() macros must further include hardware-level memory fence
@@ -72,7 +63,7 @@
*
* On most supported platforms, TAS() uses a tas() function written
* in assembly language to execute a hardware atomic-test-and-set
- * instruction. Equivalent OS-supplied mutex routines could be used too.
+ * instruction. Equivalent compiler intrinsics are another popular option.
*
*
* Portions Copyright (c) 1996-2026, PostgreSQL Global Development Group
@@ -642,19 +633,23 @@ spin_delay(void)
#endif /* !defined(TAS) */
-/* Blow up if we didn't have any way to do spinlocks */
-#ifndef TAS
-#error PostgreSQL does not have spinlock support on this platform. Please report this to pgsql-bugs@lists.postgresql.org.
-#endif
-
-
/*
* Default Definitions - override these above as needed.
*/
+/*
+ * Make sure S_LOCK is defined, either explicitly for the platform or via a TAS
+ * macro for the platform.
+ */
#if !defined(S_LOCK)
+#ifdef TAS
+#define USE_DEFAULT_S_LOCK
+extern int s_lock(volatile slock_t *lock, const char *file, int line, const char *func);
#define S_LOCK(lock) \
(TAS(lock) ? s_lock((lock), __FILE__, __LINE__, __func__) : 0)
+#else
+#error PostgreSQL does not have spinlock support on this platform. Please report this to pgsql-bugs@lists.postgresql.org.
+#endif /* TAS */
#endif /* S_LOCK */
#if !defined(S_UNLOCK)
@@ -687,15 +682,18 @@ extern void s_unlock(volatile slock_t *lock);
#define SPIN_DELAY() ((void) 0)
#endif /* SPIN_DELAY */
-#if !defined(TAS_SPIN)
+/*
+ * TAS_SPIN is only needed by the default S_LOCK's helper function (s_lock()),
+ * so we only provide a default when there is a TAS to base it on.
+ */
+#if !defined(TAS_SPIN) && defined(TAS)
#define TAS_SPIN(lock) TAS(lock)
-#endif /* TAS_SPIN */
+#endif /* ! TAS_SPIN && TAS */
/*
* Platform-independent out-of-line support routines
*/
-extern int s_lock(volatile slock_t *lock, const char *file, int line, const char *func);
/* Support for dynamic adjustment of spins_per_delay */
#define DEFAULT_SPINS_PER_DELAY 100
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index c72ee31cdce..3deb4ed7203 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -675,7 +675,7 @@ test_spinlock(void)
S_UNLOCK(&struct_w_lock.lock);
/* and that "contended" acquisition works */
- s_lock(&struct_w_lock.lock, "testfile", 17, "testfunc");
+ S_LOCK(&struct_w_lock.lock);
S_UNLOCK(&struct_w_lock.lock);
/*
--
2.55.0
view thread (14+ messages) latest in thread
Message-ID: <arbhohGoRM2Phzm4@nathan>
Permalink: ../arbhohGoRM2Phzm4@nathan/
Also on: postgresql.org/message-id/arbhohGoRM2Phzm4@nathan
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, tgl@sss.pgh.pa.us
Subject: Re: small cleanup for s_lock.h
In-Reply-To: <arbhohGoRM2Phzm4@nathan>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox