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
