pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedconvert SpinLock* macros to static inline functions
7+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2026-02-23 bfc321b472 Convert SpinLock* macros to static inline functions.
2026-02-19 ba401828c1 Remove SpinLockFree() and S_LOCK_FREE().
* convert SpinLock* macros to static inline functions
@ 2026-02-18 17:28 Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 2 replies; 7+ messages in thread
From: Nathan Bossart @ 2026-02-18 17:28 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Andres Freund <andres@anarazel.de>; pgsql-hackers
Starting a new thread from [0].
On Wed, Feb 18, 2026 at 02:17:34AM +0200, Heikki Linnakangas wrote:
> On 18/02/2026 01:11, Andres Freund wrote:
>> I think the spinlock functions should also assert this.
>
> +1
While working on adding assertions that we are not in a signal handler to
the spinlock functions, I figured I'd take the opportunity to convert the
spin.h macros to static inline functions. This was previously discussed
[1], but AFAICT there was debate over whether to remove S_LOCK and friends
altogether (which doesn't seem to have happened). But IIUC nobody was
horribly opposed to $subject, which I think makes sense to do as
prerequisite work for the aforementioned assertions.
However, as soon as I did this, I got a bunch of build failures because
various parts of the code still use volatile qualifiers quite liberally.
It looks like most of these (e.g., see code from commits 2487d872e0,
966fb05b58, 4bc15a8bfb, and 4db3744f1f) predate making spinlocks compiler
barriers (commit 0709b7ee72) or were cargo-culted from code that predated
it. So, IIUC, it's probably safe to remove these volatile qualifiers now.
We could alternatively add volatile qualifiers to the new static inline
function parameters, but that seems like it might just encourage continued
unnecessary use.
I also noticed that SpinLockFree() is unused (and apparently never was).
There seems to have been agreement back in 2020 to remove it [2], but it
still exists. I added a prerequisite patch for this, too.
Thoughts?
[0] https://postgr.es/m/2f25aa74-990d-4412-a032-c876dbcff667%40iki.fi
[1] https://postgr.es/m/flat/20200617183354.pm3biu3zbmo2pktq%40alap3.anarazel.de#160ac60fddc7934777a0d20...
[2] https://postgr.es/m/flat/20200608225338.m5zho424w6lpwb2d%40alap3.anarazel.de
--
nathan
Attachments: download all (3 files, .tar.gz)
v1-0001-Remove-unnecessary-volatile-qualifiers.patch (text/plain, 5.2K) download
From 9917d563397ffac4b41af2e9d58dbf99efbf0924 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Wed, 18 Feb 2026 10:31:24 -0600
Subject: [PATCH v1 1/3] Remove unnecessary volatile qualifiers.
---
src/backend/replication/syncrep.c | 19 ++++++++-----------
src/backend/storage/ipc/procsignal.c | 4 ++--
src/backend/storage/lmgr/lock.c | 2 +-
src/test/modules/test_shm_mq/setup.c | 4 ++--
src/test/modules/test_shm_mq/worker.c | 2 +-
5 files changed, 14 insertions(+), 17 deletions(-)
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index 7ea6001e9ad..045fd35786a 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -473,7 +473,6 @@ SyncRepInitConfig(void)
void
SyncRepReleaseWaiters(void)
{
- volatile WalSndCtlData *walsndctl = WalSndCtl;
XLogRecPtr writePtr;
XLogRecPtr flushPtr;
XLogRecPtr applyPtr;
@@ -548,19 +547,19 @@ SyncRepReleaseWaiters(void)
* Set the lsn first so that when we wake backends they will release up to
* this location.
*/
- if (walsndctl->lsn[SYNC_REP_WAIT_WRITE] < writePtr)
+ if (WalSndCtl->lsn[SYNC_REP_WAIT_WRITE] < writePtr)
{
- walsndctl->lsn[SYNC_REP_WAIT_WRITE] = writePtr;
+ WalSndCtl->lsn[SYNC_REP_WAIT_WRITE] = writePtr;
numwrite = SyncRepWakeQueue(false, SYNC_REP_WAIT_WRITE);
}
- if (walsndctl->lsn[SYNC_REP_WAIT_FLUSH] < flushPtr)
+ if (WalSndCtl->lsn[SYNC_REP_WAIT_FLUSH] < flushPtr)
{
- walsndctl->lsn[SYNC_REP_WAIT_FLUSH] = flushPtr;
+ WalSndCtl->lsn[SYNC_REP_WAIT_FLUSH] = flushPtr;
numflush = SyncRepWakeQueue(false, SYNC_REP_WAIT_FLUSH);
}
- if (walsndctl->lsn[SYNC_REP_WAIT_APPLY] < applyPtr)
+ if (WalSndCtl->lsn[SYNC_REP_WAIT_APPLY] < applyPtr)
{
- walsndctl->lsn[SYNC_REP_WAIT_APPLY] = applyPtr;
+ WalSndCtl->lsn[SYNC_REP_WAIT_APPLY] = applyPtr;
numapply = SyncRepWakeQueue(false, SYNC_REP_WAIT_APPLY);
}
@@ -767,8 +766,7 @@ SyncRepGetCandidateStandbys(SyncRepStandbyData **standbys)
n = 0;
for (i = 0; i < max_wal_senders; i++)
{
- volatile WalSnd *walsnd; /* Use volatile pointer to prevent code
- * rearrangement */
+ WalSnd *walsnd;
SyncRepStandbyData *stby;
WalSndState state; /* not included in SyncRepStandbyData */
@@ -905,7 +903,6 @@ SyncRepGetStandbyPriority(void)
static int
SyncRepWakeQueue(bool all, int mode)
{
- volatile WalSndCtlData *walsndctl = WalSndCtl;
int numprocs = 0;
dlist_mutable_iter iter;
@@ -920,7 +917,7 @@ SyncRepWakeQueue(bool all, int mode)
/*
* Assume the queue is ordered by LSN
*/
- if (!all && walsndctl->lsn[mode] < proc->waitLSN)
+ if (!all && WalSndCtl->lsn[mode] < proc->waitLSN)
return numprocs;
/*
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index 5d33559926a..97791f979ef 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -283,7 +283,7 @@ CleanupProcSignalState(int status, Datum arg)
int
SendProcSignal(pid_t pid, ProcSignalReason reason, ProcNumber procNumber)
{
- volatile ProcSignalSlot *slot;
+ ProcSignalSlot *slot;
if (procNumber != INVALID_PROC_NUMBER)
{
@@ -394,7 +394,7 @@ EmitProcSignalBarrier(ProcSignalBarrierType type)
*/
for (int i = NumProcSignalSlots - 1; i >= 0; i--)
{
- volatile ProcSignalSlot *slot = &ProcSignal->psh_slot[i];
+ ProcSignalSlot *slot = &ProcSignal->psh_slot[i];
pid_t pid = pg_atomic_read_u32(&slot->pss_pid);
if (pid != 0)
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index e1168ad3837..718de1b5e98 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -309,7 +309,7 @@ typedef struct
uint32 count[FAST_PATH_STRONG_LOCK_HASH_PARTITIONS];
} FastPathStrongRelationLockData;
-static volatile FastPathStrongRelationLockData *FastPathStrongRelationLocks;
+static FastPathStrongRelationLockData *FastPathStrongRelationLocks;
/*
diff --git a/src/test/modules/test_shm_mq/setup.c b/src/test/modules/test_shm_mq/setup.c
index 579e5933d28..a1ec13d66b9 100644
--- a/src/test/modules/test_shm_mq/setup.c
+++ b/src/test/modules/test_shm_mq/setup.c
@@ -36,7 +36,7 @@ static worker_state *setup_background_workers(int nworkers,
dsm_segment *seg);
static void cleanup_background_workers(dsm_segment *seg, Datum arg);
static void wait_for_workers_to_become_ready(worker_state *wstate,
- volatile test_shm_mq_header *hdr);
+ test_shm_mq_header *hdr);
static bool check_worker_status(worker_state *wstate);
/* value cached, fetched from shared memory */
@@ -256,7 +256,7 @@ cleanup_background_workers(dsm_segment *seg, Datum arg)
static void
wait_for_workers_to_become_ready(worker_state *wstate,
- volatile test_shm_mq_header *hdr)
+ test_shm_mq_header *hdr)
{
bool result = false;
diff --git a/src/test/modules/test_shm_mq/worker.c b/src/test/modules/test_shm_mq/worker.c
index 368e4f3f234..14972f7e7cd 100644
--- a/src/test/modules/test_shm_mq/worker.c
+++ b/src/test/modules/test_shm_mq/worker.c
@@ -50,7 +50,7 @@ test_shm_mq_main(Datum main_arg)
shm_toc *toc;
shm_mq_handle *inqh;
shm_mq_handle *outqh;
- volatile test_shm_mq_header *hdr;
+ test_shm_mq_header *hdr;
int myworkernumber;
PGPROC *registrant;
--
2.50.1 (Apple Git-155)
v1-0002-Remove-SpinLockFree-and-S_LOCK_FREE.patch (text/plain, 3.2K) download
From 5033d7108d4203a34fb7eb84f793494ded735803 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Wed, 18 Feb 2026 11:03:01 -0600
Subject: [PATCH v1 2/3] Remove SpinLockFree() and S_LOCK_FREE().
---
src/backend/storage/lmgr/s_lock.c | 24 ------------------------
src/include/storage/s_lock.h | 8 --------
src/include/storage/spin.h | 6 ------
3 files changed, 38 deletions(-)
diff --git a/src/backend/storage/lmgr/s_lock.c b/src/backend/storage/lmgr/s_lock.c
index 5b79556bc9c..6df568eccb3 100644
--- a/src/backend/storage/lmgr/s_lock.c
+++ b/src/backend/storage/lmgr/s_lock.c
@@ -262,12 +262,6 @@ main()
return 1;
}
- if (!S_LOCK_FREE(&test_lock.lock))
- {
- printf("S_LOCK_TEST: failed, lock not initialized\n");
- return 1;
- }
-
S_LOCK(&test_lock.lock);
if (test_lock.pad1 != 0x44 || test_lock.pad2 != 0x44)
@@ -276,12 +270,6 @@ main()
return 1;
}
- if (S_LOCK_FREE(&test_lock.lock))
- {
- printf("S_LOCK_TEST: failed, lock not locked\n");
- return 1;
- }
-
S_UNLOCK(&test_lock.lock);
if (test_lock.pad1 != 0x44 || test_lock.pad2 != 0x44)
@@ -290,12 +278,6 @@ main()
return 1;
}
- if (!S_LOCK_FREE(&test_lock.lock))
- {
- printf("S_LOCK_TEST: failed, lock not unlocked\n");
- return 1;
- }
-
S_LOCK(&test_lock.lock);
if (test_lock.pad1 != 0x44 || test_lock.pad2 != 0x44)
@@ -304,12 +286,6 @@ main()
return 1;
}
- if (S_LOCK_FREE(&test_lock.lock))
- {
- printf("S_LOCK_TEST: failed, lock not re-locked\n");
- return 1;
- }
-
printf("S_LOCK_TEST: this will print %d stars and then\n", NUM_DELAYS);
printf(" exit with a 'stuck spinlock' message\n");
printf(" if S_LOCK() and TAS() are working.\n");
diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index 3d9070e79d4..89d28ac40c9 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -21,10 +21,6 @@
* void S_UNLOCK(slock_t *lock)
* Unlock a previously acquired lock.
*
- * bool S_LOCK_FREE(slock_t *lock)
- * Tests if the lock is free. Returns true if free, false if locked.
- * This does *not* change the state of the lock.
- *
* void SPIN_DELAY(void)
* Delay operation to occur inside spinlock wait loop.
*
@@ -671,10 +667,6 @@ spin_delay(void)
(TAS(lock) ? s_lock((lock), __FILE__, __LINE__, __func__) : 0)
#endif /* S_LOCK */
-#if !defined(S_LOCK_FREE)
-#define S_LOCK_FREE(lock) (*(lock) == 0)
-#endif /* S_LOCK_FREE */
-
#if !defined(S_UNLOCK)
/*
* Our default implementation of S_UNLOCK is essentially *(lock) = 0. This
diff --git a/src/include/storage/spin.h b/src/include/storage/spin.h
index 0f160d0e395..78c95ae538b 100644
--- a/src/include/storage/spin.h
+++ b/src/include/storage/spin.h
@@ -18,10 +18,6 @@
* void SpinLockRelease(volatile slock_t *lock)
* Unlock a previously acquired lock.
*
- * bool SpinLockFree(slock_t *lock)
- * Tests if the lock is free. Returns true if free, false if locked.
- * This does *not* change the state of the lock.
- *
* Callers must beware that the macro argument may be evaluated multiple
* times!
*
@@ -60,6 +56,4 @@
#define SpinLockRelease(lock) S_UNLOCK(lock)
-#define SpinLockFree(lock) S_LOCK_FREE(lock)
-
#endif /* SPIN_H */
--
2.50.1 (Apple Git-155)
v1-0003-Convert-SpinLock-macros-to-static-inline-function.patch (text/plain, 2.6K) download
From e757c04f3da5729400d466ffa8a0bf91f8cf272f Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Wed, 18 Feb 2026 10:32:26 -0600
Subject: [PATCH v1 3/3] Convert SpinLock* macros to static inline functions.
---
src/include/storage/spin.h | 34 +++++++++++++++++++++-------------
1 file changed, 21 insertions(+), 13 deletions(-)
diff --git a/src/include/storage/spin.h b/src/include/storage/spin.h
index 78c95ae538b..5b680ff5dd8 100644
--- a/src/include/storage/spin.h
+++ b/src/include/storage/spin.h
@@ -5,22 +5,19 @@
*
*
* The interface to spinlocks is defined by the typedef "slock_t" and
- * these macros:
+ * these functions:
*
- * void SpinLockInit(volatile slock_t *lock)
+ * void SpinLockInit(slock_t *lock)
* Initialize a spinlock (to the unlocked state).
*
- * void SpinLockAcquire(volatile slock_t *lock)
+ * void SpinLockAcquire(slock_t *lock)
* Acquire a spinlock, waiting if necessary.
* Time out and abort() if unable to acquire the lock in a
* "reasonable" amount of time --- typically ~ 1 minute.
*
- * void SpinLockRelease(volatile slock_t *lock)
+ * void SpinLockRelease(slock_t *lock)
* Unlock a previously acquired lock.
*
- * Callers must beware that the macro argument may be evaluated multiple
- * times!
- *
* Load and store operations in calling code are guaranteed not to be
* reordered with respect to these operations, because they include a
* compiler barrier. (Before PostgreSQL 9.5, callers needed to use a
@@ -29,9 +26,9 @@
* Keep in mind the coding rule that spinlocks must not be held for more
* than a few instructions. In particular, we assume it is not possible
* for a CHECK_FOR_INTERRUPTS() to occur while holding a spinlock, and so
- * it is not necessary to do HOLD/RESUME_INTERRUPTS() in these macros.
+ * it is not necessary to do HOLD/RESUME_INTERRUPTS() in these functions.
*
- * These macros are implemented in terms of hardware-dependent macros
+ * These functions are implemented in terms of hardware-dependent macros
* supplied by s_lock.h. There is not currently any extra functionality
* added by this header, but there has been in the past and may someday
* be again.
@@ -49,11 +46,22 @@
#include "storage/s_lock.h"
+static inline void
+SpinLockInit(slock_t *lock)
+{
+ S_INIT_LOCK(lock);
+}
-#define SpinLockInit(lock) S_INIT_LOCK(lock)
-
-#define SpinLockAcquire(lock) S_LOCK(lock)
+static inline void
+SpinLockAcquire(slock_t *lock)
+{
+ S_LOCK(lock);
+}
-#define SpinLockRelease(lock) S_UNLOCK(lock)
+static inline void
+SpinLockRelease(slock_t *lock)
+{
+ S_UNLOCK(lock);
+}
#endif /* SPIN_H */
--
2.50.1 (Apple Git-155)
* Re: convert SpinLock* macros to static inline functions
@ 2026-02-18 17:52 Fabrízio de Royes Mello <fabriziomello@gmail.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
1 sibling, 1 reply; 7+ messages in thread
From: Fabrízio de Royes Mello @ 2026-02-18 17:52 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; Andres Freund <andres@anarazel.de>; pgsql-hackers
On Wed, Feb 18, 2026 at 2:28 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:
>
> Starting a new thread from [0].
>
> On Wed, Feb 18, 2026 at 02:17:34AM +0200, Heikki Linnakangas wrote:
> > On 18/02/2026 01:11, Andres Freund wrote:
> >> I think the spinlock functions should also assert this.
> >
> > +1
>
> While working on adding assertions that we are not in a signal handler to
> the spinlock functions, I figured I'd take the opportunity to convert the
> spin.h macros to static inline functions. This was previously discussed
> [1], but AFAICT there was debate over whether to remove S_LOCK and friends
> altogether (which doesn't seem to have happened). But IIUC nobody was
> horribly opposed to $subject, which I think makes sense to do as
> prerequisite work for the aforementioned assertions.
>
+1
> However, as soon as I did this, I got a bunch of build failures because
> various parts of the code still use volatile qualifiers quite liberally.
> It looks like most of these (e.g., see code from commits 2487d872e0,
> 966fb05b58, 4bc15a8bfb, and 4db3744f1f) predate making spinlocks compiler
> barriers (commit 0709b7ee72) or were cargo-culted from code that predated
> it. So, IIUC, it's probably safe to remove these volatile qualifiers now.
> We could alternatively add volatile qualifiers to the new static inline
> function parameters, but that seems like it might just encourage continued
> unnecessary use.
>
Just wondering if there's some code-path that uses it inside
PG_TRY..PG_CATCH that can use longjump.
> I also noticed that SpinLockFree() is unused (and apparently never was).
> There seems to have been agreement back in 2020 to remove it [2], but it
> still exists. I added a prerequisite patch for this, too.
>
Yes, let's remove it.
Regards,
--
Fabrízio de Royes Mello
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: convert SpinLock* macros to static inline functions
@ 2026-02-18 18:03 Nathan Bossart <nathandbossart@gmail.com>
parent: Fabrízio de Royes Mello <fabriziomello@gmail.com>
0 siblings, 1 reply; 7+ messages in thread
From: Nathan Bossart @ 2026-02-18 18:03 UTC (permalink / raw)
To: Fabrízio de Royes Mello <fabriziomello@gmail.com>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; Andres Freund <andres@anarazel.de>; pgsql-hackers
On Wed, Feb 18, 2026 at 02:52:46PM -0300, Fabrízio de Royes Mello wrote:
> On Wed, Feb 18, 2026 at 2:28 PM Nathan Bossart <nathandbossart@gmail.com>
> wrote:
>> However, as soon as I did this, I got a bunch of build failures because
>> various parts of the code still use volatile qualifiers quite liberally.
>> It looks like most of these (e.g., see code from commits 2487d872e0,
>> 966fb05b58, 4bc15a8bfb, and 4db3744f1f) predate making spinlocks compiler
>> barriers (commit 0709b7ee72) or were cargo-culted from code that predated
>> it. So, IIUC, it's probably safe to remove these volatile qualifiers now.
>> We could alternatively add volatile qualifiers to the new static inline
>> function parameters, but that seems like it might just encourage continued
>> unnecessary use.
>
> Just wondering if there's some code-path that uses it inside
> PG_TRY..PG_CATCH that can use longjump.
I didn't notice any such code. For reference, we only need "volatile" in
PG_TRY..PG_CATCH code if a local variable is modified in the PG_TRY section
and used in the PG_CATCH section.
--
nathan
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: convert SpinLock* macros to static inline functions
@ 2026-02-18 19:21 Fabrízio de Royes Mello <fabriziomello@gmail.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 0 replies; 7+ messages in thread
From: Fabrízio de Royes Mello @ 2026-02-18 19:21 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; Andres Freund <andres@anarazel.de>; pgsql-hackers
On Wed, Feb 18, 2026 at 3:03 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:
>
> On Wed, Feb 18, 2026 at 02:52:46PM -0300, Fabrízio de Royes Mello wrote:
> > On Wed, Feb 18, 2026 at 2:28 PM Nathan Bossart <nathandbossart@gmail.com
>
> > wrote:
> >> However, as soon as I did this, I got a bunch of build failures because
> >> various parts of the code still use volatile qualifiers quite
liberally.
> >> It looks like most of these (e.g., see code from commits 2487d872e0,
> >> 966fb05b58, 4bc15a8bfb, and 4db3744f1f) predate making spinlocks
compiler
> >> barriers (commit 0709b7ee72) or were cargo-culted from code that
predated
> >> it. So, IIUC, it's probably safe to remove these volatile qualifiers
now.
> >> We could alternatively add volatile qualifiers to the new static inline
> >> function parameters, but that seems like it might just encourage
continued
> >> unnecessary use.
> >
> > Just wondering if there's some code-path that uses it inside
> > PG_TRY..PG_CATCH that can use longjump.
>
> I didn't notice any such code. For reference, we only need "volatile" in
> PG_TRY..PG_CATCH code if a local variable is modified in the PG_TRY
section
> and used in the PG_CATCH section.
>
Sure sure sure... there's no such condition with your change (took another
look into it).
So your patches LGTM.
--
Fabrízio de Royes Mello
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: convert SpinLock* macros to static inline functions
@ 2026-02-18 21:23 Andres Freund <andres@anarazel.de>
parent: Nathan Bossart <nathandbossart@gmail.com>
1 sibling, 1 reply; 7+ messages in thread
From: Andres Freund @ 2026-02-18 21:23 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; pgsql-hackers
Hi,
On 2026-02-18 11:28:01 -0600, Nathan Bossart wrote:
> While working on adding assertions that we are not in a signal handler to
> the spinlock functions, I figured I'd take the opportunity to convert the
> spin.h macros to static inline functions. This was previously discussed
> [1], but AFAICT there was debate over whether to remove S_LOCK and friends
> altogether (which doesn't seem to have happened). But IIUC nobody was
> horribly opposed to $subject, which I think makes sense to do as
> prerequisite work for the aforementioned assertions.
+1
> However, as soon as I did this, I got a bunch of build failures because
> various parts of the code still use volatile qualifiers quite liberally.
> It looks like most of these (e.g., see code from commits 2487d872e0,
> 966fb05b58, 4bc15a8bfb, and 4db3744f1f) predate making spinlocks compiler
> barriers (commit 0709b7ee72) or were cargo-culted from code that predated
> it. So, IIUC, it's probably safe to remove these volatile qualifiers now.
> We could alternatively add volatile qualifiers to the new static inline
> function parameters, but that seems like it might just encourage continued
> unnecessary use.
I'm all for removing volatile across random parts of the code. However:
Not quite as sure it's the right thing to remove it from SpinLockAcquire()
though. I think removing it more widely would likely cause issues, e.g. if you
removed it from s_lock(), the compiler would be in its right to just turn:
int
s_lock(volatile slock_t *lock, const char *file, int line, const char *func)
...
while (TAS_SPIN(lock))
{
perform_spin_delay(&delayStatus);
}
...
into
...
if (*lock)
{
while (true)
perform_spin_delay(&delayStatus);
}
else
TAS(lock);
...
as without the volatile, or a compiler barrier, the compiler can assume that
nothing will change the the value of *lock (at least theoretically, if it can
prove that perform_spin_delay() doesn't change the value of *lock).
Such a transformation would never lead to a missing memory barrier if the lock
is actually acquired, however it could very well lead to never acquiring the
lock even though it was unlocked concurrently.
You could address this by changing all the TAS_SPIN definitions, but I
probably would just use the SpinLockAcquire() signature with volatile that is
currently used as documentation.
> I also noticed that SpinLockFree() is unused (and apparently never was).
> There seems to have been agreement back in 2020 to remove it [2], but it
> still exists. I added a prerequisite patch for this, too.
+1
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: convert SpinLock* macros to static inline functions
@ 2026-02-19 16:08 Nathan Bossart <nathandbossart@gmail.com>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 7+ messages in thread
From: Nathan Bossart @ 2026-02-19 16:08 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; pgsql-hackers
Thanks for looking.
On Wed, Feb 18, 2026 at 04:23:28PM -0500, Andres Freund wrote:
> Not quite as sure it's the right thing to remove it from SpinLockAcquire()
> though. I think removing it more widely would likely cause issues, e.g. if you
> removed it from s_lock(), the compiler would be in its right to just turn:
>
> int
> s_lock(volatile slock_t *lock, const char *file, int line, const char *func)
> ...
> while (TAS_SPIN(lock))
> {
> perform_spin_delay(&delayStatus);
> }
> ...
>
> into
> ...
> if (*lock)
> {
> while (true)
> perform_spin_delay(&delayStatus);
> }
> else
> TAS(lock);
> ...
>
> as without the volatile, or a compiler barrier, the compiler can assume that
> nothing will change the the value of *lock (at least theoretically, if it can
> prove that perform_spin_delay() doesn't change the value of *lock).
This crossed my mind, but I couldn't see how it could introduce any issues
that don't already exist. Both tas() and s_lock() have the lock parameter
marked as volatile, and S_LOCK is often expanded into functions where the
lock variable is not marked volatile. Why would those existing cases not
be subject to this problem but the static inline function would? And why
would marking SpinLockAcquire()'s lock parameter volatile fix it if doing
so for tas() and s_lock() doesn't? FWIW I don't see any changes to the
code generated for s_lock() with these patches, although I only looked on a
couple of machines.
Perhaps it's still worth doing out of an abundance of caution, or maybe I
am missing something subtle about the liberties that compilers take in this
area...
--
nathan
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: convert SpinLock* macros to static inline functions
@ 2026-02-23 21:33 Nathan Bossart <nathandbossart@gmail.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 0 replies; 7+ messages in thread
From: Nathan Bossart @ 2026-02-23 21:33 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; pgsql-hackers
On Thu, Feb 19, 2026 at 10:08:20AM -0600, Nathan Bossart wrote:
> Perhaps it's still worth doing out of an abundance of caution, or maybe I
> am missing something subtle about the liberties that compilers take in this
> area...
Committed. I ended up abandoning 0001 and adding volatile to the SpinLock*
function signatures, as Andres suggested.
--
nathan
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2026-02-23 21:33 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-02-18 17:28 convert SpinLock* macros to static inline functions Nathan Bossart <nathandbossart@gmail.com>
2026-02-18 17:52 ` Fabrízio de Royes Mello <fabriziomello@gmail.com>
2026-02-18 18:03 ` Nathan Bossart <nathandbossart@gmail.com>
2026-02-18 19:21 ` Fabrízio de Royes Mello <fabriziomello@gmail.com>
2026-02-18 21:23 ` Andres Freund <andres@anarazel.de>
2026-02-19 16:08 ` Nathan Bossart <nathandbossart@gmail.com>
2026-02-23 21:33 ` Nathan Bossart <nathandbossart@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox