public inbox for [email protected]
help / color / mirror / Atom feedFrom: Greg Burd <[email protected]>
To: Thomas Munro <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Nathan Bossart <[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: Thu, 11 Dec 2025 11:16:28 -0500
Message-ID: <[email protected]> (raw)
In-Reply-To: <CA+hUKG+v8uqSibXXg5c1+qwJVY0nmpp_2YbTCpN8qrFwb+dvew@mail.gmail.com>
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>
On Wed, Dec 10, 2025, at 4:31 PM, Thomas Munro wrote:
> On Thu, Dec 11, 2025 at 5:32 AM Greg Burd <[email protected]> wrote:
>> Rebased with only minor changes to meson.build this patch is ready for review/commit as it is passing tests on my aarch64 Win11 MSVC system. Also note that this system I'm testing on is ready to become a member of the buildfarm (application submitted) and monitor this combo in perpetuity.
>
> - if cc.links(prog, name: '__crc32cb, __crc32ch, __crc32cw, and
> __crc32cd without -march=armv8-a+crc',
> ...
> + if host_machine.cpu_family() == 'aarch64'
Thanks for taking the time to review.
> I think this new nesting of the CRC32 feature tests breaks the test on
> "armv7" distros (in our build farm, that's a bunch of RPis running
> Debian/Raspbian, but at least FreeBSD and NetBSD also support
> "armv7"). Any ARM chip made since around 2011 is really an ARMv8+
> chip running Aarch32 code and can thus reach the ARMv8 instructions.
> For example "grison" says:
>
> checking build system type... (cached) armv7l-unknown-linux-gnueabihf
> ...
> checking which CRC-32C implementation to use... ARMv8 CRC instructions
> with runtime check
That was rather silly of me to overlook, apologies. I went back to the code that was there before written by Andres I believe and simply added a test for this one special case. That reduces code churn and simplifies the logic while preserving the earlier behavior.
As I was re-reading I decided to review the YEILD vs ISB question for this platform combo again in light of a82a5ee [1]. What I found was a wealth of information and work on the topic and the thread mentioned MariaDB so I did some digging (because, why not?). Go-lang[2] uses ISB but a comment there led me on to another resource, a blog post from ARM on the topic of spin locks for ARM64[3]. I found that like Go-lang, Rust[4] uses ISB (and if you read only one thread on the topic this is the one to dig into). The developers on the thread wrote a number of benchmarks and tested a variety of approaches landing on "ISB SY" on aarch64. I found that MySQL[5][6] also went with ISB, as has Cloudera[7], Impala[8], Aerospike[9], and WebAssembly[10] at which point I stopped looking around. There were other resources out there for understanding ARM and relaxed memory models such as [11], [12], and [13] for those that care to learn.
All of this is to say that I swapped out the YEILD for "ISB SY" for the ARM64/MSVC because that seems to make the most sense from the evidence I found and for consistency in our code.
> -#define S_UNLOCK(lock) \
> +#define S_UNLOCK(lock) \
>
> Bogus whitespace change.
Fixed, thanks.
best.
-greg
[1] https://postgr.es/m/[email protected]
[2] https://github.com/golang/go/issues/69232
[3] https://community.arm.com/arm-community-blogs/b/architectures-and-processors-blog/posts/multi-thread...
[4] https://github.com/rust-lang/rust/commit/c064b6560b7ce0adeb9bbf5d7dcf12b1acb0c807
[5] https://bugs.mysql.com/bug.php?id=100664
[6] https://github.com/mysql/mysql-server/pull/305/files
[7] https://gerrit.cloudera.org/#/c/19865/
[8] https://issues.apache.org/jira/browse/IMPALA-12122
[9] https://github.com/aerospike/aerospike-common/pull/17
[10] https://github.com/WebAssembly/threads/issues/15
[11] https://developer.arm.com/documentation/genc007826/latest "Barrier Litmus Tests and Cookbook"
[12] http://www.rdrop.com/users/paulmck/scalability/paper/whymb.2010.07.23a.pdf - Memory Barriers: a Hardware View for Software Hackers
[13] https://randomascii.wordpress.com/2020/11/29/arm-and-lock-free-programming/
Attachments:
[application/octet-stream] v7-0001-Enable-the-Microsoft-Windows-ARM64-MSVC-platform.patch (7.4K, ../[email protected]/2-v7-0001-Enable-the-Microsoft-Windows-ARM64-MSVC-platform.patch)
download | inline diff:
From 85dc82d7ad53c1bd19725883125f5f19a506973e Mon Sep 17 00:00:00 2001
From: Dave Cramer <[email protected]>
Date: Sun, 13 Jul 2025 06:33:17 -0400
Subject: [PATCH v7] 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.
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 | 60 +++++++++++++++++++++++++++-------
src/port/pg_crc32c_armv8.c | 6 ++++
src/tools/msvc_gendef.pl | 8 ++---
5 files changed, 67 insertions(+), 18 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 718150e3ac0..e57c233b124 100644
--- a/meson.build
+++ b/meson.build
@@ -2512,7 +2512,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_machine.cpu_family() == '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)
@@ -2531,7 +2536,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..50262cca887 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,71 @@ 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.
+/*
+ * _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))
+
+#if defined(_WIN64) /* Microsoft Windows x64 */
+
+#if defined(_M_ARM64) /* aarch64 */
+/*
+ * 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);
+}
+
+#elif defined(_M_X64) /* x86-64 */
+
+/*
+ * Use _mm_pause() intrinsic for x86-64. This emits the PAUSE instruction,
+ * which improves performance in spin-wait loops by preventing pipeline flush
+ * on Hyper-Threading systems.
*/
-#if defined(_WIN64)
static __forceinline void
spin_delay(void)
{
_mm_pause();
}
-#else
+
+#endif /* defined(_M_ARM64|_M_X64) */
+
+#else /* !defined(_WIN64) */
+
+#ifdef _M_IX86 /* x86-specific */
+
+/* Use no-op for MSVC 32bit x86 */
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) \
+#define S_UNLOCK(lock) \
do { _ReadWriteBarrier(); (*(lock)) = 0; } while (0)
-#endif
-
-
-#endif /* !defined(HAS_TEST_AND_SET) */
+#endif /* defined(_M_IX86) */
+#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: 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