Horton

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: pgsql-hackers@lists.postgresql.org
Date: 2020-06-05T02:33:02Z
Lists: pgsql-hackers 
Hi,

On 2020-06-04 15:07:34 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I'd still like to know which problem we're actually trying to solve
> > here. I don't understand the "error" issues you mentioned upthread.
>
> If you error out of getting the inner spinlock, the outer spinlock
> is stuck, permanently, because there is no mechanism for spinlock
> release during transaction abort.  Admittedly it's not very likely
> for the inner acquisition to fail, but it's possible.

We PANIC on stuck spinlocks, so I don't think that's a problem.


>  * We map all spinlocks onto a set of NUM_SPINLOCK_SEMAPHORES semaphores.
>  * It's okay to map multiple spinlocks onto one semaphore because no process
>  * should ever hold more than one at a time.
>
> You've falsified that argument ...  and no, I don't want to upgrade
> the spinlock infrastructure enough to make this OK.

Well, theoretically we take care to avoid this problem. That's why we
have a separate define for spinlocks and atomics:

/*
 * When we don't have native spinlocks, we use semaphores to simulate them.
 * Decreasing this value reduces consumption of OS resources; increasing it
 * may improve performance, but supplying a real spinlock implementation is
 * probably far better.
 */
#define NUM_SPINLOCK_SEMAPHORES		128

/*
 * When we have neither spinlocks nor atomic operations support we're
 * implementing atomic operations on top of spinlock on top of semaphores. To
 * be safe against atomic operations while holding a spinlock separate
 * semaphores have to be used.
 */
#define NUM_ATOMICS_SEMAPHORES		64

and

#ifndef HAVE_SPINLOCKS

	/*
	 * NB: If we're using semaphore based TAS emulation, be careful to use a
	 * separate set of semaphores. Otherwise we'd get in trouble if an atomic
	 * var would be manipulated while spinlock is held.
	 */
	s_init_lock_sema((slock_t *) &ptr->sema, true);
#else
	SpinLockInit((slock_t *) &ptr->sema);
#endif

But it looks like that code is currently buggy (and looks like it always
has been), because we don't look at the nested argument when
initializing the semaphore.  So we currently allocate too many
semaphores, without benefiting from them :(.


> We shouldn't ever be holding spinlocks long enough, or doing anything
> complicated enough inside them, for such an upgrade to have merit.

Well, I don't think atomic instructions are that complicated.  And I
think prohibiting atomics-within-spinlock adds a problematic
restriction, without much in the way of benefits:

There's plenty things where it's somewhat easy to make the fast-path
lock-free, but the slow path still needs a lock (e.g. around a
freelist). And for those it's really useful to still be able to have a
coherent update to an atomic variable, to synchronize with the fast-path
that doesn't take the spinlock.

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.