Feedback-ID: i675e48f3:Fastmail
MIME-Version: 1.0
Date: Fri, 12 Dec 2025 14:21:47 -0500
From: "Greg Burd" <greg@burd.me>
To: "Nathan Bossart" <nathandbossart@gmail.com>
Cc: "Thomas Munro" <thomas.munro@gmail.com>,
 "PostgreSQL Hackers" <pgsql-hackers@lists.postgresql.org>,
 "Peter Eisentraut" <peter@eisentraut.org>,
 "Dave Cramer" <davecramer@gmail.com>, "Andres Freund" <andres@anarazel.de>
Message-Id: <8e926f26-0a97-434f-a665-8bb6f9159b08@app.fastmail.com>
In-Reply-To: <aTw8tb_eIpDBYHfW@nathan>
References: 
 <CA+hUKG+r5TUe0vvqyFdD4cCpmYOXNso3QJ0whi0Md0YY6xG-Gw@mail.gmail.com>
 <386595D6-0373-4E08-BC00-D8F17F5F7CAE@greg.burd.me>
 <ed03732f-0aca-4c07-8832-34d43f95c232@app.fastmail.com>
 <63d07b91-a55a-4c5f-bf62-d3506acfdb5b@app.fastmail.com>
 <beirrgqo5n5e73dwa4dsdnlbtef3bsdv5sgarm6przdzxvifk5@whyuhyemmhyr>
 <c57bcd13-558f-4004-8988-55df8512b878@app.fastmail.com>
 <5e169c16-9e54-4325-8027-1f52a5bb5c1e@app.fastmail.com>
 <CA+hUKG+v8uqSibXXg5c1+qwJVY0nmpp_2YbTCpN8qrFwb+dvew@mail.gmail.com>
 <1c2a29b8-5b1e-44f7-a871-71ec5fefc120@app.fastmail.com>
 <f9bbd950-b70d-4881-9ae2-263e4d01e644@app.fastmail.com>
 <aTw8tb_eIpDBYHfW@nathan>
Subject: Re: [PATCH] Fix ARM64/MSVC atomic memory ordering issues on Win11 by
 adding
 explicit DMB ?barriers
Content-Type: text/plain
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/8e926f26-0a97-434f-a665-8bb6f9159b08%40app.fastmail.com>
Precedence: bulk


On Fri, Dec 12, 2025, at 11:03 AM, Nathan Bossart wrote:
> +/*
> + * _InterlockedExchange() generates a full memory barrier (or release
> + * semantics that ensures all prior memory operations are visible to
> + * other cores before the lock is released.
> + */
> +#define S_UNLOCK(lock) (InterlockedExchange(lock, 0))

Nathan, thanks for looking at the patch!

> This seems to change the implementation from
>
> 	#define S_UNLOCK(lock)	\
> 		do { _ReadWriteBarrier(); (*(lock)) = 0; } while (0)
>
> in some cases, but I am insufficiently caffeinated to figure out what
> platforms use which implementation.  In any case, it looks like we are
> changing it for some currently-supported platforms, and I'm curious why.

This change is within _MSC_VER, but AFAICT this intrinsic is available across their supported platforms.  The previous implementation of S_UNLOCK() boils down to a no-op because the _ReadWriteBarrier()[1] is a hint to the compiler and does not emit any instruction on any platform and it's also deprecated.  So, on MSVC S_UNLOCK is an unguarded assignment and then a loop that will be optimized out, not really what we wanted I'd imagine.  My tests with godbolt showed this to be true, no instruction barriers emitted.  I think it was Andres[2] who suggested replacing it with _InterlockedExchange()[3].  So, given that _ReadWriteBarrier() is deprecated I decided not to specialize this change to only the ARM64 platform, sorry for not making that clear in the commit or email.

My buildfarm animal for this platform was just approved and is named "unicorn", I'm not making that up. :)

best.

-greg

> Perhaps there's some way to make the #ifdefs a bit more readable, too
> (e.g., a prerequisite patch that rearranges things).
>
> The rest looks generally reasonable to me.
>
> -- 
> nathan

[1] https://learn.microsoft.com/en-us/cpp/intrinsics/readwritebarrier
[2] https://www.postgresql.org/message-id/beirrgqo5n5e73dwa4dsdnlbtef3bsdv5sgarm6przdzxvifk5%40whyuhyemmhyr
[3] https://learn.microsoft.com/en-us/cpp/intrinsics/interlockedexchange-intrinsic-functions