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 <[email protected]> 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 <[email protected]> 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 [email protected]. -#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 [email protected]. +#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
