small cleanup for s_lock.h

On Mon May 4, 2026 at 4:50 PM CDT, Nathan Bossart wrote:

I noticed that s_lock.h points to a default implementation of tas() in
tas.s or s_lock.c, but AFAICT there hasn't been a tas() implementation in
s_lock.c since commit 718aa43a4e, and commit 25f36066dd seems to have
removed the last remaining tas.s files. So, I think this is dead code.

I also noticed that HAS_TEST_AND_SET just means that TAS is defined, so I
wrote a 0002 that removes it in favor of checking TAS directly. I'd like
to rewrite the comment at the top of the file, too, but haven't gotten to
that yet. I find it a little misleading, especially because we #error if
TAS isn't defined.

This looks pretty reasonable to me.

--
Tristan Partin
PostgreSQL Contributors Team
AWS (https://aws.amazon.com)

Nathan Bossart <nathandbossart@gmail.com> writes:

I noticed that s_lock.h points to a default implementation of tas() in
tas.s or s_lock.c, but AFAICT there hasn't been a tas() implementation in
s_lock.c since commit 718aa43a4e, and commit 25f36066dd seems to have
removed the last remaining tas.s files. So, I think this is dead code.

It is, but I think the 0001 patch should be more like

 #if !defined(TAS)
-extern int	tas(volatile slock_t *lock);		/* in port/.../tas.s, or
-							 * s_lock.c */
-
-#define TAS(lock)		tas(lock)
+#error "must provide a spinlock implementation"
 #endif	 /* TAS */

Perhaps this could be merged with the earlier bit about erroring
if not HAS_TEST_AND_SET.

I also noticed that HAS_TEST_AND_SET just means that TAS is defined, so I
wrote a 0002 that removes it in favor of checking TAS directly.

I'm pretty much -1 on that; HAS_TEST_AND_SET is clearer than TAS, and
removing it seems quite likely to break someone's code. We could
perhaps collect all the separate instances into this end location:

#if defined(TAS)
#define HAS_TEST_AND_SET
#else
#error "must provide a spinlock implementation"
#endif /* TAS */

I'd like
to rewrite the comment at the top of the file, too, but haven't gotten to
that yet. I find it a little misleading, especially because we #error if
TAS isn't defined.

No objection in principle to improving that comment, but what did you
have in mind exactly?

regards, tom lane

On Tue, 5 May 2026 at 02:49, Nathan Bossart <nathandbossart@gmail.com> wrote:

I noticed that s_lock.h points to a default implementation of tas() in
tas.s or s_lock.c, but AFAICT there hasn't been a tas() implementation in
s_lock.c since commit 718aa43a4e, and commit 25f36066dd seems to have
removed the last remaining tas.s files. So, I think this is dead code.

This indeed looks like a dead code. I also noticed `tas.s` is present
in meson.build, gitignore and src/backend/Makefile
should we remove that too?

I also noticed that HAS_TEST_AND_SET just means that TAS is defined, so I
wrote a 0002 that removes it in favor of checking TAS directly. I'd like
to rewrite the comment at the top of the file, too, but haven't gotten to
that yet. I find it a little misleading, especially because we #error if
TAS isn't defined.

--
nathan

--
Best regards,
Kirill Reshke

On Mon, May 04, 2026 at 06:16:47PM -0400, Tom Lane wrote:

Nathan Bossart <nathandbossart@gmail.com> writes:

I'd like to rewrite the comment at the top of the file, too, but haven't
gotten to that yet. I find it a little misleading, especially because
we #error if TAS isn't defined.

No objection in principle to improving that comment, but what did you
have in mind exactly?

I think the way the comment presents the macros gives a potentially
misleading impression about what you typically need to do to get a new
platform working, and you basically need to read through the whole file to
make sense of what's going on. Some of the macros it mentions have a
default implementation that we use everywhere (e.g., S_INIT_LOCK), and if
you're using gcc, you may be able to just use the __sync_lock_test_and_set
versions. If you _did_ need to add a new section for a new platform, you'd
probably be more interested in defining slock_t, HAS_TEST_AND_TEST/TAS,
S_UNLOCK, SPIN_DELAY, and maybe TAS_SPIN. In fact, you _must_ ensure TAS
is defined or else we'll fail to compile.

Although as I write this e-mail and think about how exactly I'd rewrite the
comment, I grow less confident about doing so...

--
nathan

On Mon, May 04, 2026 at 06:16:47PM -0400, Tom Lane wrote:

Nathan Bossart <nathandbossart@gmail.com> writes:

I noticed that s_lock.h points to a default implementation of tas() in
tas.s or s_lock.c, but AFAICT there hasn't been a tas() implementation in
s_lock.c since commit 718aa43a4e, and commit 25f36066dd seems to have
removed the last remaining tas.s files. So, I think this is dead code.

It is, but I think the 0001 patch should be more like

#if !defined(TAS)
-extern int	tas(volatile slock_t *lock);		/* in port/.../tas.s, or
-							 * s_lock.c */
-
-#define TAS(lock)		tas(lock)
+#error "must provide a spinlock implementation"
#endif	 /* TAS */

Perhaps this could be merged with the earlier bit about erroring
if not HAS_TEST_AND_SET.

I also noticed that HAS_TEST_AND_SET just means that TAS is defined, so I
wrote a 0002 that removes it in favor of checking TAS directly.

I'm pretty much -1 on that; HAS_TEST_AND_SET is clearer than TAS, and
removing it seems quite likely to break someone's code. We could
perhaps collect all the separate instances into this end location:

#if defined(TAS)
#define HAS_TEST_AND_SET
#else
#error "must provide a spinlock implementation"
#endif /* TAS */

Okay, here's a new version of the patch that I believe addresses both
points.

--
nathan

Nathan Bossart <nathandbossart@gmail.com> writes:

Okay, here's a new version of the patch that I believe addresses both
points.

This seems cleaner than what we have ... but ...

After thinking some more I realized that what's confusing us here
is an API-level problem. s_lock.h's header comment says

* Usually, S_LOCK() is implemented in terms of even lower-level macros
* TAS() and TAS_SPIN():

As things stand, we have no platforms where that's not the case,
and so we've lost sight of the fact that the contract shouldn't be
"you must provide TAS()". It should be "you must either provide
S_LOCK(), or provide TAS() to base it on".

A rough cut as to the right way to do this is attached. The
main loose end here is that it's not very clear what s_lock.c's
s_lock() should do if there's no TAS (and hence no TAS_SPIN).
Maybe we should just not compile that function at all without
TAS; if a platform provides a non-default S_LOCK that needs a
helper function, it's on the platform to supply that helper.

Also, after noting that HAS_TEST_AND_SET is referenced nowhere
outside s_lock.h, I'm coming around to your previous position
that it's redundant and we should drop it. This is mainly because
it's not clear to me whether it should be set on a platform that
provides S_LOCK but not TAS. I didn't touch that here though.

Lastly, I definitely agree now that the file's header comment needs
some work. Maybe this insight helps you with that? (One thing
I noticed is that the ending comment about "Equivalent OS-supplied
mutex routines could be used too" feels pretty obsolete. Maybe
instead, "Equivalent compiler intrinsics are another popular option".)

regards, tom lane

On Tue, May 05, 2026 at 12:56:09PM -0400, Tom Lane wrote:

After thinking some more I realized that what's confusing us here
is an API-level problem. s_lock.h's header comment says

* Usually, S_LOCK() is implemented in terms of even lower-level macros
* TAS() and TAS_SPIN():

As things stand, we have no platforms where that's not the case,
and so we've lost sight of the fact that the contract shouldn't be
"you must provide TAS()". It should be "you must either provide
S_LOCK(), or provide TAS() to base it on".

A rough cut as to the right way to do this is attached. The
main loose end here is that it's not very clear what s_lock.c's
s_lock() should do if there's no TAS (and hence no TAS_SPIN).
Maybe we should just not compile that function at all without
TAS; if a platform provides a non-default S_LOCK that needs a
helper function, it's on the platform to supply that helper.

That makes sense to me.

Also, after noting that HAS_TEST_AND_SET is referenced nowhere
outside s_lock.h, I'm coming around to your previous position
that it's redundant and we should drop it. This is mainly because
it's not clear to me whether it should be set on a platform that
provides S_LOCK but not TAS. I didn't touch that here though.

Lastly, I definitely agree now that the file's header comment needs
some work. Maybe this insight helps you with that? (One thing
I noticed is that the ending comment about "Equivalent OS-supplied
mutex routines could be used too" feels pretty obsolete. Maybe
instead, "Equivalent compiler intrinsics are another popular option".)

I think it does help, thanks. I'll give it a whirl.

--
nathan

On Tue, May 05, 2026 at 12:57:10PM -0500, Nathan Bossart wrote:

On Tue, May 05, 2026 at 12:56:09PM -0400, Tom Lane wrote:

Lastly, I definitely agree now that the file's header comment needs
some work. Maybe this insight helps you with that? (One thing
I noticed is that the ending comment about "Equivalent OS-supplied
mutex routines could be used too" feels pretty obsolete. Maybe
instead, "Equivalent compiler intrinsics are another popular option".)

I think it does help, thanks. I'll give it a whirl.

Here is a first try. While at it, I removed the note about using
"volatile" prior to v9.5, as well as the note about using TAS() in a loop.
I couldn't really understand why the latter part needs to be said out loud,
so I did some more digging. It was added by commit 7f60b81e and refers to
an unsupported platform. At the time, it was apparently normal to use
TAS() outside of s_lock.h, but the "NOT part of the API" note above it was
added the following year in commit 499abb0c.

Two other things I noticed after staring at s_lock.h for a while:

* If we're willing to define TAS_SPIN to first do an unlocked test
everywhere, we can simplify the ARM and x86 code. Specifically, we can
merge the x86 blocks together, and we can remove all but the SPIN_DELAY
definition for AArch64. We've thus far been hesistant to add the unlocked
test to platforms without evidence of improvements, but I'm a little
skeptical that folks have performance-critical workloads on architectures
that don't already have it.

* Furthermore, do we really need architecture-specific implementations of
anything except for SPIN_DELAY()? I suspect that
__sync_lock_test_and_set() and __sync_lock_release() work pretty well most
of the time, and they've been available in gcc and clang for ~20 years.
Perhaps we could even start using the __atomic builtins if available.
We've been slowly trying to move away from spinlocks, anyway, and I'm not
seeing huge differences in generated code on https://godbolt.org/ for newer
compiler versions.

Of course, further research and benchmarking would be needed, but I figured
I'd at least jot down these thoughts.

--
nathan

On Tue, May 05, 2026 at 03:20:46AM +0500, Kirill Reshke wrote:

This indeed looks like a dead code. I also noticed `tas.s` is present
in meson.build, gitignore and src/backend/Makefile
should we remove that too?

I forgot to remove these in v3. Here's a new patch set with that taken
care of.

--
nathan

On Thu, May 07, 2026 at 03:41:56PM -0500, Nathan Bossart wrote:

+/*
+ * We can only define TAS_SPIN if TAS was defined.  Otherwise, the platform
+ * defined its own S_LOCK without TAS, and therefore is responsible for
+ * defining its own TAS_SPIN as well.  (Note that we currently do not have any
+ * platforms that don't define TAS.)
+ */
#if !defined(TAS_SPIN)
+#if defined(TAS)
#define TAS_SPIN(lock)	TAS(lock)
-#endif	 /* TAS_SPIN */
+#else
+#error Neither TAS nor TAS_SPIN defined on this platform.  Please report this to pgsql-bugs@lists.postgresql.org.
+#endif	 /* TAS */
+#endif	 /* ! TAS_SPIN */

Wait, this isn't right. TAS_SPIN is only used by s_lock(), which is only
used by the default S_LOCK. We should just not compile s_lock() if the
platform defines its own S_LOCK, and we shouldn't #error here if TAS is not
defined.

--
nathan

On Thu, May 07, 2026 at 04:12:09PM -0500, Nathan Bossart wrote:

On Thu, May 07, 2026 at 03:41:56PM -0500, Nathan Bossart wrote:

+/*
+ * We can only define TAS_SPIN if TAS was defined.  Otherwise, the platform
+ * defined its own S_LOCK without TAS, and therefore is responsible for
+ * defining its own TAS_SPIN as well.  (Note that we currently do not have any
+ * platforms that don't define TAS.)
+ */
#if !defined(TAS_SPIN)
+#if defined(TAS)
#define TAS_SPIN(lock)	TAS(lock)
-#endif	 /* TAS_SPIN */
+#else
+#error Neither TAS nor TAS_SPIN defined on this platform.  Please report this to pgsql-bugs@lists.postgresql.org.
+#endif	 /* TAS */
+#endif	 /* ! TAS_SPIN */

Wait, this isn't right. TAS_SPIN is only used by s_lock(), which is only
used by the default S_LOCK. We should just not compile s_lock() if the
platform defines its own S_LOCK, and we shouldn't #error here if TAS is not
defined.

Should be fixed in v5, sorry for the noise.

--
nathan