From 0bc424cc8c33184123aff2d3106111d1366bfdc1 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Sat, 26 Jun 2021 23:20:42 -0500 Subject: [PATCH 1/2] Drop zero-timeout pthread_mutex_trylock() optimization Previously, if the mutex protecting the inner state of the event object were locked by another thread, the early abort in `WaitForEvent()` with a zero-ms timeout would have translated that to a `WAIT_TIMEOUT` result, even if the application logic is such that the event in question is always set (e.g. simultaneous calls to `WaitForEvent()` and `SetEvent()` on an already-set event). This patch makes `event->State` an atomic that can be used as a synchronization point (currently in addition to rather than instead of `event->Mutex`). Note: Unlocking the spinlock (i.e. resetting the event) is done with release semantics rather than using `std::memory_order_relaxed` due to ambiguities in the C++ spec that make it unclear whether or not consecutive Set/Reset blocks could be interleaved with the latter; see the discussion preceding [0] for reference. Closes #18. [0]: https://old.reddit.com/r/cpp/comments/g84bzv/c/fpua2yq/ --- src/pevents.cpp | 58 +++++++++++++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 24 deletions(-) diff --git a/src/pevents.cpp b/src/pevents.cpp index 880fdc2..857a626 100644 --- a/src/pevents.cpp +++ b/src/pevents.cpp @@ -12,6 +12,8 @@ #include #include #include +#include +#include #ifdef WFMO #include #include @@ -54,7 +56,7 @@ namespace neosmart { pthread_cond_t CVariable; pthread_mutex_t Mutex; bool AutoReset; - bool State; + std::atomic State; #ifdef WFMO std::deque RegisteredWaits; #endif @@ -100,7 +102,8 @@ namespace neosmart { result = pthread_mutex_init(&event->Mutex, 0); assert(result == 0); - event->State = false; + // memory_order_relaxed: Newly created event is guaranteed to not have any waiters + event->State.store(false, std::memory_order_relaxed); event->AutoReset = !manualReset; if (initialState) { @@ -113,7 +116,8 @@ namespace neosmart { static int UnlockedWaitForEvent(neosmart_event_t event, uint64_t milliseconds) { int result = 0; - if (!event->State) { + // memory_order_relaxed: unlocking/ordering is guaranteed prior to calling this function + if (!event->State.load(std::memory_order_relaxed)) { // Zero-timeout event state check optimization if (milliseconds == 0) { return WAIT_TIMEOUT; @@ -139,17 +143,20 @@ namespace neosmart { } else { result = pthread_cond_wait(&event->CVariable, &event->Mutex); } - } while (result == 0 && !event->State); + // memory_order_relaxed: ordering is guaranteed by the mutex + } while (result == 0 && !event->State.load(std::memory_order_relaxed)); if (result == 0 && event->AutoReset) { // We've only accquired the event if the wait succeeded - event->State = false; + // memory_order_release: Prevent overlapping/interleaved Set/Reset contexts + event->State.store(false, std::memory_order_release); } } else if (event->AutoReset) { // It's an auto-reset event that's currently available; // we need to stop anyone else from using it result = 0; - event->State = false; + // memory_order_release: Prevent overlapping/interleaved Set/Reset contexts + event->State.store(false, std::memory_order_release); } // Else we're trying to obtain a manual reset event with a signaled state; // don't do anything @@ -158,21 +165,19 @@ namespace neosmart { } int WaitForEvent(neosmart_event_t event, uint64_t milliseconds) { - int tempResult; - if (milliseconds == 0) { - tempResult = pthread_mutex_trylock(&event->Mutex); - if (tempResult == EBUSY) { - return WAIT_TIMEOUT; - } + // Optimization: bypass acquiring the event lock if the state atomic is unavailable. + // memory_order_relaxed: This is just an optimization, it's OK to be biased towards a stale + // value here, and preferable to synchronizing CPU caches to get a more accurate result. + if (milliseconds == 0 && !event->State.load(std::memory_order_relaxed)) { + return WAIT_TIMEOUT; } else { - tempResult = pthread_mutex_lock(&event->Mutex); + int result = pthread_mutex_lock(&event->Mutex); + assert(result == 0); } - assert(tempResult == 0); - int result = UnlockedWaitForEvent(event, milliseconds); - tempResult = pthread_mutex_unlock(&event->Mutex); + int tempResult = pthread_mutex_unlock(&event->Mutex); assert(tempResult == 0); return result; @@ -344,7 +349,8 @@ namespace neosmart { int result = pthread_mutex_lock(&event->Mutex); assert(result == 0); - event->State = true; + // memory_order_release: Unblock threads waiting for the event + event->State.store(true, std::memory_order_release); // Depending on the event type, we either trigger everyone or only one if (event->AutoReset) { @@ -369,8 +375,6 @@ namespace neosmart { continue; } - event->State = false; - if (i->Waiter->WaitAll) { --i->Waiter->Status.EventsLeft; assert(i->Waiter->Status.EventsLeft >= 0); @@ -391,14 +395,18 @@ namespace neosmart { event->RegisteredWaits.pop_front(); + // memory_order_release: Prevent overlapping of sequential Set/Reset states. + event->State.store(false, std::memory_order_release); + result = pthread_mutex_unlock(&event->Mutex); assert(result == 0); return 0; } #endif // WFMO - // event->State can be false if compiled with WFMO support - if (event->State) { + // event->State can be false if compiled with WFMO support + // memory_order_relaxed: ordering is ensured by the mutex + if (event->State.load(std::memory_order_relaxed)) { result = pthread_mutex_unlock(&event->Mutex); assert(result == 0); @@ -407,7 +415,7 @@ namespace neosmart { return 0; } - } else { + } else { // this is a manual reset event #ifdef WFMO for (size_t i = 0; i < event->RegisteredWaits.size(); ++i) { neosmart_wfmo_info_t info = &event->RegisteredWaits[i]; @@ -463,7 +471,9 @@ namespace neosmart { int result = pthread_mutex_lock(&event->Mutex); assert(result == 0); - event->State = false; + // memory_order_release: Prevent sequential Set/Reset calls from overlapping, this seems to + // be required per https://old.reddit.com/r/cpp/comments/g84bzv/c/fpua2yq/ + event->State.store(false, std::memory_order_release); result = pthread_mutex_unlock(&event->Mutex); assert(result == 0); @@ -493,7 +503,7 @@ namespace neosmart { #endif } // namespace neosmart -#else //_WIN32 +#else // _WIN32 #include #include "pevents.h" From 11a56d06205c2ba0cad9828d4fb471e1d33d75bc Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Sun, 27 Jun 2021 15:19:13 -0500 Subject: [PATCH 2/2] Add regression test for WAIT_TIMEOUT under mutex contention Test case based off the pseudocode provided by @qwertymaster617 in #18. --- meson.build | 1 + tests/EventContention.cpp | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+) create mode 100644 tests/EventContention.cpp diff --git a/meson.build b/meson.build index aecb5db..b5e6b79 100644 --- a/meson.build +++ b/meson.build @@ -28,6 +28,7 @@ basic_tests = ['ManualResetInitialState', 'AutoResetInitialState', 'ManualResetBasicTests', 'AutoResetBasicTests', + 'EventContention', ] # Tests that required wfmo wfmo_tests = [ diff --git a/tests/EventContention.cpp b/tests/EventContention.cpp new file mode 100644 index 0000000..828c226 --- /dev/null +++ b/tests/EventContention.cpp @@ -0,0 +1,39 @@ +// This tests contention on an auto-reset event that is always available at a high level, to verify +// that other threads acquiring the protective pthread mutex don't result in a spurious WAIT_TIMEOUT +// error for any `WaitForEvent()` callers. +// +// See https://github.com/neosmart/pevents/issues/18 + +#ifdef _WIN32 +#include +#endif +#include +#include +#include +#include + +using namespace neosmart; + +int main() { + // Create an auto-reset event that is initially signalled + neosmart_event_t event = CreateEvent(false, true); + + + // Create n threads to constantly call SetEvent() in a tight loop + for (int i = 0; i < 16; ++i) { + std::thread t1([&] { + SetEvent(event); + }); + t1.detach(); + } + + // Call WaitForEvent() in a tight loop; we can expect it to always be available. + for (int i = 0; i < 200000; ++i) { + int result = WaitForEvent(event, 0); + assert(result == 0); + // Guarantee this thread always calls `WaitForEvent()` on a signalled event + SetEvent(event); + } + + return 0; +}