Обсуждение: convert SpinLock* macros to static inline functions

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#160ac60fddc7934777a0d20b9183d840
[2] https://postgr.es/m/flat/20200608225338.m5zho424w6lpwb2d%40alap3.anarazel.de

-- 
nathan

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

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

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