On Thu, Feb 9, 2023 at 3:36 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> + pg_atomic_exchange_u64(valptr, val);
>
> nitpick: I'd add a (void) at the beginning of these calls to
> pg_atomic_exchange_u64() so that it's clear that we are discarding the
> return value.
I did that in the attached v5 patch although it's a mix elsewhere;
some doing explicit return value cast with (void) and some not.
> + /*
> + * Update the lock variable atomically first without having to acquire wait
> + * list lock, so that if anyone looking for the lock will have chance to
> + * grab it a bit quickly.
> + *
> + * NB: Note the use of pg_atomic_exchange_u64 as opposed to just
> + * pg_atomic_write_u64 to update the value. Since pg_atomic_exchange_u64 is
> + * a full barrier, we're guaranteed that the subsequent atomic read of lock
> + * state to check if it has any waiters happens after we set the lock
> + * variable to new value here. Without a barrier, we could end up missing
> + * waiters that otherwise should have been woken up.
> + */
> + pg_atomic_exchange_u64(valptr, val);
> +
> + /*
> + * Quick exit when there are no waiters. This avoids unnecessary lwlock's
> + * wait list lock acquisition and release.
> + */
> + if ((pg_atomic_read_u32(&lock->state) & LW_FLAG_HAS_WAITERS) == 0)
> + return;
>
> I think this makes sense. A waiter could queue itself after the exchange,
> but it'll recheck after queueing. IIUC this is basically how this works
> today. We update the value and release the lock before waking up any
> waiters, so the same principle applies.
Yes, a waiter right after self-queuing (LWLockQueueSelf) checks for
the value (LWLockConflictsWithVar) before it goes and waits until
awakened in LWLockWaitForVar. A waiter added to the queue is
guaranteed to be woken up by the
LWLockUpdateVar but before that the lock value is set and we have
pg_atomic_exchange_u64 as a memory barrier, so no memory reordering.
Essentially, the order of these operations aren't changed. The benefit
that we're seeing is from avoiding LWLock's waitlist lock for reading
and updating the lock value relying on 64-bit atomics.
> Overall, I think this patch is in reasonable shape.
Thanks for reviewing. Please see the attached v5 patch.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com