public inbox for [email protected]
help / color / mirror / Atom feedFrom: Greg Burd <[email protected]>
To: Andres Freund <[email protected]>
Cc: Nathan Bossart <[email protected]>
Cc: Thomas Munro <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Dave Cramer <[email protected]>
Subject: Re: [PATCH] Fix ARM64/MSVC atomic memory ordering issues on Win11 by adding explicit DMB ?barriers
Date: Mon, 15 Dec 2025 12:27:25 -0500
Message-ID: <[email protected]> (raw)
In-Reply-To: <sxhkd5ucchq4vululj6xiwkrd6tsxh53gjiku2knjnqalglumc@hs4vp3b7e7by>
References: <[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>
<[email protected]>
<sxhkd5ucchq4vululj6xiwkrd6tsxh53gjiku2knjnqalglumc@hs4vp3b7e7by>
On Fri, Dec 12, 2025, at 2:32 PM, Andres Freund wrote:
> Hi,
>
> On 2025-12-12 14:21:47 -0500, Greg Burd wrote:
>>
>> 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.
Thanks Andres for the comments.
> I don't think it can be optimized out, that should be prevented by
> _ReadWriteBarrier() being a compiler barrier.
While the documentation does mention that this has been deprecated, I tend to agree with you. This has been in place for a while, no need to change what works at this time. A new thread might be a better forum if there is some evidence that we should revisit this in the future. I'd like to land the ARM64/MSVC changes and enable that platform in this thread.
>> 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.
>
> I don't think that's a good idea - the _ReadWriteBarrier() is sufficient on
> x86 to implement a spinlock release (due to x86 being a total store order
> architecture, once the lock is observed as being released, all the effects
> protected by the lock are also guaranteed to be visible). Making the
> spinlocks use an atomic op for both acquire and release does cause measurable
> slowdowns on x86 with gcc, so I'd expect the same to be true on windows.
Got it, fixed in v9.
best.
-greg
> Greetings,
>
> Andres Freund
Attachments:
[application/octet-stream] v9-0001-Enable-the-Microsoft-Windows-ARM64-MSVC-platform.patch (7.6K, ../[email protected]/2-v9-0001-Enable-the-Microsoft-Windows-ARM64-MSVC-platform.patch)
download | inline diff:
From 396e5e5ab644aae261f852d616fe644f25238c37 Mon Sep 17 00:00:00 2001
From: Dave Cramer <[email protected]>
Date: Sun, 13 Jul 2025 06:33:17 -0400
Subject: [PATCH v9] Enable the Microsoft Windows ARM64/MSVC platform
Add support for the ARM64 architecture on Windows 11 using MSVC compiler
addressing build issues and implementing proper memory synchronization
semantics for this platform.
* Implement spin_delay() with __isb(_ARM64_BARRIER_SY) intrinsic to emit
the "ISB SY" instruction which matches the GCC/Clang approach to
spinloop delay and emperical evidence that it out-scales the YIELD
instruction in practice.
* Unconditionally choose to use the MSVC supplied intrinsic
for CRC32 on ARM64.
* Implement the S_UNLOCK() macro using the InterlockedExchange()
intrinsic on ARM64.
Author: Greg Burd <[email protected]>
Author: Dave Cramer <[email protected]>
Discussion: https://postgr.es/m/3c576ad7-d2da-4137-b791-5821da7cc370%40app.fastmail.com
---
doc/src/sgml/installation.sgml | 2 +-
meson.build | 9 ++++-
src/include/storage/s_lock.h | 72 +++++++++++++++++++++++++++-------
src/port/pg_crc32c_armv8.c | 6 +++
src/tools/msvc_gendef.pl | 8 ++--
5 files changed, 75 insertions(+), 22 deletions(-)
diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml
index fe8d73e1f8c..3f8d512a906 100644
--- a/doc/src/sgml/installation.sgml
+++ b/doc/src/sgml/installation.sgml
@@ -3967,7 +3967,7 @@ configure ... LDFLAGS="-R /usr/sfw/lib:/opt/sfw/lib:/usr/local/lib"
<sect3 id="install-windows-full-64-bit">
<title>Special Considerations for 64-Bit Windows</title>
<para>
- PostgreSQL will only build for the x64 architecture on 64-bit Windows.
+ PostgreSQL will only build for the x64 and ARM64 architectures on 64-bit Windows.
</para>
<para>
Mixing 32- and 64-bit versions in the same build tree is not supported.
diff --git a/meson.build b/meson.build
index d7c5193d4ce..57679e28443 100644
--- a/meson.build
+++ b/meson.build
@@ -2523,7 +2523,12 @@ int main(void)
}
'''
- if cc.links(prog, name: '__crc32cb, __crc32ch, __crc32cw, and __crc32cd without -march=armv8-a+crc',
+ # Check first for a MSVC/ARM64 combo because the test prog above won't
+ # compile (as it doesn't '#ifdef _MSC_VER #include <intrin.h>'), which
+ # is okay as we know for a fact that this platform combo supports the
+ # intrinsic for ARM64 CRC the test performs, so use that unconditionally.
+ if (host_cpu == 'aarch64' and cc.get_id() == 'msvc') or \
+ cc.links(prog, name: '__crc32cb, __crc32ch, __crc32cw, and __crc32cd without -march=armv8-a+crc',
args: test_c_args)
# Use ARM CRC Extension unconditionally
cdata.set('USE_ARMV8_CRC32C', 1)
@@ -2542,7 +2547,7 @@ int main(void)
cdata.set('USE_ARMV8_CRC32C', false)
cdata.set('USE_ARMV8_CRC32C_WITH_RUNTIME_CHECK', 1)
have_optimized_crc = true
- endif
+endif
elif host_cpu == 'loongarch64'
diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index 7f8f566bd40..870010c67fd 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -594,7 +594,8 @@ tas(volatile slock_t *lock)
#if !defined(HAS_TEST_AND_SET) /* We didn't trigger above, let's try here */
-#ifdef _MSC_VER
+/* When compiling for Microsoft Windows using MSVC */
+#if defined(_MSC_VER)
typedef LONG slock_t;
#define HAS_TEST_AND_SET
@@ -602,34 +603,75 @@ typedef LONG slock_t;
#define SPIN_DELAY() spin_delay()
-/* If using Visual C++ on Win64, inline assembly is unavailable.
- * Use a _mm_pause intrinsic instead of rep nop.
+/*
+ * When using MSVC on any non-ARM64 (aarch64) platforms (x86-64 and x86)
+ * _ReadWriteBarrier is used to prevent the compiler from reordering
+ * memory operations across the lock operation.
*/
+#if !defined(_M_ARM64)
+
+#include <intrin.h>
+#pragma intrinsic(_ReadWriteBarrier)
+
+#define S_UNLOCK(lock) \
+ do { _ReadWriteBarrier(); (*(lock)) = 0; } while (0)
+
+#endif /* !defined(_M_ARM64) */
+
#if defined(_WIN64)
+#if defined(_M_ARM64)
+
+/*
+ * _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))
+
+/*
+ * While there is support for a __yield() intrinsic for MSVC/ARM64[1], there
+ * is a wealth of real-world testing across databases and languages as well
+ * as a blog post by ARM[2] suggest that ISB is the most scalable and power
+ * friendly instruction to use for spinlock delay loops. So we use the only
+ * supported intrinsic/flag combination availble for this platform combo[3].
+ * This matches what we do above when compiling with either GCC or Clang.
+ *
+ * [1] https://learn.microsoft.com/en-us/cpp/intrinsics/arm64-intrinsics
+ * [2] https://developer.arm.com/community/arm-community-blogs/b/architectures-and-processors-blog/posts/multi-threaded-applications-arm
+ * [3] https://github.com/MicrosoftDocs/cpp-docs/blob/main/docs/intrinsics/arm64-intrinsics.md
+ */
+static __forceinline void
+spin_delay(void)
+{
+ __isb(_ARM64_BARRIER_SY);
+}
+
+#else /* !defined(_M_ARM64) */
+
+/*
+ * If using Visual C++ on Win64, inline assembly is unavailable. Use the
+ * _mm_pause intrinsic instead of rep nop.
+ */
static __forceinline void
spin_delay(void)
{
_mm_pause();
}
-#else
+
+#endif /* defined(_M_ARM64) */
+#else /* !defined(_WIN64) */
+
+/* On 32bit systems (x86) use a no-op instruction */
static __forceinline void
spin_delay(void)
{
/* See comment for gcc code. Same code, MASM syntax */
__asm rep nop;
}
-#endif
-
-#include <intrin.h>
-#pragma intrinsic(_ReadWriteBarrier)
-
-#define S_UNLOCK(lock) \
- do { _ReadWriteBarrier(); (*(lock)) = 0; } while (0)
-#endif
-
-
-#endif /* !defined(HAS_TEST_AND_SET) */
+#endif /* !defined(_WIN64) */
+#endif /* defined(_MSC_VER) */
+#endif /* !defined(HAS_TEST_AND_SET) */
/* Blow up if we didn't have any way to do spinlocks */
diff --git a/src/port/pg_crc32c_armv8.c b/src/port/pg_crc32c_armv8.c
index 5ba070bb99d..29a91dca62f 100644
--- a/src/port/pg_crc32c_armv8.c
+++ b/src/port/pg_crc32c_armv8.c
@@ -14,7 +14,13 @@
*/
#include "c.h"
+#ifdef _MSC_VER
+ /* MSVC ARM64 intrinsics */
+#include <intrin.h>
+#else
+ /* GCC/Clang: Use ACLE intrinsics from arm_acle.h */
#include <arm_acle.h>
+#endif
#include "port/pg_crc32c.h"
diff --git a/src/tools/msvc_gendef.pl b/src/tools/msvc_gendef.pl
index 868aad51b09..c92c94c4775 100644
--- a/src/tools/msvc_gendef.pl
+++ b/src/tools/msvc_gendef.pl
@@ -118,9 +118,9 @@ sub writedef
{
my $isdata = $def->{$f} eq 'data';
- # Strip the leading underscore for win32, but not x64
+ # Strip the leading underscore for win32, but not x64 and aarch64
$f =~ s/^_//
- unless ($arch eq "x86_64");
+ unless ($arch eq "x86_64" || $arch eq "aarch64");
# Emit just the name if it's a function symbol, or emit the name
# decorated with the DATA option for variables.
@@ -141,7 +141,7 @@ sub writedef
sub usage
{
die("Usage: msvc_gendef.pl --arch <arch> --deffile <deffile> --tempdir <tempdir> files-or-directories\n"
- . " arch: x86 | x86_64\n"
+ . " arch: x86 | x86_64 | aarch64\n"
. " deffile: path of the generated file\n"
. " tempdir: directory for temporary files\n"
. " files or directories: object files or directory containing object files\n"
@@ -158,7 +158,7 @@ GetOptions(
'tempdir:s' => \$tempdir,) or usage();
usage("arch: $arch")
- unless ($arch eq 'x86' || $arch eq 'x86_64');
+ unless ($arch eq 'x86' || $arch eq 'x86_64' || $arch eq 'aarch64');
my @files;
--
2.52.0.windows.1
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