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

On 03/02/2025 13:05, Yura Sokolov wrote:
> Investigating some performance issues of a client, our engineers found
> msgnumLock to be contended.
>
> Looking closer it is obvious it is not needed at all: it used only as
> memory barrier. It is even stated in comment at file start:
>
> * We deal with that by having a spinlock that readers must take for just
> * long enough to read maxMsgNum, while writers take it for just long enough
> * to write maxMsgNum. (The exact rule is that you need the spinlock to
> * read maxMsgNum if you are not holding SInvalWriteLock, and you need the
> * spinlock to write maxMsgNum unless you are holding both locks.)
> *
> * Note: since maxMsgNum is an int and hence presumably atomically readable/
> * writable, the spinlock might seem unnecessary. The reason it is needed
> * is to provide a memory barrier: we need to be sure that messages written
> * to the array are actually there before maxMsgNum is increased, and that
> * readers will see that data after fetching maxMsgNum.
>
> So we changed maxMsgNum to be pg_atomic_uint32, and put appropriate memory
> barriers:
> - pg_write_barrier() before write to maxMsgNum (when only SInvalWriteLock
> is held)
> - pg_read_barrier() after read of maxMsgNum (when only SInvalReadLock is held)
>
> It improved situation for our client.
>
> Note: pg_(write|read)_barrier() is chosen instead of
> pg_atomic_(read|write)_membarrier_u32() because it certainly cheaper at
> least on x86_64 where it is translated to just a compiler barrier (empty
> asm statement).
> At least pg_atomic_read_membarrier_u32() is implemented currently as a
> write operation, that's not good for contended place.

Makes sense.

> @@ -640,8 +626,12 @@ SICleanupQueue(bool callerHasWriteLock, int minFree)
> */
> if (min >= MSGNUMWRAPAROUND)
> {
> - segP->minMsgNum -= MSGNUMWRAPAROUND;
> - segP->maxMsgNum -= MSGNUMWRAPAROUND;
> + /*
> + * we don't need memory barrier here, but using sub_fetch is just
> + * simpler than read_u32+write_u32 pair, and this place is not
> + * contented.
> + */
> + pg_atomic_sub_fetch_u32(&segP->maxMsgNum, MSGNUMWRAPAROUND);
> for (i = 0; i < segP->numProcs; i++)
> segP->procState[segP->pgprocnos[i]].nextMsgNum -= MSGNUMWRAPAROUND;
> }

Did you lose the 'segP->minMsgNum -= MSGNUMWRAPAROUND;' here? Do we have
any tests for the wraparound?

Now that maxMsgNum is unsigned, should we switch to uint32 for minMsgNum
and nextThreshold for consistency? They still don't need to be atomic
IIRC, they're protected by SInvalReadLock and SInvalWriteLock.

I kind of wonder why we need that MSGNUMWRAPAROUND limit at all; can't
we just let the integer wrap around naturally? (after switching to
unsigned, that is). That doesn't need to be part of this patch though,
it can be done separately, if it's worthwhile..
.
--
Heikki Linnakangas
Neon (https://neon.tech)