Hi, On 2024-07-29 09:40:26 -0700, Andres Freund wrote: > On 2024-07-29 12:33:13 -0400, Tom Lane wrote: > > Andres Freund <and...@anarazel.de> writes: > > > On 2024-07-29 11:31:56 -0400, Tom Lane wrote: > > >> There was some recent discussion about getting rid of > > >> --disable-spinlocks on the grounds that nobody would use > > >> hardware that lacked native spinlocks. But now I wonder > > >> if there is a testing/debugging reason to keep it. > > > > > Seems it'd be a lot more straightforward to just add an assertion to the > > > x86-64 spinlock implementation verifying that the spinlock isn't already > > > free? > > FWIW, I quickly hacked that up, and it indeed quickly fails with 0393f542d72^ > and passes with 0393f542d72.
Thought it'd be valuable to post a patch to go along with this, to -hackers. The thread started at [1] Other context from this discussion: > > I dunno, is that the only extra check that the --disable-spinlocks > > implementation is providing? > > I think it also provides the (valuable!) check that spinlocks were actually > initialized. But that again seems like something we'd be better off adding > more general infrastructure for - nobody runs --disable-spinlocks locally, we > shouldn't need to run this on the buildfarm to find problems like this. > > > > I'm kind of allergic to putting Asserts into spinlocked code segments, > > mostly on the grounds that it violates the straight-line-code precept. > > I suppose it's not really that bad for tests that you don't expect > > to fail, but still ... > > I don't think the spinlock implementation itself is really affected by that > rule - after all, the --disable-spinlocks implementation actually consists out > of several layers of external function calls (including syscalls in some > cases!). Greetings, Andres Freund [1] https://www.postgresql.org/message-id/E1sYSF2-001lEB-D1%40gemulon.postgresql.org
>From 06bd992f4be2f46586bfc7bab3135b3a2ca84e72 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Mon, 29 Jul 2024 09:48:26 -0700 Subject: [PATCH v1] Detect unlocking an unlocked spinlock Author: Reviewed-by: Discussion: https://postgr.es/m/20240729164026.yurg37tej34o7...@awork3.anarazel.de Backpatch: --- src/include/storage/spin.h | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/include/storage/spin.h b/src/include/storage/spin.h index c0679c59992..7ec32cd816a 100644 --- a/src/include/storage/spin.h +++ b/src/include/storage/spin.h @@ -61,7 +61,22 @@ #define SpinLockAcquire(lock) S_LOCK(lock) -#define SpinLockRelease(lock) S_UNLOCK(lock) +static inline void +SpinLockRelease(volatile slock_t *lock) +{ + /* + * Check that this backend actually held the spinlock. Implemented as a + * static inline to avoid multiple-evaluation hazards. + * + * Can't assert when using the semaphore fallback implementation - it + * doesn't implement S_LOCK_FREE() - but the fallback triggers failures in + * the case of double-release on its own. + */ +#ifdef HAVE_SPINLOCKS + Assert(!S_LOCK_FREE(lock)); +#endif + S_UNLOCK(lock); +} #define SpinLockFree(lock) S_LOCK_FREE(lock) -- 2.45.2.746.g06e570c0df.dirty
