Re: sinvaladt.c: remove msgnumLock, use atomic operations on maxMsgNum

Hi,

On 2025-03-21 14:35:16 +0300, Yura Sokolov wrote:
> From 080c9e0de5e6e10751347e1ff50b65df424744cb Mon Sep 17 00:00:00 2001
> From: Yura Sokolov <y(dot)sokolov(at)postgrespro(dot)ru>
> Date: Mon, 3 Feb 2025 11:58:33 +0300
> Subject: [PATCH v2] sinvaladt.c: use atomic operations on maxMsgNum
>
> msgnumLock spinlock could be highly contended.
> Comment states it was used as memory barrier.
> Lets use atomic ops with memory barriers directly instead.
>
> Note: patch uses pg_read_barrier()/pg_write_barrier() instead of
> pg_atomic_read_membarrier_u32()/pg_atomic_write_membarrier_u32() since
> no full barrier semantic is required, and explicit read/write barriers
> are cheaper at least on x86_64.

Is it actually true that full barriers aren't required? I think we might
actually rely on a stronger ordering.

> @@ -506,10 +493,9 @@ SIGetDataEntries(SharedInvalidationMessage *data, int datasize)
> */
> stateP->hasMessages = false;
>
> - /* Fetch current value of maxMsgNum using spinlock */
> - SpinLockAcquire(&segP->msgnumLock);
> - max = segP->maxMsgNum;
> - SpinLockRelease(&segP->msgnumLock);
> + /* Fetch current value of maxMsgNum using atomic with memory barrier */
> + max = pg_atomic_read_u32(&segP->maxMsgNum);
> + pg_read_barrier();
>
> if (stateP->resetState)
> {
> /*
> * Force reset. We can say we have dealt with any messages added
> * since the reset, as well; and that means we should clear the
> * signaled flag, too.
> */
> stateP->nextMsgNum = max;
> stateP->resetState = false;
> stateP->signaled = false;
> LWLockRelease(SInvalReadLock);
> return -1;
> }

vs

> @@ -410,17 +398,16 @@ SIInsertDataEntries(const SharedInvalidationMessage *data, int n)
> /*
> * Insert new message(s) into proper slot of circular buffer
> */
> - max = segP->maxMsgNum;
> + max = pg_atomic_read_u32(&segP->maxMsgNum);
> while (nthistime-- > 0)
> {
> segP->buffer[max % MAXNUMMESSAGES] = *data++;
> max++;
> }
>
> - /* Update current value of maxMsgNum using spinlock */
> - SpinLockAcquire(&segP->msgnumLock);
> - segP->maxMsgNum = max;
> - SpinLockRelease(&segP->msgnumLock);
> + /* Update current value of maxMsgNum using atomic with memory barrier */
> + pg_write_barrier();
> + pg_atomic_write_u32(&segP->maxMsgNum, max);
>
> /*
> * Now that the maxMsgNum change is globally visible, we give everyone
> * a swift kick to make sure they read the newly added messages.
> * Releasing SInvalWriteLock will enforce a full memory barrier, so
> * these (unlocked) changes will be committed to memory before we exit
> * the function.
> */
> for (i = 0; i < segP->numProcs; i++)
> {
> ProcState *stateP = &segP->procState[segP->pgprocnos[i]];
>
> stateP->hasMessages = true;
> }

On a loosely ordered architecture, the hasMessage=false in SIGetDataEntries()
could be reordered with the read of maxMsgNum. Which, afaict, would lead to
missing messages. That's not prevented by the pg_write_barrier() in
SIInsertDataEntries(). I think there may be other similar dangers.

This could be solved by adding full memory barriers in a few places. But:

I'd also like to know a bit more about the motivation here - I can easily
believe that you hit contention around the shared inval queue, but I find it
somewhat hard to believe that a spinlock that's acquired *once* per batch
("quantum"), covering a single read/write, is going to be the bottleneck,
rather than the much longer held LWLock, that protects iterating over all
procs.

Have you verified that this actually addresses the performance issue?

Greetings,

Andres Freund