From 2f2f29391012949385631e5bee7c35fed71392dd Mon Sep 17 00:00:00 2001 From: Benoit Germain Date: Wed, 13 Nov 2024 11:05:32 +0100 Subject: Cleaning up guano * Lane::waiting on does not need to be volatile or anything else, all accesses are controlled through status acquire/release semantics * this contains fixes about bad ordering of said accesses in Linda:send and Linda:receive --- src/_pch.hpp | 1 - src/lane.cpp | 7 ++++--- src/lane.hpp | 5 ++--- src/linda.cpp | 4 ++-- 4 files changed, 8 insertions(+), 9 deletions(-) (limited to 'src') diff --git a/src/_pch.hpp b/src/_pch.hpp index 1c7b7dc..495a959 100644 --- a/src/_pch.hpp +++ b/src/_pch.hpp @@ -60,7 +60,6 @@ extern "C" #pragma warning(disable : 5027) // 'x': move assignment operator was implicitly defined as deleted #pragma warning(disable : 5039) // 'x': pointer or reference to potentially throwing function passed to 'extern "C"' function under -EHc. Undefined behavior may occur if this function throws an exception. #pragma warning(disable : 5045) // Compiler will insert Spectre mitigation for memory load if /Qspectre switch specified -#pragma warning(disable : 5220) // 'x': a non-static data member with a volatile qualified type no longer implies that compiler generated copy/move constructors and copy/move assignment operators are not trivial #pragma warning(disable : 5246) // 'x': the initialization of a subobject should be wrapped in braces #endif // _MSC_VER diff --git a/src/lane.cpp b/src/lane.cpp index 2bc3431..00f857e 100644 --- a/src/lane.cpp +++ b/src/lane.cpp @@ -945,9 +945,10 @@ CancelResult Lane::cancel(CancelOp const op_, std::chrono::time_pointthread.get_stop_source().request_stop(); } if (wakeLane_ == WakeLane::Yes) { // wake the thread so that execution returns from any pending linda operation if desired - std::condition_variable* const _waiting_on{ waiting_on }; - if (status.load(std::memory_order_acquire) == Lane::Waiting && _waiting_on != nullptr) { - _waiting_on->notify_all(); + if (status.load(std::memory_order_acquire) == Lane::Waiting) { // waiting_on is updated under control of status acquire/release semantics + if (std::condition_variable* const _waiting_on{ waiting_on }) { + _waiting_on->notify_all(); + } } } // wait until the lane stops working with its state (either Suspended or Done+) diff --git a/src/lane.hpp b/src/lane.hpp index eb51aa3..b5be9ab 100644 --- a/src/lane.hpp +++ b/src/lane.hpp @@ -50,7 +50,6 @@ enum class WakeLane Yes }; -// NOTE: values to be changed by either thread, during execution, without locking, are marked "volatile" class Lane { public: @@ -118,9 +117,9 @@ class Lane // M: sets to Pending (before launching) // S: updates -> Running/Waiting/Suspended -> Done/Error/Cancelled - std::condition_variable* volatile waiting_on{ nullptr }; - // + // accessed under control of status acquire/release semantics // When status is Waiting, points on the linda's signal the thread waits on, else nullptr + std::condition_variable* waiting_on{ nullptr }; std::atomic cancelRequest{ CancelRequest::None }; static_assert(std::atomic::is_always_lock_free); diff --git a/src/linda.cpp b/src/linda.cpp index 1536da5..99d0cbe 100644 --- a/src/linda.cpp +++ b/src/linda.cpp @@ -673,9 +673,9 @@ LUAG_FUNC(linda_receive) // change status of lane to "waiting" _prev_status = _lane->status.load(std::memory_order_acquire); // Running, most likely LUA_ASSERT(L_, _prev_status == Lane::Running); // but check, just in case - _lane->status.store(Lane::Waiting, std::memory_order_release); LUA_ASSERT(L_, _lane->waiting_on == nullptr); _lane->waiting_on = &_linda->writeHappened; + _lane->status.store(Lane::Waiting, std::memory_order_release); } // not enough data to read: wakeup when data was sent, or when timeout is reached std::unique_lock _guard{ _keeper->mutex, std::adopt_lock }; @@ -818,9 +818,9 @@ LUAG_FUNC(linda_send) // change status of lane to "waiting" _prev_status = _lane->status.load(std::memory_order_acquire); // Running, most likely LUA_ASSERT(L_, _prev_status == Lane::Running); // but check, just in case - _lane->status.store(Lane::Waiting, std::memory_order_release); LUA_ASSERT(L_, _lane->waiting_on == nullptr); _lane->waiting_on = &_linda->readHappened; + _lane->status.store(Lane::Waiting, std::memory_order_release); } // could not send because no room: wait until some data was read before trying again, or until timeout is reached std::unique_lock _guard{ _keeper->mutex, std::adopt_lock }; -- cgit v1.2.3-55-g6feb