Re: Remove last traces of HPPA support

On Wed, Jul 31, 2024 at 8:47 PM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> On 31/07/2024 08:52, Thomas Munro wrote:
> The old __i386__ implementation of TAS() said:
>
> > * When this was last tested, we didn't have separate TAS() and TAS_SPIN()
> > * macros. Nowadays it probably would be better to do a non-locking test
> > * in TAS_SPIN() but not in TAS(), like on x86_64, but no-one's done the
> > * testing to verify that. Without some empirical evidence, better to
> > * leave it alone.
>
> It seems that you did what the comment suggested. That seems fine. For
> sake of completeness, if someone has an i386 machine lying around, it
> would be nice to verify that. Or an official CPU manufacturer's
> implementation guide, or references to other implementations or something.

Hmm, the last "real" 32 bit CPU is from ~20 years ago. Now the only
32 bit x86 systems we should nominally care about are modern CPUs that
can also run 32 bit instruction; is there a reason to think they'd
behave differently at this level? Looking at the current Intel
optimisation guide's discussion of spinlock implementation at page
2-34 of [1], it doesn't distinguish between 32 and 64, and it has that
double-check thing.

> > - * On platforms with weak memory ordering, the TAS(), TAS_SPIN(), and
> > - * S_UNLOCK() macros must further include hardware-level memory fence
> > - * instructions to prevent similar re-ordering at the hardware level.
> > - * TAS() and TAS_SPIN() must guarantee that loads and stores issued after
> > - * the macro are not executed until the lock has been obtained. Conversely,
> > - * S_UNLOCK() must guarantee that loads and stores issued before the macro
> > - * have been executed before the lock is released.
>
> That old comment means that both SpinLockAcquire() and SpinLockRelease()
> acted as full memory barriers, and looking at the implementations, that
> was indeed so. With the new implementation, SpinLockAcquire() will have
> "acquire semantics" and SpinLockRelease will have "release semantics".
> That's very sensible, and I don't believe it will break anything, but
> it's a change in semantics nevertheless.

Yeah. It's interesting that our pg_atomic_clear_flag(f) is like
standard atomic_flag_clear_explicit(f, memory_order_release), not like
atomic_flag_clear(f) which is short for atomic_flag_clear_explicit(f,
memory_order_seq_cst). Example spinlock code I've seen written in
modern C or C++ therefore uses the _explicit variants, so it can get
acquire/release, which is what people usually want from a lock-like
thing. What's a good way to test the performance in PostgreSQL? In a
naive loop that just test-and-sets and clears a flag a billion times
in a loop and does nothing else, I see 20-40% performance increase
depending on architecture when comparing _seq_cst with
_acquire/_release. You're right that this semantic change deserves
explicit highlighting, in comments somewhere... I wonder if we have
anywhere that is counting on the stronger barrier...

[1] https://www.intel.com/content/www/us/en/content-details/671488/intel-64-and-ia-32-architectures-optimization-reference-manual-volume-1.html