On Mon, Dec 18, 2023 at 10:00:29PM -0600, Nathan Bossart wrote:
> I found this code when searching for callers that use atomic exchanges as
> atomic writes with barriers (for a separate thread [0]). Can't we use
> pg_atomic_write_u64() here since the locking functions that follow should
> serve as barriers?
The full barrier guaranteed by pg_atomic_exchange_u64() in
LWLockUpdateVar() was also necessary for the shortcut based on
read_u32() to see if there are no waiters, but it has been discarded
in the later versions of the patch because it did not influence
performance under heavy WAL inserts.
So you mean to rely on the full barriers taken by the fetches in
LWLockRelease() and LWLockWaitListLock()? Hmm, I got the impression
that pg_atomic_exchange_u64() with its full barrier was necessary in
these two paths so as all the loads and stores are completed *before*
updating these variables. So I am not sure to get why it would be
safe to switch to a write with no barrier.
> I've attached a patch to demonstrate what I'm thinking. This might be more
> performant, although maybe less so after commit 64b1fb5. Am I missing
> something obvious here? If not, I might rerun the benchmarks to see
> whether it makes any difference.
I was wondering as well if the numbers we got upthread would go up
after what you have committed to improve the exchanges. :)
Any change in this area should be strictly benchmarked.
--
Michael