[libcxx-commits] [libcxx] [libc++] fix `counting_semaphore` lost wakeups (PR #79265)
via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Jan 31 14:26:43 PST 2024
https://github.com/huixie90 updated https://github.com/llvm/llvm-project/pull/79265
>From d03e1342a4c24206f4f2020e78a008e5bd9a7e0a Mon Sep 17 00:00:00 2001
From: Hui <hui.xie0621 at gmail.com>
Date: Wed, 24 Jan 2024 08:38:00 +0000
Subject: [PATCH 1/3] [libc++] fix `counting_semaphore` lost wakeups
---
libcxx/include/__atomic/atomic_sync.h | 16 +++--
libcxx/include/semaphore | 21 ++++--
.../thread.semaphore/lost_wakeup.pass.cpp | 65 +++++++++++++++++++
3 files changed, 92 insertions(+), 10 deletions(-)
create mode 100644 libcxx/test/std/thread/thread.semaphore/lost_wakeup.pass.cpp
diff --git a/libcxx/include/__atomic/atomic_sync.h b/libcxx/include/__atomic/atomic_sync.h
index 93527958b2e1c..ba83396189edf 100644
--- a/libcxx/include/__atomic/atomic_sync.h
+++ b/libcxx/include/__atomic/atomic_sync.h
@@ -46,12 +46,12 @@ __libcpp_atomic_wait(__cxx_atomic_contention_t const volatile*, __cxx_contention
template <class _Atp, class _Fn>
struct __libcpp_atomic_wait_backoff_impl {
_Atp* __a;
- _Fn __test_fn;
+ _Fn __test_with_old;
_LIBCPP_AVAILABILITY_SYNC
_LIBCPP_HIDE_FROM_ABI bool operator()(chrono::nanoseconds __elapsed) const {
if (__elapsed > chrono::microseconds(64)) {
- auto const __monitor = std::__libcpp_atomic_monitor(__a);
- if (__test_fn())
+ auto __monitor = std::__libcpp_atomic_monitor(__a);
+ if (__test_with_old(__monitor))
return true;
std::__libcpp_atomic_wait(__a, __monitor);
} else if (__elapsed > chrono::microseconds(4))
@@ -62,9 +62,17 @@ struct __libcpp_atomic_wait_backoff_impl {
}
};
+template <class _Atp, class _Fn, class _Fn2>
+_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool
+__cxx_atomic_wait(_Atp* __a, _Fn&& __test_fn, _Fn2&& __test_with_old) {
+ __libcpp_atomic_wait_backoff_impl<_Atp, __decay_t<_Fn2> > __backoff_fn = {__a, __test_with_old};
+ return std::__libcpp_thread_poll_with_backoff(__test_fn, __backoff_fn);
+}
+
template <class _Atp, class _Fn>
_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool __cxx_atomic_wait(_Atp* __a, _Fn&& __test_fn) {
- __libcpp_atomic_wait_backoff_impl<_Atp, __decay_t<_Fn> > __backoff_fn = {__a, __test_fn};
+ auto __test_with_old = [&](auto&) { return __test_fn(); };
+ __libcpp_atomic_wait_backoff_impl<_Atp, decltype(__test_with_old)> __backoff_fn{__a, __test_with_old};
return std::__libcpp_thread_poll_with_backoff(__test_fn, __backoff_fn);
}
diff --git a/libcxx/include/semaphore b/libcxx/include/semaphore
index 649705f45b049..c4d9ee1ab63e8 100644
--- a/libcxx/include/semaphore
+++ b/libcxx/include/semaphore
@@ -54,6 +54,7 @@ using binary_semaphore = counting_semaphore<1>;
#include <__assert> // all public C++ headers provide the assertion handler
#include <__atomic/atomic_base.h>
#include <__atomic/atomic_sync.h>
+#include <__atomic/contention_t.h>
#include <__atomic/memory_order.h>
#include <__availability>
#include <__chrono/time_point.h>
@@ -95,19 +96,22 @@ public:
_LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
__update <= _LIBCPP_SEMAPHORE_MAX - __old, "update is greater than the expected value");
- if (__old > 0) {
- // Nothing to do
- } else if (__update > 1)
+ if (__old == 0) {
__a_.notify_all();
- else
- __a_.notify_one();
+ }
}
_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void acquire() {
auto const __test_fn = [this]() -> bool {
auto __old = __a_.load(memory_order_relaxed);
return (__old != 0) && __a_.compare_exchange_strong(__old, __old - 1, memory_order_acquire, memory_order_relaxed);
};
- __cxx_atomic_wait(&__a_.__a_, __test_fn);
+ auto const __test_with_old = [this](__cxx_contention_t& __monitor) -> bool {
+ ptrdiff_t __old = __monitor;
+ bool __r = __try_acquire_impl(__old);
+ __monitor = static_cast<__cxx_contention_t>(__old);
+ return __r;
+ };
+ __cxx_atomic_wait(&__a_.__a_, __test_fn, __test_with_old);
}
template <class _Rep, class _Period>
_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool
@@ -119,6 +123,11 @@ public:
}
_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool try_acquire() {
auto __old = __a_.load(memory_order_acquire);
+ return __try_acquire_impl(__old);
+ }
+
+private:
+ _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY bool __try_acquire_impl(ptrdiff_t& __old) {
while (true) {
if (__old == 0)
return false;
diff --git a/libcxx/test/std/thread/thread.semaphore/lost_wakeup.pass.cpp b/libcxx/test/std/thread/thread.semaphore/lost_wakeup.pass.cpp
new file mode 100644
index 0000000000000..aa21e93221b31
--- /dev/null
+++ b/libcxx/test/std/thread/thread.semaphore/lost_wakeup.pass.cpp
@@ -0,0 +1,65 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: libcpp-has-no-threads
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
+// This test requires the dylib support introduced in D68480, which shipped in macOS 11.0.
+// XFAIL: use_system_cxx_lib && target={{.+}}-apple-macosx10.{{9|10|11|12|13|14|15}}
+
+// This is a regression test for https://llvm.org/PR47013.
+
+// <semaphore>
+
+#include <barrier>
+#include <semaphore>
+#include <thread>
+#include <vector>
+
+#include "make_test_thread.h"
+
+static std::counting_semaphore<> s(0);
+static std::barrier<> b(8 + 1);
+
+void acquire() {
+ for (int i = 0; i < 10'000; ++i) {
+ s.acquire();
+ b.arrive_and_wait();
+ }
+}
+
+void release() {
+ for (int i = 0; i < 10'000; ++i) {
+ s.release(1);
+ s.release(1);
+ s.release(1);
+ s.release(1);
+
+ s.release(1);
+ s.release(1);
+ s.release(1);
+ s.release(1);
+
+ b.arrive_and_wait();
+ }
+}
+
+int main(int, char**) {
+ for (int run = 0; run < 20; ++run) {
+ std::vector<std::thread> threads;
+ for (int i = 0; i < 8; ++i)
+ threads.push_back(support::make_test_thread(acquire));
+
+ threads.push_back(support::make_test_thread(release));
+
+ for (auto& thread : threads)
+ thread.join();
+ }
+
+ return 0;
+}
>From 7f731e892fe91751d8691556b32f261403284c3b Mon Sep 17 00:00:00 2001
From: Hui <hui.xie0621 at gmail.com>
Date: Wed, 31 Jan 2024 16:17:28 +0000
Subject: [PATCH 2/3] address feedback
---
libcxx/include/__atomic/atomic_sync.h | 38 ++++++++++++++++-----------
libcxx/include/semaphore | 8 +++---
2 files changed, 28 insertions(+), 18 deletions(-)
diff --git a/libcxx/include/__atomic/atomic_sync.h b/libcxx/include/__atomic/atomic_sync.h
index ba83396189edf..e1994ddde86c1 100644
--- a/libcxx/include/__atomic/atomic_sync.h
+++ b/libcxx/include/__atomic/atomic_sync.h
@@ -43,17 +43,17 @@ __libcpp_atomic_monitor(__cxx_atomic_contention_t const volatile*);
_LIBCPP_AVAILABILITY_SYNC _LIBCPP_EXPORTED_FROM_ABI void
__libcpp_atomic_wait(__cxx_atomic_contention_t const volatile*, __cxx_contention_t);
-template <class _Atp, class _Fn>
+template <class _Atp, class _BackoffTest>
struct __libcpp_atomic_wait_backoff_impl {
- _Atp* __a;
- _Fn __test_with_old;
+ _Atp* __a_;
+ _BackoffTest __backoff_test_;
_LIBCPP_AVAILABILITY_SYNC
_LIBCPP_HIDE_FROM_ABI bool operator()(chrono::nanoseconds __elapsed) const {
if (__elapsed > chrono::microseconds(64)) {
- auto __monitor = std::__libcpp_atomic_monitor(__a);
- if (__test_with_old(__monitor))
+ auto __monitor = std::__libcpp_atomic_monitor(__a_);
+ if (__backoff_test_(__monitor))
return true;
- std::__libcpp_atomic_wait(__a, __monitor);
+ std::__libcpp_atomic_wait(__a_, __monitor);
} else if (__elapsed > chrono::microseconds(4))
__libcpp_thread_yield();
else {
@@ -62,18 +62,26 @@ struct __libcpp_atomic_wait_backoff_impl {
}
};
-template <class _Atp, class _Fn, class _Fn2>
+template <class _Atp, class _Poll, class _BackoffTest>
_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool
-__cxx_atomic_wait(_Atp* __a, _Fn&& __test_fn, _Fn2&& __test_with_old) {
- __libcpp_atomic_wait_backoff_impl<_Atp, __decay_t<_Fn2> > __backoff_fn = {__a, __test_with_old};
- return std::__libcpp_thread_poll_with_backoff(__test_fn, __backoff_fn);
+__cxx_atomic_wait(_Atp* __a, _Poll&& __poll, _BackoffTest&& __backoff_test) {
+ __libcpp_atomic_wait_backoff_impl<_Atp, __decay_t<_BackoffTest> > __backoff_fn = {__a, __backoff_test};
+ return std::__libcpp_thread_poll_with_backoff(__poll, __backoff_fn);
}
-template <class _Atp, class _Fn>
-_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool __cxx_atomic_wait(_Atp* __a, _Fn&& __test_fn) {
- auto __test_with_old = [&](auto&) { return __test_fn(); };
- __libcpp_atomic_wait_backoff_impl<_Atp, decltype(__test_with_old)> __backoff_fn{__a, __test_with_old};
- return std::__libcpp_thread_poll_with_backoff(__test_fn, __backoff_fn);
+template <class _Poll>
+struct __libcpp_atomic_wait_poll_as_backoff_test {
+ _Poll __poll_;
+
+ _LIBCPP_AVAILABILITY_SYNC
+ _LIBCPP_HIDE_FROM_ABI bool operator()(__cxx_contention_t&) const { return __poll_(); }
+};
+
+template <class _Atp, class _Poll>
+_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool __cxx_atomic_wait(_Atp* __a, _Poll&& __poll) {
+ __libcpp_atomic_wait_backoff_impl<_Atp, __libcpp_atomic_wait_poll_as_backoff_test<_Poll&> > __backoff_fn = {
+ __a, {__poll}};
+ return std::__libcpp_thread_poll_with_backoff(__poll, __backoff_fn);
}
#else // _LIBCPP_HAS_NO_THREADS
diff --git a/libcxx/include/semaphore b/libcxx/include/semaphore
index c4d9ee1ab63e8..2b317daa7fead 100644
--- a/libcxx/include/semaphore
+++ b/libcxx/include/semaphore
@@ -95,9 +95,11 @@ public:
auto __old = __a_.fetch_add(__update, memory_order_release);
_LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
__update <= _LIBCPP_SEMAPHORE_MAX - __old, "update is greater than the expected value");
-
- if (__old == 0) {
+ (void)__old;
+ if (__update > 1) {
__a_.notify_all();
+ } else {
+ __a_.notify_one();
}
}
_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void acquire() {
@@ -127,7 +129,7 @@ public:
}
private:
- _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY bool __try_acquire_impl(ptrdiff_t& __old) {
+ _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool __try_acquire_impl(ptrdiff_t& __old) {
while (true) {
if (__old == 0)
return false;
>From 280b9002938bfc6407292ebc3f539b68a6676146 Mon Sep 17 00:00:00 2001
From: Hui <hui.xie0621 at gmail.com>
Date: Wed, 31 Jan 2024 22:26:32 +0000
Subject: [PATCH 3/3] rename
---
libcxx/include/semaphore | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/libcxx/include/semaphore b/libcxx/include/semaphore
index 2b317daa7fead..5fd06ed09e4df 100644
--- a/libcxx/include/semaphore
+++ b/libcxx/include/semaphore
@@ -103,25 +103,25 @@ public:
}
}
_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void acquire() {
- auto const __test_fn = [this]() -> bool {
+ auto const __poll_fn = [this]() -> bool {
auto __old = __a_.load(memory_order_relaxed);
return (__old != 0) && __a_.compare_exchange_strong(__old, __old - 1, memory_order_acquire, memory_order_relaxed);
};
- auto const __test_with_old = [this](__cxx_contention_t& __monitor) -> bool {
+ auto const __backoff_test = [this](__cxx_contention_t& __monitor) -> bool {
ptrdiff_t __old = __monitor;
bool __r = __try_acquire_impl(__old);
__monitor = static_cast<__cxx_contention_t>(__old);
return __r;
};
- __cxx_atomic_wait(&__a_.__a_, __test_fn, __test_with_old);
+ __cxx_atomic_wait(&__a_.__a_, __poll_fn, __backoff_test);
}
template <class _Rep, class _Period>
_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool
try_acquire_for(chrono::duration<_Rep, _Period> const& __rel_time) {
if (__rel_time == chrono::duration<_Rep, _Period>::zero())
return try_acquire();
- auto const __test_fn = [this]() { return try_acquire(); };
- return std::__libcpp_thread_poll_with_backoff(__test_fn, __libcpp_timed_backoff_policy(), __rel_time);
+ auto const __poll_fn = [this]() { return try_acquire(); };
+ return std::__libcpp_thread_poll_with_backoff(__poll_fn, __libcpp_timed_backoff_policy(), __rel_time);
}
_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool try_acquire() {
auto __old = __a_.load(memory_order_acquire);
More information about the libcxx-commits
mailing list