+ 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.
+ /*
+ * 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.
Overall, I think this patch is in reasonable shape.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
