diff options
author | Benoit Germain <benoit.germain@ubisoft.com> | 2024-11-13 11:05:32 +0100 |
---|---|---|
committer | Benoit Germain <benoit.germain@ubisoft.com> | 2024-11-13 11:05:32 +0100 |
commit | 2f2f29391012949385631e5bee7c35fed71392dd (patch) | |
tree | 382a2bbaa7dcbf5bda3ca322626d29d539f6f738 | |
parent | f2a3c033fc31332e78aa45d2d9deaf51359b584a (diff) | |
download | lanes-2f2f29391012949385631e5bee7c35fed71392dd.tar.gz lanes-2f2f29391012949385631e5bee7c35fed71392dd.tar.bz2 lanes-2f2f29391012949385631e5bee7c35fed71392dd.zip |
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
-rw-r--r-- | src/_pch.hpp | 1 | ||||
-rw-r--r-- | src/lane.cpp | 7 | ||||
-rw-r--r-- | src/lane.hpp | 5 | ||||
-rw-r--r-- | src/linda.cpp | 4 |
4 files changed, 8 insertions, 9 deletions
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" | |||
60 | #pragma warning(disable : 5027) // 'x': move assignment operator was implicitly defined as deleted | 60 | #pragma warning(disable : 5027) // 'x': move assignment operator was implicitly defined as deleted |
61 | #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. | 61 | #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. |
62 | #pragma warning(disable : 5045) // Compiler will insert Spectre mitigation for memory load if /Qspectre switch specified | 62 | #pragma warning(disable : 5045) // Compiler will insert Spectre mitigation for memory load if /Qspectre switch specified |
63 | #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 | ||
64 | #pragma warning(disable : 5246) // 'x': the initialization of a subobject should be wrapped in braces | 63 | #pragma warning(disable : 5246) // 'x': the initialization of a subobject should be wrapped in braces |
65 | 64 | ||
66 | #endif // _MSC_VER | 65 | #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_point<std::chron | |||
945 | // lane_->thread.get_stop_source().request_stop(); | 945 | // lane_->thread.get_stop_source().request_stop(); |
946 | } | 946 | } |
947 | if (wakeLane_ == WakeLane::Yes) { // wake the thread so that execution returns from any pending linda operation if desired | 947 | if (wakeLane_ == WakeLane::Yes) { // wake the thread so that execution returns from any pending linda operation if desired |
948 | std::condition_variable* const _waiting_on{ waiting_on }; | 948 | if (status.load(std::memory_order_acquire) == Lane::Waiting) { // waiting_on is updated under control of status acquire/release semantics |
949 | if (status.load(std::memory_order_acquire) == Lane::Waiting && _waiting_on != nullptr) { | 949 | if (std::condition_variable* const _waiting_on{ waiting_on }) { |
950 | _waiting_on->notify_all(); | 950 | _waiting_on->notify_all(); |
951 | } | ||
951 | } | 952 | } |
952 | } | 953 | } |
953 | // wait until the lane stops working with its state (either Suspended or Done+) | 954 | // 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 | |||
50 | Yes | 50 | Yes |
51 | }; | 51 | }; |
52 | 52 | ||
53 | // NOTE: values to be changed by either thread, during execution, without locking, are marked "volatile" | ||
54 | class Lane | 53 | class Lane |
55 | { | 54 | { |
56 | public: | 55 | public: |
@@ -118,9 +117,9 @@ class Lane | |||
118 | // M: sets to Pending (before launching) | 117 | // M: sets to Pending (before launching) |
119 | // S: updates -> Running/Waiting/Suspended -> Done/Error/Cancelled | 118 | // S: updates -> Running/Waiting/Suspended -> Done/Error/Cancelled |
120 | 119 | ||
121 | std::condition_variable* volatile waiting_on{ nullptr }; | 120 | // accessed under control of status acquire/release semantics |
122 | // | ||
123 | // When status is Waiting, points on the linda's signal the thread waits on, else nullptr | 121 | // When status is Waiting, points on the linda's signal the thread waits on, else nullptr |
122 | std::condition_variable* waiting_on{ nullptr }; | ||
124 | 123 | ||
125 | std::atomic<CancelRequest> cancelRequest{ CancelRequest::None }; | 124 | std::atomic<CancelRequest> cancelRequest{ CancelRequest::None }; |
126 | static_assert(std::atomic<CancelRequest>::is_always_lock_free); | 125 | static_assert(std::atomic<CancelRequest>::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) | |||
673 | // change status of lane to "waiting" | 673 | // change status of lane to "waiting" |
674 | _prev_status = _lane->status.load(std::memory_order_acquire); // Running, most likely | 674 | _prev_status = _lane->status.load(std::memory_order_acquire); // Running, most likely |
675 | LUA_ASSERT(L_, _prev_status == Lane::Running); // but check, just in case | 675 | LUA_ASSERT(L_, _prev_status == Lane::Running); // but check, just in case |
676 | _lane->status.store(Lane::Waiting, std::memory_order_release); | ||
677 | LUA_ASSERT(L_, _lane->waiting_on == nullptr); | 676 | LUA_ASSERT(L_, _lane->waiting_on == nullptr); |
678 | _lane->waiting_on = &_linda->writeHappened; | 677 | _lane->waiting_on = &_linda->writeHappened; |
678 | _lane->status.store(Lane::Waiting, std::memory_order_release); | ||
679 | } | 679 | } |
680 | // not enough data to read: wakeup when data was sent, or when timeout is reached | 680 | // not enough data to read: wakeup when data was sent, or when timeout is reached |
681 | std::unique_lock<std::mutex> _guard{ _keeper->mutex, std::adopt_lock }; | 681 | std::unique_lock<std::mutex> _guard{ _keeper->mutex, std::adopt_lock }; |
@@ -818,9 +818,9 @@ LUAG_FUNC(linda_send) | |||
818 | // change status of lane to "waiting" | 818 | // change status of lane to "waiting" |
819 | _prev_status = _lane->status.load(std::memory_order_acquire); // Running, most likely | 819 | _prev_status = _lane->status.load(std::memory_order_acquire); // Running, most likely |
820 | LUA_ASSERT(L_, _prev_status == Lane::Running); // but check, just in case | 820 | LUA_ASSERT(L_, _prev_status == Lane::Running); // but check, just in case |
821 | _lane->status.store(Lane::Waiting, std::memory_order_release); | ||
822 | LUA_ASSERT(L_, _lane->waiting_on == nullptr); | 821 | LUA_ASSERT(L_, _lane->waiting_on == nullptr); |
823 | _lane->waiting_on = &_linda->readHappened; | 822 | _lane->waiting_on = &_linda->readHappened; |
823 | _lane->status.store(Lane::Waiting, std::memory_order_release); | ||
824 | } | 824 | } |
825 | // could not send because no room: wait until some data was read before trying again, or until timeout is reached | 825 | // could not send because no room: wait until some data was read before trying again, or until timeout is reached |
826 | std::unique_lock<std::mutex> _guard{ _keeper->mutex, std::adopt_lock }; | 826 | std::unique_lock<std::mutex> _guard{ _keeper->mutex, std::adopt_lock }; |