Horton

Re: global barrier & atomics in signal handlers (Re: Atomic operations within spinlocks)

Andres Freund <andres@anarazel.de>

View thread View on postgr.es
From: Andres Freund <andres@anarazel.de>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Robert Haas <robertmhaas@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2020-06-18T18:30:41Z
Lists: pgsql-hackers 
Hi,

On 2020-06-18 12:29:40 -0400, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Thu, Jun 18, 2020 at 11:59 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Sure, but wouldn't making the SpinLockAcquire layer into static inlines be
> >> sufficient to address that point, with no need to touch s_lock.h at all?
> 
> > I mean, wouldn't you then end up with a bunch of 1-line functions
> > where you can step into the function but not through whatever
> > individual things it does?
> 
> Not following your point.  The s_lock.h implementations tend to be either
> simple C statements ("*lock = 0") or asm blocks; if you feel a need to
> step through them you're going to be resorting to "si" anyway.

I agree on that.


I do think it'd be better to not have the S_LOCK macro though (but have
TAS/TAS_SPIN as we do now). And instead have SpinLockAcquire() call
s_lock() (best renamed to something a bit more meaningful). Makes the
code a bit easier to understand (no S_LOCK vs s_lock) and yields simpler
macros.

There's currently no meaningful ifdefs for S_LOCK (in contrast to
e.g. S_UNLOCK), so I don't see it making s_lock.h more complicated.

I think part of the issue here is that the naming of the s_lock exposed
macros is confusing. We have S_INIT_LOCK, TAS, SPIN_DELAY, TAS,
TAS_SPIN, S_UNLOCK, S_LOCK_FREE that are essentially hardware
dependent. But then there's S_LOCK which basically isn't.

It may have made some sense when the file was originally written, if one
imagines that S_ is the only external API, and the rest is
implementation. But given that s_lock() uses TAS() directly (and says
"platform-independent portion of waiting for a spinlock"), and that we
only have one definition of S_UNLOCK that doesn't seem quite right.


> I think the main usefulness of doing anything here would be (a) separating
> the spinlock infrastructure from callers and (b) ensuring that we have a
> declared argument type, and single-evaluation semantics, for the spinlock
> function parameters.  Both of those are adequately addressed by fixing
> spin.h, IMO anyway.

The abstraction point made me grep for includes of s_lock.h. Seems we
have some unnecessary includes of s_lock.h.

lwlock.h doesn't need to include spinlock related things anymore, and
hasn't for years, the spinlocks are replaced with atomics. That seems
pretty obvious. I'm gonna fix that in master, unless somebody thinks we
should do that more widely?

But we also have s_lock.h includes in condition_variable.h and
main.c. Seems the former should instead include spin.h and the latter
include should just be removed?


All of this however makes me wonder whether it's worth polishing
spinlocks instead of just ripping them out. Obviously we'd still need a
fallback for atomics, but it not be hard to just replace the
spinlock use with either "smaller" atomics or semaphores.

Greetings,

Andres Freund

Commits

  1. Convert SpinLock* macros to static inline functions.

  2. Clean up includes of s_lock.h.

  3. Fix deadlock danger when atomic ops are done under spinlock.

  4. Add basic spinlock tests to regression tests.

  5. spinlock emulation: Fix bug when more than INT_MAX spinlocks are initialized.

  6. Avoid potential spinlock in a signal handler as part of global barriers.

  7. Make pg_stat_wal_receiver consistent with the WAL receiver's shmem info

  8. Improve 64bit atomics support.