Skip to content

[libc++] Refactor the predicate taking variant of __cxx_atomic_wait #80596

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Feb 19, 2024
Merged

[libc++] Refactor the predicate taking variant of __cxx_atomic_wait #80596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8e34214
refactor the predicate taking variant of '__cxx_atomic_wait'
jiixyj Feb 4, 2024
ce27d09
remove unneeded include of '__atomic/contention_t.h'
jiixyj Feb 4, 2024
6f39049
try to fix CI error
jiixyj Feb 4, 2024
0866001
CI
jiixyj Feb 4, 2024
5541162
CI
jiixyj Feb 4, 2024
f151e17
rename '_cxx_atomic_wait_fn' to '__cxx_atomic_wait_unless'
jiixyj Feb 18, 2024
2daeb53
rename '__cxx_atomic_wait_poll_fn_impl' to '__atomic_compare_unequal_to'
jiixyj Feb 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 66 additions & 35 deletions libcxx/include/__atomic/atomic_sync.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,19 @@

_LIBCPP_BEGIN_NAMESPACE_STD

template <class _Atp, class _Poll>
struct __libcpp_atomic_wait_poll_impl {
_Atp* __a_;
_Poll __poll_;
memory_order __order_;

_LIBCPP_AVAILABILITY_SYNC
_LIBCPP_HIDE_FROM_ABI bool operator()() const {
auto __current_val = std::__cxx_atomic_load(__a_, __order_);
return __poll_(__current_val);
}
};

#ifndef _LIBCPP_HAS_NO_THREADS

_LIBCPP_AVAILABILITY_SYNC _LIBCPP_EXPORTED_FROM_ABI void __cxx_atomic_notify_one(void const volatile*);
Expand All @@ -43,15 +56,40 @@ __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 _BackoffTest>
template <class _Atp, class _Poll>
struct __libcpp_atomic_wait_backoff_impl {
_Atp* __a_;
_BackoffTest __backoff_test_;
_Poll __poll_;
memory_order __order_;

_LIBCPP_AVAILABILITY_SYNC
_LIBCPP_HIDE_FROM_ABI bool
__poll_or_get_monitor(__cxx_atomic_contention_t const volatile*, __cxx_contention_t& __monitor) const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, this is roughly what @huixie90 is transforming into a CPO in #81427 .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the two __poll_or_get_monitor functions here are basically the same thing as the two __update_monitor_val_and_poll functions from #81427 (here).

// In case the atomic can be waited on directly, the monitor value is just
// the value of the atomic.
// `__poll_` takes the current value of the atomic as an in-out argument
// to potentially modify it. After it returns, `__monitor` has a value
// which can be safely waited on by `std::__libcpp_atomic_wait` without any
// ABA style issues.
__monitor = std::__cxx_atomic_load(__a_, __order_);
return __poll_(__monitor);
}

_LIBCPP_AVAILABILITY_SYNC
_LIBCPP_HIDE_FROM_ABI bool __poll_or_get_monitor(void const volatile*, __cxx_contention_t& __monitor) const {
// In case we must wait on an atomic from the pool, the monitor comes from
// `std::__libcpp_atomic_monitor`.
// Only then we may read from `__a_`. This is the "event count" pattern.
__monitor = std::__libcpp_atomic_monitor(__a_);
auto __current_val = std::__cxx_atomic_load(__a_, __order_);
return __poll_(__current_val);
}

_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 (__backoff_test_(__monitor))
__cxx_contention_t __monitor;
if (__poll_or_get_monitor(__a_, __monitor))
return true;
std::__libcpp_atomic_wait(__a_, __monitor);
} else if (__elapsed > chrono::microseconds(4))
Expand All @@ -62,26 +100,20 @@ struct __libcpp_atomic_wait_backoff_impl {
}
};

template <class _Atp, class _Poll, class _BackoffTest>
_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool
__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 _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_(); }
};

// The semantics of this function are similar to `atomic`'s
// `.wait(T old, std::memory_order order)`, but instead of having a hardcoded
// predicate (is the loaded value unequal to `old`?), the predicate function is
// specified as an argument. The loaded value is given as an in-out argument to
// the predicate. If the predicate function returns `true`,
// `_cxx_atomic_wait_unless` will return. If the predicate function returns
// `false`, it must set the argument to its current understanding of the atomic
// value. The predicate function must not return `false` spuriously.
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);
_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void
__cxx_atomic_wait_unless(_Atp* __a, _Poll&& __poll, memory_order __order) {
__libcpp_atomic_wait_poll_impl<_Atp, __decay_t<_Poll> > __poll_fn = {__a, __poll, __order};
__libcpp_atomic_wait_backoff_impl<_Atp, __decay_t<_Poll> > __backoff_fn = {__a, __poll, __order};
(void)std::__libcpp_thread_poll_with_backoff(__poll_fn, __backoff_fn);
}

#else // _LIBCPP_HAS_NO_THREADS
Expand All @@ -90,9 +122,10 @@ template <class _Tp>
_LIBCPP_HIDE_FROM_ABI void __cxx_atomic_notify_all(__cxx_atomic_impl<_Tp> const volatile*) {}
template <class _Tp>
_LIBCPP_HIDE_FROM_ABI void __cxx_atomic_notify_one(__cxx_atomic_impl<_Tp> const volatile*) {}
template <class _Atp, class _Fn>
_LIBCPP_HIDE_FROM_ABI bool __cxx_atomic_wait(_Atp*, _Fn&& __test_fn) {
return std::__libcpp_thread_poll_with_backoff(__test_fn, __spinning_backoff_policy());
template <class _Atp, class _Poll>
_LIBCPP_HIDE_FROM_ABI void __cxx_atomic_wait_unless(_Atp* __a, _Poll&& __poll, memory_order __order) {
__libcpp_atomic_wait_poll_impl<_Atp, __decay_t<_Poll> > __poll_fn = {__a, __poll, __order};
(void)std::__libcpp_thread_poll_with_backoff(__poll_fn, __spinning_backoff_policy());
}

#endif // _LIBCPP_HAS_NO_THREADS
Expand All @@ -102,21 +135,19 @@ _LIBCPP_HIDE_FROM_ABI bool __cxx_nonatomic_compare_equal(_Tp const& __lhs, _Tp c
return std::memcmp(std::addressof(__lhs), std::addressof(__rhs), sizeof(_Tp)) == 0;
}

template <class _Atp, class _Tp>
struct __cxx_atomic_wait_test_fn_impl {
_Atp* __a;
template <class _Tp>
struct __atomic_compare_unequal_to {
_Tp __val;
memory_order __order;
_LIBCPP_HIDE_FROM_ABI bool operator()() const {
return !std::__cxx_nonatomic_compare_equal(std::__cxx_atomic_load(__a, __order), __val);
_LIBCPP_HIDE_FROM_ABI bool operator()(_Tp& __current_val) const {
return !std::__cxx_nonatomic_compare_equal(__current_val, __val);
}
};

template <class _Atp, class _Tp>
_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool
_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void
__cxx_atomic_wait(_Atp* __a, _Tp const __val, memory_order __order) {
__cxx_atomic_wait_test_fn_impl<_Atp, _Tp> __test_fn = {__a, __val, __order};
return std::__cxx_atomic_wait(__a, __test_fn);
__atomic_compare_unequal_to<_Tp> __poll_fn = {__val};
std::__cxx_atomic_wait_unless(__a, __poll_fn, __order);
}

_LIBCPP_END_NAMESPACE_STD
Expand Down
11 changes: 9 additions & 2 deletions libcxx/include/latch
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,13 @@ public:
if (__old == __update)
__a_.notify_all();
}
inline _LIBCPP_HIDE_FROM_ABI bool try_wait() const noexcept { return 0 == __a_.load(memory_order_acquire); }
inline _LIBCPP_HIDE_FROM_ABI bool try_wait() const noexcept {
auto __value = __a_.load(memory_order_acquire);
return try_wait_impl(__value);
}
inline _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void wait() const {
__cxx_atomic_wait(&__a_.__a_, [this]() -> bool { return try_wait(); });
__cxx_atomic_wait_unless(
&__a_.__a_, [this](ptrdiff_t& __value) -> bool { return try_wait_impl(__value); }, memory_order_acquire);
}
inline _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void arrive_and_wait(ptrdiff_t __update = 1) {
_LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(__update >= 0, "latch::arrive_and_wait called with a negative value");
Expand All @@ -108,6 +112,9 @@ public:
count_down(__update);
wait();
}

private:
inline _LIBCPP_HIDE_FROM_ABI bool try_wait_impl(ptrdiff_t& __value) const noexcept { return __value == 0; }
};

_LIBCPP_END_NAMESPACE_STD
Expand Down
18 changes: 4 additions & 14 deletions libcxx/include/semaphore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ 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>
Expand Down Expand Up @@ -100,17 +99,8 @@ public:
}
}
_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void acquire() {
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 __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_, __poll_fn, __backoff_test);
__cxx_atomic_wait_unless(
&__a_.__a_, [this](ptrdiff_t& __old) { return __try_acquire_impl(__old); }, memory_order_relaxed);
}
template <class _Rep, class _Period>
_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool
Expand All @@ -121,7 +111,7 @@ public:
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);
auto __old = __a_.load(memory_order_relaxed);
return __try_acquire_impl(__old);
}

Expand All @@ -130,7 +120,7 @@ private:
while (true) {
if (__old == 0)
return false;
if (__a_.compare_exchange_strong(__old, __old - 1, memory_order_acquire, memory_order_relaxed))
if (__a_.compare_exchange_weak(__old, __old - 1, memory_order_acquire, memory_order_relaxed))
return true;
}
}
Expand Down
1 change: 1 addition & 0 deletions libcxx/src/atomic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ _LIBCPP_EXPORTED_FROM_ABI void __cxx_atomic_notify_one(__cxx_atomic_contention_t
_LIBCPP_EXPORTED_FROM_ABI void __cxx_atomic_notify_all(__cxx_atomic_contention_t const volatile* __location) {
__libcpp_contention_notify(&__libcpp_contention_state(__location)->__contention_state, __location, false);
}
// This function is never used, but still exported for ABI compatibility.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this because this is handled by __poll_or_get_monitor now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly. This overload of __libcpp_atomic_monitor was used to get the "monitor" value in case the ptrdiff_t of the semaphore could be waited on directly. And this was the reason for the original ABA bug, where this could happen:

  1. __libcpp_atomic_monitor returns "1" (i.e. the semaphore has the value 1)
  2. another thread swoops in, takes the "1" down to "0"
  3. "our" poll function returns "false", because there is nothing left to try_acquire
  4. another thread releases the semaphore (increases the value from "0" to "1")
  5. atomic_wait now waits on the "1", resulting in a deadlock.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And now, __poll_or_get_monitor makes sure that in this case, the "monitor" value is always "0" (because return __poll_(__monitor); sets __monitor to "0" if it returns false).

_LIBCPP_EXPORTED_FROM_ABI __cxx_contention_t
__libcpp_atomic_monitor(__cxx_atomic_contention_t const volatile* __location) {
return __libcpp_contention_monitor_for_wait(&__libcpp_contention_state(__location)->__contention_state, __location);
Expand Down