On Tue, Jul 30, 2024 at 9:50 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> I guess we should also consider reimplementing the spinlock on the
> atomic API, but I can see that Andres is poking at spinlock code right
> now so I'll keep out of his way...
Here is a first attempt at that. I haven't compared the generated asm
yet, but it seems to work OK. I solved some mysteries (or probably
just rediscovered things that others already knew) along the way:
1. The reason we finished up with OK-looking MSVC atomics code that
was probably never actually reachable might be that it was
copied-and-pasted from the spinlock code. This patch de-duplicates
that (and much more).
2. The pg_atomic_unlocked_test_flag() function was surprising to me:
it returns true if it's not currently set (according to a relaxed
load). Most of this patch was easy, but figuring out that I had
reverse polarity here was a multi-coffee operation :-) I can't call
it wrong though, as it's not based on <stdatomic.h>, and it's clearly
documented, so *shrug*.
3. As for why we have a function that <stdatomic.h> doesn't, I
speculate that it might have been intended for implementing this exact
patch, ie wanting to perform that relaxed load while spinning as
recommended by Intel. (If we strictly had to use <stdatomic.h>
functions, we couldn't use atomic_flag due to the lack of a relaxed
load operation on that type, so we'd probably have to use atomic_char
instead. Perhaps one day we will cross that bridge.)
4. Another reason would be that you need it to implement
SpinLockFree() and S_LOCK_FREE(). They don't seem to have had any
real callers since the beginning of open source PostgreSQL!, except
for a test of limited value in a new world without ports developing
their own spinlock code. Let's remove them! I see this was already
threatened by Andres in 3b37a6de.
Archeological notes: I went back further and found that POSTGRES 4.2
used them only twice for assertions. These S_LOCK() etc interfaces
seem to derive from Dynix's parallel programming library, but it
didn't have S_LOCK_FREE() either. It looks like the Berkeley guys
added _FREE() for *internal* use when dealing with PA-RISC, where free
spinlocks were non-zero, but we later developed a different way of
dealing with that.
