public inbox for [email protected]  

help / color / mirror / Atom feed

From: Greg Burd <[email protected]>
To: Nathan Bossart <[email protected]>
Cc: Thomas Munro <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Dave Cramer <[email protected]>
Cc: Andres Freund <[email protected]>
Subject: Re: [PATCH] Fix ARM64/MSVC atomic memory ordering issues on Win11 by adding explicit DMB ?barriers
Date: Fri, 12 Dec 2025 14:21:47 -0500
Message-ID: <[email protected]> (raw)
In-Reply-To: <aTw8tb_eIpDBYHfW@nathan>
References: <CA+hUKG+r5TUe0vvqyFdD4cCpmYOXNso3QJ0whi0Md0YY6xG-Gw@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<beirrgqo5n5e73dwa4dsdnlbtef3bsdv5sgarm6przdzxvifk5@whyuhyemmhyr>
	<[email protected]>
	<[email protected]>
	<CA+hUKG+v8uqSibXXg5c1+qwJVY0nmpp_2YbTCpN8qrFwb+dvew@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<aTw8tb_eIpDBYHfW@nathan>


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%40whyuhyemm...
[3] https://learn.microsoft.com/en-us/cpp/intrinsics/interlockedexchange-intrinsic-functions

view thread (42+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: [PATCH] Fix ARM64/MSVC atomic memory ordering issues on Win11 by adding explicit DMB ?barriers
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox