From 5b845de636224ef3e065be8e1c7d2df3389aa175 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 7 Jan 2023 23:21:52 -0800 Subject: [PATCH 1/2] Use Windows Interlocked* APIs for refcounts when C11 isn't available Right now, MSVC has to fallback to refcount_lock.c, which uses a single, global lock for all refcount operations. Instead, use the Interlocked* APIs to implement them. The motivation is two-fold. First, this removes a performance cliff when building for Windows on a non-Clang compiler. (Although I've not been able to measure it in an end-to-end EVP benchmark, only a synthetic refcount-only benchmark.) More importantly, it gets us closer to assuming atomics support on all non-NO_THREADS configurations. (The next CL will clear through that.) That, in turn, will make it easier to add an atomics-like abstractions to some of our hotter synchronization points. (Even in newer glibc, with its better rwlock, read locks fundamentally need to write to memory, so we have some cacheline contention on shared locks.) Annoyingly, the Windows atomic_load replacement is not quite right. I've used a "no-op" InterlockedCompareExchange(p, 0, 0) which, empirically, still results in a write. But a write to the refcount cacheline is surely better than taking a global exclusive lock. See comments in file for details. OpenSSL uses InterlockedOr(p, 0), but that actually results in even worse code. (InterlockedOr needs a retry loop when the underlying cmpxchg fails, whereas InterlockedCompareExchange is a single cmpxchg.) Hopefully, in the future (perhaps when we require VS 2022's successor, based on [1]), this can be removed in favor of C11 atomics everywhere. [1] https://devblogs.microsoft.com/cppblog/c11-atomics-in-visual-studio-2022-version-17-5-preview-2/ Bug: 570 Cq-Include-Trybots: luci.boringssl.try:linux_clang_rel_tsan Change-Id: I125da139e2fd3ae51e54309309fda16ba97ccf20 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59846 Commit-Queue: David Benjamin Reviewed-by: Adam Langley --- crypto/CMakeLists.txt | 1 + crypto/internal.h | 8 ++++ crypto/refcount_lock.c | 4 +- crypto/refcount_win.c | 89 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 100 insertions(+), 2 deletions(-) create mode 100644 crypto/refcount_win.c diff --git a/crypto/CMakeLists.txt b/crypto/CMakeLists.txt index bc307023c..12d15a84e 100644 --- a/crypto/CMakeLists.txt +++ b/crypto/CMakeLists.txt @@ -204,6 +204,7 @@ add_library( rc4/rc4.c refcount_c11.c refcount_lock.c + refcount_win.c rsa_extra/rsa_asn1.c rsa_extra/rsa_crypt.c rsa_extra/rsa_print.c diff --git a/crypto/internal.h b/crypto/internal.h index a4cd92912..adcd44435 100644 --- a/crypto/internal.h +++ b/crypto/internal.h @@ -548,6 +548,14 @@ OPENSSL_EXPORT void CRYPTO_once(CRYPTO_once_t *once, void (*init)(void)); #define OPENSSL_C11_ATOMIC #endif +// Older MSVC does not support C11 atomics, so we fallback to the Windows APIs. +// This can be removed once we can rely on +// https://devblogs.microsoft.com/cppblog/c11-atomics-in-visual-studio-2022-version-17-5-preview-2/ +#if !defined(OPENSSL_C11_ATOMIC) && defined(OPENSSL_THREADS) && \ + defined(OPENSSL_WINDOWS) +#define OPENSSL_WINDOWS_ATOMIC +#endif + // CRYPTO_REFCOUNT_MAX is the value at which the reference count saturates. #define CRYPTO_REFCOUNT_MAX 0xffffffff diff --git a/crypto/refcount_lock.c b/crypto/refcount_lock.c index 173267e38..7886bf899 100644 --- a/crypto/refcount_lock.c +++ b/crypto/refcount_lock.c @@ -18,7 +18,7 @@ #include -#if !defined(OPENSSL_C11_ATOMIC) +#if !defined(OPENSSL_C11_ATOMIC) && !defined(OPENSSL_WINDOWS_ATOMIC) static_assert((CRYPTO_refcount_t)-1 == CRYPTO_REFCOUNT_MAX, "CRYPTO_REFCOUNT_MAX is incorrect"); @@ -49,4 +49,4 @@ int CRYPTO_refcount_dec_and_test_zero(CRYPTO_refcount_t *count) { return ret; } -#endif // OPENSSL_C11_ATOMIC +#endif // !OPENSSL_C11_ATOMIC && !OPENSSL_WINDOWS_ATOMICS diff --git a/crypto/refcount_win.c b/crypto/refcount_win.c new file mode 100644 index 000000000..7a2740bc2 --- /dev/null +++ b/crypto/refcount_win.c @@ -0,0 +1,89 @@ +/* Copyright (c) 2023, Google Inc. + * + * Permission to use, copy, modify, and/or distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY + * SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION + * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ + +#include "internal.h" + +#if defined(OPENSSL_WINDOWS_ATOMIC) + +#include + + +// See comment above the typedef of CRYPTO_refcount_t about these tests. +static_assert(alignof(CRYPTO_refcount_t) == alignof(LONG), + "CRYPTO_refcount_t does not match LONG alignment"); +static_assert(sizeof(CRYPTO_refcount_t) == sizeof(LONG), + "CRYPTO_refcount_t does not match LONG size"); + +static_assert((CRYPTO_refcount_t)-1 == CRYPTO_REFCOUNT_MAX, + "CRYPTO_REFCOUNT_MAX is incorrect"); + +static uint32_t atomic_load_u32(volatile LONG *ptr) { + // This is not ideal because it still writes to a cacheline. MSVC is not able + // to optimize this to a true atomic read, and Windows does not provide an + // InterlockedLoad function. + // + // The Windows documentation [1] does say "Simple reads and writes to + // properly-aligned 32-bit variables are atomic operations", but this is not + // phrased in terms of the C11 and C++11 memory models, and indeed a read or + // write seems to produce slightly different code on MSVC than a sequentially + // consistent std::atomic::load in C++. Moreover, it is unclear if non-MSVC + // compilers on Windows provide the same guarantees. Thus we avoid relying on + // this and instead still use an interlocked function. This is still + // preferable a global mutex, and eventually this code will be replaced by + // [2]. Additionally, on clang-cl, we'll use the |OPENSSL_C11_ATOMIC| path. + // + // [1] https://learn.microsoft.com/en-us/windows/win32/sync/interlocked-variable-access + // [2] https://devblogs.microsoft.com/cppblog/c11-atomics-in-visual-studio-2022-version-17-5-preview-2/ + return (uint32_t)InterlockedCompareExchange(ptr, 0, 0); +} + +static int atomic_compare_exchange_u32(volatile LONG *ptr, uint32_t *expected32, + uint32_t desired) { + LONG expected = (LONG)*expected32; + LONG actual = InterlockedCompareExchange(ptr, (LONG)desired, expected); + *expected32 = (uint32_t)actual; + return actual == expected; +} + +void CRYPTO_refcount_inc(CRYPTO_refcount_t *in_count) { + volatile LONG *count = (volatile LONG *)in_count; + uint32_t expected = atomic_load_u32(count); + + while (expected != CRYPTO_REFCOUNT_MAX) { + const uint32_t new_value = expected + 1; + if (atomic_compare_exchange_u32(count, &expected, new_value)) { + break; + } + } +} + +int CRYPTO_refcount_dec_and_test_zero(CRYPTO_refcount_t *in_count) { + volatile LONG *count = (volatile LONG *)in_count; + uint32_t expected = atomic_load_u32(count); + + for (;;) { + if (expected == 0) { + abort(); + } else if (expected == CRYPTO_REFCOUNT_MAX) { + return 0; + } else { + const uint32_t new_value = expected - 1; + if (atomic_compare_exchange_u32(count, &expected, new_value)) { + return new_value == 0; + } + } + } +} + +#endif // OPENSSL_WINDOWS_ATOMIC From 8a85012bc47ebb3e985839d1cb7c699d325ff279 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 8 Jan 2023 08:55:40 -0800 Subject: [PATCH 2/2] Remove the lock-based atomics fallback On Windows, we can rely on Interlocked APIs. On non-Windows builds, we currently require C11 but permit C11 atomics to be missing, via __STDC_NO_ATOMICS__. This CL tightens this so C11 atomics are required on non-MSVC builds. My hope is that, now that we require C11 on non-Windows, this is a fairly safe requirement. We already require pthreads on any platform where this might apply, and it's hard to imagine someone has C11, pthreads, but not C11 atomics. This change means that, in later work, we can refactor the refcount logic to instead be a compatibility layer, and then an atomics-targetting CRYPTO_refcount_t implementation. With a compatibility layer, we can use atomics in more places, notably where our uses of read locks are causing cacheline contention. The platform restriction isn't *strictly* necessary. We could, like with refcounts, emulate with a single, global lock. Indeed any platforms in this situation have already been living with that lock for refcounts without noticing. But then later work to add "atomics" to read locks would regress contention for those platforms. So I'm starting by rejecting this, so if any such platform exists, we can understand their performance needs before doing that. Update-Note: On non-Windows platforms, we now require C11 atomics support. Note we already require C11 itself. If this affects your build, get in touch with BoringSSL maintainers. Bug: 570 Cq-Include-Trybots: luci.boringssl.try:linux_clang_rel_tsan Change-Id: I868fa4ba87ed73dfc9d52e80d46853ef56715a5f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59847 Commit-Queue: David Benjamin Reviewed-by: Adam Langley --- crypto/CMakeLists.txt | 2 +- crypto/internal.h | 10 ++++++++++ .../{refcount_lock.c => refcount_no_threads.c} | 16 +++------------- 3 files changed, 14 insertions(+), 14 deletions(-) rename crypto/{refcount_lock.c => refcount_no_threads.c} (72%) diff --git a/crypto/CMakeLists.txt b/crypto/CMakeLists.txt index 12d15a84e..c427b20c5 100644 --- a/crypto/CMakeLists.txt +++ b/crypto/CMakeLists.txt @@ -203,7 +203,7 @@ add_library( rand_extra/windows.c rc4/rc4.c refcount_c11.c - refcount_lock.c + refcount_no_threads.c refcount_win.c rsa_extra/rsa_asn1.c rsa_extra/rsa_crypt.c diff --git a/crypto/internal.h b/crypto/internal.h index adcd44435..5c0473596 100644 --- a/crypto/internal.h +++ b/crypto/internal.h @@ -556,6 +556,16 @@ OPENSSL_EXPORT void CRYPTO_once(CRYPTO_once_t *once, void (*init)(void)); #define OPENSSL_WINDOWS_ATOMIC #endif +// Require some atomics implementation. Contact BoringSSL maintainers if you +// have a platform with fails this check. +// +// Note this check can only be done in C. From C++, we don't know whether the +// corresponding C mode would support C11 atomics. +#if !defined(__cplusplus) && defined(OPENSSL_THREADS) && \ + !defined(OPENSSL_C11_ATOMIC) && !defined(OPENSSL_WINDOWS_ATOMIC) +#error "Thread-compatible configurations require atomics" +#endif + // CRYPTO_REFCOUNT_MAX is the value at which the reference count saturates. #define CRYPTO_REFCOUNT_MAX 0xffffffff diff --git a/crypto/refcount_lock.c b/crypto/refcount_no_threads.c similarity index 72% rename from crypto/refcount_lock.c rename to crypto/refcount_no_threads.c index 7886bf899..096b4fa93 100644 --- a/crypto/refcount_lock.c +++ b/crypto/refcount_no_threads.c @@ -18,35 +18,25 @@ #include -#if !defined(OPENSSL_C11_ATOMIC) && !defined(OPENSSL_WINDOWS_ATOMIC) +#if !defined(OPENSSL_THREADS) static_assert((CRYPTO_refcount_t)-1 == CRYPTO_REFCOUNT_MAX, "CRYPTO_REFCOUNT_MAX is incorrect"); -static struct CRYPTO_STATIC_MUTEX g_refcount_lock = CRYPTO_STATIC_MUTEX_INIT; - void CRYPTO_refcount_inc(CRYPTO_refcount_t *count) { - CRYPTO_STATIC_MUTEX_lock_write(&g_refcount_lock); if (*count < CRYPTO_REFCOUNT_MAX) { (*count)++; } - CRYPTO_STATIC_MUTEX_unlock_write(&g_refcount_lock); } int CRYPTO_refcount_dec_and_test_zero(CRYPTO_refcount_t *count) { - int ret; - - CRYPTO_STATIC_MUTEX_lock_write(&g_refcount_lock); if (*count == 0) { abort(); } if (*count < CRYPTO_REFCOUNT_MAX) { (*count)--; } - ret = (*count == 0); - CRYPTO_STATIC_MUTEX_unlock_write(&g_refcount_lock); - - return ret; + return *count == 0; } -#endif // !OPENSSL_C11_ATOMIC && !OPENSSL_WINDOWS_ATOMICS +#endif // !OPENSSL_THREADS