Re: Atomic operations within spinlocks

Hi,

On 2020-06-05 21:01:56 -0400, Tom Lane wrote:
> > I'm not really sure what to do about that issue. The easisest thing
> > would probably be to change the barrier generation to 32bit (which
> > doesn't have to use locks for reads in any situation).
>
> Yeah, I think we need a hard rule that you can't use a spinlock in
> an interrupt handler --- which means no atomics that don't have
> non-spinlock implementations on every platform.

Yea, that might be the easiest thing to do. The only other thing I can
think of would be to mask all signals for the duration of the
atomic-using-spinlock operation. That'd make the fallback noticably more
expensive, but otoh, do we care enough?

I think a SIGNAL_HANDLER_BEGIN(); SIGNAL_HANDLER_END(); to back an
Assert(!InSignalHandler()); could be quite useful. Could also save
errno etc.

> At some point I think we'll have to give up --disable-spinlocks; it's
> really of pretty marginal use (how often does anyone port PG to a new
> CPU type?) and the number of weird interactions it adds in this area
> seems like more than it's worth.

Indeed. And any new architecture one would port PG to would have good
enough compiler intrinsics to make that trivial. I still think it'd make
sense to have a fallback implementation using compiler intrinsics...

And I think we should just require 32bit atomics at the same time. Would
probably kill gaur though.

I did just find a longstanding bug in the spinlock emulation code:

void
s_init_lock_sema(volatile slock_t *lock, bool nested)
{
static int counter = 0;

*lock = ((++counter) % NUM_SPINLOCK_SEMAPHORES) + 1;
}

void
s_unlock_sema(volatile slock_t *lock)
{
int lockndx = *lock;

if (lockndx <= 0 || lockndx > NUM_SPINLOCK_SEMAPHORES)
elog(ERROR, "invalid spinlock number: %d", lockndx);
PGSemaphoreUnlock(SpinlockSemaArray[lockndx - 1]);
}

I don't think it's ok that counter is a signed integer... While it maybe
used to be unlikely that we ever have that many spinlocks, I don't think
it's that hard anymore, because we dynamically allocate them for a lot
of parallel query stuff. A small regression test that initializes
enough spinlocks indeed errors out with
2020-06-05 18:08:29.110 PDT [734946][3/2:0] ERROR: invalid spinlock number: -126
2020-06-05 18:08:29.110 PDT [734946][3/2:0] STATEMENT: SELECT test_atomic_ops();

> > Randomly noticed while looking at the code:
> > uint64 flagbit = UINT64CONST(1) << (uint64) type;
>
> I'm surprised we didn't get any compiler warnings about that.

Unfortunately I don't think one can currently compile postgres with
warnings for "implicit casts" enabled :(.

> But of course requiring 64-bit atomics is still a step too far.

If we had a 32bit compare-exchange it ought to be possible to write a
signal-safe emulation of 64bit atomics. I think. Something *roughly*
like:

typedef struct pg_atomic_uint64
{
/*
* Meaning of state bits:
* 0-1: current valid
* 2-4: current proposed
* 5: in signal handler
* 6-31: pid of proposer
*/
pg_atomic_uint32 state;

/*
* One current value, two different proposed values.
*/
uint64 value[3];
} pg_atomic_uint64;

The update protocol would be something roughly like:

bool
pg_atomic_compare_exchange_u64_impl(volatile pg_atomic_uint64 *ptr, uint64 *expected, uint64 newval)
{
while (true)
{
uint32 old_state = pg_atomic_read_u32(&ptr->state);
uint32 updater_pid = PID_FROM_STATE(old_state);
uint32 new_state;
uint64 old_value;

int proposing;

/*
* Value changed, so fail. This is obviously racy, but we'll
* notice concurrent updates later.
*/
if (ptr->value[VALID_FIELD(old_state)] != *expected)
{
return false;
}

if (updater_pid == INVALID_PID)
{

new_state = old_state;

/* signal that current process is updating */
new_state |= MyProcPid >> PID_STATE_SHIFT;
if (InSignalHandler)
new_state |= PROPOSER_IN_SIGNAL_HANDLER_BIT;

/* set which index is being proposed */
new_state = (new_state & ~PROPOSER_BITS) |
NEXT_PROPOSED_FIELD(old_state, &proposing);

/*
* If we successfully can update state to contain our new
* value, we have a right to do so, and can only be
* interrupted by ourselves, in a signal handler.
*/
if (!pg_atomic_compare_exchange(&ptr->state, &old_state, new_state))
{
/* somebody else updated, restart */
continue;
}

old_state = new_state;

/*
* It's ok to compare the values now. If we are interrupted
* by a signal handler, we'll notice when updating
* state. There's no danger updating the same proposed value
* in two processes, because they they always would get
* offsets to propse into.
*/
ptr->value[proposing] = newval;

/* set the valid field to the one we just filled in */
new_state = (new_state & ~VALID_FIELD_BITS) | proposed;
/* remove ourselve as updater */
new_state &= UPDATER_BITS;

if (!pg_atomic_compare_exchange(&ptr->state, &old_state, new_state))
{
/*
* Should only happen when we were interrupted by this
* processes' handler.
*/
Assert(!InSignalHandler);

/*
* Signal handler must have cleaned out pid as updater.
*/
Assert(PID_FROM_STATE(old_state) != MyProcPid);
continue;
}
else
{
return true;
}
}
else if (PID_FROM_STATE(current_state) == MyProcPid)
{
/*
* This should only happen when in a signal handler. We don't
* currently allow nesting of signal handlers.
*/
Assert(!(current_state & PROPOSER_IN_SIGNAL_HANDLER_BIT));

/* interrupt our own non-signal-handler update */
new_state = old_state | PROPOSER_IN_SIGNAL_HANDLER_BIT;

/* set which index is being proposed */
new_state = (new_state & ~PROPOSER_BITS) |
NEXT_PROPOSED_FIELD(old_state, &proposing);

// FIXME: assert that previous value still was what we assumed
pg_atomic_exchange_u32(&ptr_state.state, new_state);
}
else
{
do
{
pg_spin_delay();

current_state = pg_atomic_read_u32(&ptr->state);
} while (PID_FROM_STATE(current_state) != INVALID_PID)
}
}
}

While that's not trivial, it'd not be that expensive. The happy path
would be two 32bit atomic operations to simulate a 64bit one.

Greetings,

Andres Freund