Re: Atomic operations within spinlocks

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