Re: [PATCH] Reduce LWLockWaitListLock() cache-line contention with adaptive spin reads

On Sat, Oct 10, 2026 at 8:36 AM Min, Baohong <baohong(dot)min(at)intel(dot)com> wrote:

> Hi Jakub,

Hi Baohong,

> Thank you for testing and for your feedback. I've tried to address some of the questions you raised, as follows:
>
> a. SPINS_PER_LOCK_READ_THRESHOLD is a threshold reflecting how severe the lock contention is. It is set to 5 here, meaning the spin loop read lock->state 5 times before observing the free state;
> more than 5 reads is considered heavy contention, so we use spins_per_lock_read + 5 to ease the contention. (Although both values are 5, they mean different things.)
> Setting SPINS_PER_LOCK_READ_THRESHOLD to 3, 5, or 6 can all improve performance-there is no strict requirement.
> The optimal value may differ across CPU core counts or models, but 5 is a value that shows improvement on all the platforms tested so far.
>
> a2. 256 is an empirically chosen upper bound that proved large enough across different platforms and CPU core counts; it isn't a precise limit-values like 300 or 500 would work just as well, as long as it isn't too small.

Cool, explanation is good enough for me, but patch itself should describe
this nearby magic number definitions, so that it would easier for the next
readers to follow / reason about. Please also see questions/dobuts from Wenhui
Qiu in parallel subthread here [1] and also should be answered (they are
partially related).

BTW: here we should not top post, and just include next version of the patch
attached to the thread (we usually use something like: `git format-patch -v2
HEAD~1` to prepare the next iteration (-v2, then -v3 and so on)

> a3. Yes, your approach probably gives finer-grained control.

I think this needs to be probably fixed and this is the same thing raised by
Wenhui in review point #2 in [1]. Also MIN_DELAY_USEC is not just used by
LWLockWaitListLock()->perform_spin_delay() but as noted there are other
users too (#4).

> b. When spins_per_lock_read is shared across all LWLocks, its value is effectively determined by the most heavily contended lock - which is also where the biggest performance hotspot is.
> The current spins_per_lock_read may be too large for another LWLock and add some latency, but the gain from relieving the most contended lock should outweigh that.
> Making it per-tranche would give finer control, at the cost of more complex code changes and an uncertain overall performance benefit.

Fair point.

> c. Yes, your description is more accurate.
>
> d. Thanks, that's a good suggestion. For now, though, let's first see whether our current approach gets accepted.

Small hint: usually attaching the new patch version (potentially with some new
benchmarks even if for edge-cases), generates enough traffic on the thread
that attracts some further attention and new reviews. It usually takes several
such new patch cycles (even to just fix typos), and then hopefully interested
committers gets to it (hopefully, they seem to be occupied by the
"vulnpocalypse"). We probably need a lot more performance benchmarks from
variety of various architectures (OSes?), so that it's seen as safe change,
but that's how I understand the process here.

-J.

[1] - https://www.postgresql.org/message-id/CAGjGUAJAYx2T_GRizy4mSAnNfA4h88g3KZDQA3WFViU6UhGdQQ%40mail.gmail.com

> -----Original Message-----
> From: Jakub Wartak <jakub(dot)wartak(at)enterprisedb(dot)com>
> Sent: Thursday, October 1, 2026 5:23 PM
> To: Min, Baohong <baohong(dot)min(at)intel(dot)com>
> Cc: qiuwenhuifx(at)gmail(dot)com; Okanovic, Haris <harisokn(at)amazon(dot)com>; PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
> Subject: Re: [PATCH] Reduce LWLockWaitListLock() cache-line contention with adaptive spin reads
>
> Hi Baohong,
>
> thanks for touching such advanced
>
> > Thanks to Haris for adding the AWS Graviton performance tests - we now have performance data on both Intel (x86) and Arm.
> > Wenhui, could you please review whether this patch is ready to commit?
>
> I've tried my best here. This patched looked interesting and WOW! I've got extreme +23% with this patch in artificial highly contended LWLock scenarios (Wow-because judging from the lwlock.c and s_lock.h code and reading some previous discussion about LWLocks/ spinlocks this stuff looks like already super-optimized).
>
> In normal pgbench (-S), I've got +12%. Of course the effects seem to mostly present on >=2 socket systems mainly.
>
> I LWLock microbenchmark
> =======================
> I've tried this on couple systems, on laptop or single socket systems (single
> CCD) it doesn't move a needle (good!), but then I've primarly tested this on i4i.metal EC2 VM (Ice Lake, 2s64c128t, 2 NUMA nodes) with LWLock/Xidgen caused by wal_insert.sql and "pgbench -n -c 500 -j 128 -T 20 -P 1 -f wal_insert.sql" along with:
> - wal_buffers=256MB
> - and fsync=off (to avoid LWLock/WALWrite)
>
> Where wal_insert.sql was (and t had no pk index):
> \set id random(1, 1000000000)
> INSERT INTO t VALUES (:id, repeat('x', 100));
>
> With perf with the patch one can see LWLockWaitListLock() disapearing as top symbol. So in that stress-test this yields mentioned +21% in high contention scenarios (here anything >= 128 VCPUs), it is avg of 3 runs:
>
> clients master patch delta
> 32 153k 154k +1%
> 128 180k 201k +12%
> 256 172k 212k +23%
> 500 165k 202k +21%
> 1024 160k 164k +2%
>
> 500 not 51,* because it was just first try at this at artificial > 500 (so the -c 1024 runs) there's way more IPC/ProcarrayGroupUpdate, but that's another unrelated (futex scalability?) problem, but it explains why benefit is much smaller there.
>
> The patch comes with two changes and when trying out those with -c 500 to quantify their benefit:
> a) s_lock.c (MIN_DELAY_USEC) change alone gives + ~6%
> b) lwlock.c (adaptive re-read), alone gives + ~17%
> c) both, as mentioned above +21%, btw this also allows WAL generation rate
> to also increase from like 660MB to 790MB/s :o (but that's fsync=off)
>
> So to my understanding this patch is major reduction the of cache-line ping-pongs that occurs between sockets(L3s/CCDs) under wait list is being altered (contended). When researching this further, it looked like this patch brings us closer to what is described in [1] "Intel(r) 64 and
> IA-32 Architectures Optimization Reference Manual, v50, Volume 1" on page
> 81 "2.7.4 PAUSE LATENCY IN SKYLAKE CLIENT MICROARCHITECTURE" / "Example 2-10. Contended Locks with Increasing Back-off" (re-reading lock with atomics cmpxchng sometimes, and not always?).
>
> Because the above goes into details about CPU microarchitectures, I've gave a shot into spotting regressions on on my legacy 4s32c64t NUMA Xeon Sandy Bridge EP and with quick shot test with that wal_insert.sql it gave me much smaller benefits, but still observable:
>
> clients master patch delta
> 32 151k 152k 0.5%
> 64 161k 166k 3%
> 128 129k 140k ~9%
> 500 95k 99k 3%
>
> so there is no regression even on very old hardware. I don't have access to multi-sockets ARM/PowerPC to quanitfy the benefits there.
>
> II normal pgbench
> =================
> So with promising results above, I've gave a short to classic pgbench (-S) too. Back to that modern box i4i.metal EC2 VM (1TB RAM) with s_b=16GB and just ~100GB (-s 7000) in VFS cache that to causing stress of LWLock/BuffeMappings:
> multiple runs of "pgbench -n -S -c 500 -j 128 -T 20"" report +12% (!) on fully isolated/stabilized hw. The perf reports drop of LWLockWaitListLock() from ~4% to non-existent levels (we are talking boost from 1136k to 1273k TPS, I'm quite impressed as I haven't such big performance jump with such small patch for quite a while!)
>
> III patch itself
> ================
> I've this to commitfest as apparently it was missing from there, so people could miss this. It's tracked as https://commitfest.postgresql.org/patch/7376/
> (but just 2 with authors, I couldn't find )
>
> Some review findings (and questions!):
>
> a. This: +#define SPINS_PER_LOCK_READ_THRESHOLD 5 makes some sense to me
> (number 3 would also make sense :D), but anyway, then later in
> LWLockWaitListLock() we have:
> + /* Adaptively adjust read interval based on wait duration */
> + if (lock_read_count > SPINS_PER_LOCK_READ_THRESHOLD)
> + spins_per_lock_read = Min(spins_per_lock_read + 5, 256);
>
> shouldn't it be read:
> spins_per_lock_read = Min(spins_per_lock_read+SPINS_PER_LOCK_READ_THRESHOLD
> instead ?
>
> a2.and also how the "256" was derrived there? (that seems to 256x PAUSE
> instructions? which is somehow is bound to the Intel ?? and that earlier
> mentioned Intel doc is explict about different PAUSE instruction even on
> various Intel's microarchitectures ??? // pre vs after Skylake), If that's
> true perhaps we should somehow also cover other architectures and perhaps
> somehow use s_lock.h / pg_cpu*[.ch] to track all of that? Dunno, just
> asking? This would be also solution/in line with what Haris observed in [2]
> where he writes that his ARM LWLockWaitListLock() change causes regression
> on Intel/AMD "Intel Granite Rapids and AMD Turin (x86_64) both show minor
> degradation with the change, which is the reason the patch is currently
> limited to arch64 only"
>
> a3.This stuff is pretty hard to reason about and test, but hypothethically if
> we perform_spin_delay() and the pg_usleeps() in the edge case gets nearly
> maximums (MAX_DELAY_USLEEP = 1s), and we get spins_per_lock_read nearby to
> 256 (in increments of 5), wouldn't that mean we are spinning with stale
> cached reads potentially without a valid reason? Shouldn't we force atomic
> re-read immediatlely once after we have really slept (context-switch) with
> some long pg_usleep()? (it could have change by then, true/false?) It's
> hard to reason because now there are two independent semi-time-tracking
> things at once: spins_per_lock_read + delayStatus.cur_delay. This is more
> of question.
>
> b. isn't the common one global static variable "spins_per_lock_read" combined
> for all lwlocks good? (I'm afraid of situation, where single condented
> LWLock type cascades value to the other ones, shouldn't this be at least
> per tranche? in the artificial scenarios we simply simulate one giant
> contention, but, I'm afraid if some production systems are having multiple
> contentions all at the same time). I'm talking about this line:
> +static int spins_per_lock_read = 1;
>
> c. nitpicking :) -- commitmsg says "re-read lock->state on every iteration,
> causing heavy cache-line bouncing that limits throughput", perhaps it
> should say "atomic re-reads of ..." because pure reading doesn't seem to
> cause cache-ling ping pongs.
>
> d. There seem to be other places than just LWLockWaitListLock() that also
> seem to have the same pattern of using perform_spin_delay() in this
> pattern:
> while(somestate & SOME_FLAG) {
> perform_spin_delay()
> somestate = pg_atomic_read_u64(...)
> }
> dunno, but if we are fixing LWLockWaitListLock(), wouldn't it make some
> sense to improve also bufmgr.c's ones: LockBufHdr(), WaitBufHdrUnLock() ?
> It's an open question / idea and I haven't tried, maybe someone know if
> fixing that along they one too wouldn't boost the rightmost monotonically
> increasing inserts scenarios? (we would need to be hitting single block
> often in NUMA systems to observe this?? or maybe some other scenario?)