Re: small cleanup for s_lock.h

Nathan Bossart <nathandbossart(at)gmail(dot)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