pw_sync_threadx: Rolls back the ThreadNotification backend
It turns out the approach chosen to attempt to implement an optimized
ThreadNotification backend on ThreadX will fundamentally never work.
Unfortunately the kernel must have a saturating thread notification
mechanism which permits a thread to start receiving potential
notifications when the thread handle is registered. This is not
possible with the ThreadX API.
Lastly, event flags are equal to or larger than the binary
semaphores, and ergo we recommend using the binary semaphore backend
for thread notifications.
Change-Id: Ifa149296dd77b60b303552c0c008dee2cea46fc2
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/68180
Reviewed-by: Keir Mierle <keir@google.com>
Commit-Queue: Ewout van Bekkum <ewout@google.com>
Pigweed-Auto-Submit: Ewout van Bekkum <ewout@google.com>
diff --git a/pw_sync_threadx/BUILD.bazel b/pw_sync_threadx/BUILD.bazel
index 3cf1a1e..cb78f9e 100644
--- a/pw_sync_threadx/BUILD.bazel
+++ b/pw_sync_threadx/BUILD.bazel
@@ -132,85 +132,6 @@
)
pw_cc_library(
- name = "thread_notification_headers",
- hdrs = [
- "public/pw_sync_threadx/config.h",
- "public/pw_sync_threadx/thread_notification_inline.h",
- "public/pw_sync_threadx/thread_notification_native.h",
- "public_overrides/thread_notification/pw_sync_backend/thread_notification_inline.h",
- "public_overrides/thread_notification/pw_sync_backend/thread_notification_native.h",
- ],
- includes = [
- "public",
- "public_overrides/thread_notification",
- ],
- target_compatible_with = [
- "//pw_build/constraints/rtos:threadx",
- ],
- deps = [
- # TODO(pwbug/317): This should depend on ThreadX but our third parties
- # currently do not have Bazel support.
- "//pw_assert",
- "//pw_interrupt:context",
- "//pw_sync:interrupt_spin_lock",
- ],
-)
-
-pw_cc_library(
- name = "thread_notification",
- srcs = [
- "thread_notification.cc",
- ],
- target_compatible_with = [
- "//pw_build/constraints/rtos:threadx",
- ],
- deps = [
- ":thread_notification_headers",
- "//pw_assert",
- "//pw_interrupt:context",
- "//pw_sync:thread_notification_facade",
- ],
-)
-
-pw_cc_library(
- name = "timed_thread_notification_headers",
- hdrs = [
- "public/pw_sync_threadx/timed_thread_notification_inline.h",
- "public_overrides/timed_thread_notification/pw_sync_backend/timed_thread_notification_inline.h",
- ],
- includes = [
- "public",
- "public_overrides/timed_thread_notification",
- ],
- target_compatible_with = [
- "//pw_build/constraints/rtos:threadx",
- ],
- deps = [
- # TODO(pwbug/317): This should depend on ThreadX but our third parties
- # currently do not have Bazel support.
- "//pw_chrono:system_clock",
- "//pw_sync:timed_thread_notification_facade",
- ],
-)
-
-pw_cc_library(
- name = "timed_thread_notification",
- srcs = [
- "timed_thread_notification.cc",
- ],
- target_compatible_with = [
- "//pw_build/constraints/rtos:threadx",
- ],
- deps = [
- ":timed_thread_notification_headers",
- "//pw_assert",
- "//pw_chrono_threadx:system_clock_headers",
- "//pw_interrupt:context",
- "//pw_sync:timed_thread_notification_facade",
- ],
-)
-
-pw_cc_library(
name = "timed_mutex_headers",
hdrs = [
"public/pw_sync_threadx/timed_mutex_inline.h",
diff --git a/pw_sync_threadx/BUILD.gn b/pw_sync_threadx/BUILD.gn
index a258940..afeebe3 100644
--- a/pw_sync_threadx/BUILD.gn
+++ b/pw_sync_threadx/BUILD.gn
@@ -49,34 +49,6 @@
]
}
-config("public_overrides_thread_notification_include_path") {
- include_dirs = [ "public_overrides/thread_notification" ]
- visibility = [ ":thread_notification" ]
-}
-
-# This target provides the backend for pw::sync::ThreadNotification based on
-# tx_thread_sleep and tx_thread_wait_abort.
-pw_source_set("thread_notification") {
- public_configs = [
- ":public_include_path",
- ":public_overrides_thread_notification_include_path",
- ]
- public = [
- "public/pw_sync_threadx/thread_notification_inline.h",
- "public/pw_sync_threadx/thread_notification_native.h",
- "public_overrides/thread_notification/pw_sync_backend/thread_notification_inline.h",
- "public_overrides/thread_notification/pw_sync_backend/thread_notification_native.h",
- ]
- public_deps = [
- "$dir_pw_assert",
- "$dir_pw_interrupt:context",
- "$dir_pw_sync:interrupt_spin_lock",
- "$dir_pw_sync:thread_notification.facade",
- "$dir_pw_third_party/threadx",
- ]
- sources = [ "thread_notification.cc" ]
-}
-
if (pw_chrono_SYSTEM_CLOCK_BACKEND != "") {
# This target provides the backend for pw::sync::BinarySemaphore.
pw_source_set("binary_semaphore") {
@@ -139,41 +111,6 @@
"the ThreadX pw::chrono::SystemClock backend.")
}
- config("public_overrides_timed_thread_notification_include_path") {
- include_dirs = [ "public_overrides/timed_thread_notification" ]
- visibility = [ ":timed_thread_notification" ]
- }
-
- # This target provides the backend for pw::sync::TimedThreadNotification based
- # tx_thread_sleep and tx_thread_wait_abort.
- pw_source_set("timed_thread_notification") {
- public_configs = [
- ":public_include_path",
- ":public_overrides_timed_thread_notification_include_path",
- ]
- public = [
- "public/pw_sync_threadx/timed_thread_notification_inline.h",
- "public_overrides/timed_thread_notification/pw_sync_backend/timed_thread_notification_inline.h",
- ]
- public_deps = [
- ":thread_notification",
- "$dir_pw_chrono:system_clock",
- "$dir_pw_sync:timed_thread_notification.facade",
- ]
- sources = [ "timed_thread_notification.cc" ]
- deps = [
- "$dir_pw_assert",
- "$dir_pw_interrupt:context",
- "$dir_pw_third_party/threadx",
- pw_chrono_SYSTEM_CLOCK_BACKEND,
- ]
- assert(pw_sync_OVERRIDE_SYSTEM_CLOCK_BACKEND_CHECK ||
- pw_chrono_SYSTEM_CLOCK_BACKEND ==
- "$dir_pw_chrono_threadx:system_clock",
- "The ThreadX pw::sync::TimedMutex backend only works with the " +
- "ThreadX pw::chrono::SystemClock backend.")
- }
-
# This target provides the backend for pw::sync::TimedMutex.
pw_source_set("timed_mutex") {
public_configs = [
diff --git a/pw_sync_threadx/docs.rst b/pw_sync_threadx/docs.rst
index 1a3daa8..ec17537 100644
--- a/pw_sync_threadx/docs.rst
+++ b/pw_sync_threadx/docs.rst
@@ -40,29 +40,20 @@
ThreadNotification & TimedThreadNotification
============================================
-An optimized ThreadX backend for the ThreadNotification and
-TimedThreadNotification is provided using ``tx_thread_sleep`` and
-``tx_thread_wait_abort``. It is backed by a ``TX_THREAD*`` and a ``bool`` which
-permits these objects to track the notification value per instance. In addition,
-this backend relies on a global singleton InterruptSpinLock to provide thread
-safety in a way which is SMP compatible.
+The native ThreadX API does cover direct thread signaling and ergo we recommend
+using the binary semaphore backends for ThreadNotifications:
+- ``pw_sync:binary_semaphore_thread_notification_backend``
+- ``pw_sync:binary_semaphore_timed_thread_notification_backend``
-Design Notes
-------------
-Because ThreadX can support SMP systems, we need a lock which permits spinning
-to safely mutate the notification value and whether the thread is blocked.
-This could be allocated per ThreadNotification instance, however this cost
-quickly adds up with a large number of instances. In addition, the critical
-sections are reasonably short.
-
-For those reasons, we opted to go with a single global interrupt spin lock. This
-is the minimal memory footprint solution, perfect for uniprocessor systems. In
-addition, given that we do not expect any serious contention for most of our
-SMP users, we believe this is a good trade-off.
-
-On very large SMP systems which heavily rely on ThreadNotifications, one could
-consider moving the InterruptSpinLock to the ThreadNotification instance if
-contention on this global lock becomes a problem.
+Background Information
+----------------------
+Although one may be tempted to use ``tx_thread_sleep`` and
+``tx_thread_wait_abort`` to implement direct thread notifications with ThreadX,
+this unfortunately cannot work. Between the blocking thread setting its
+``TX_THREAD*`` handle and actually executing ``tx_thread_sleep`` there will
+always exist a race condition. Another thread and/or interrupt may attempt
+to invoke ``tx_thread_wait_abort`` before the blocking thread has executed
+``tx_thread_sleep`` meaning the wait abort would fail.
BinarySemaphore & CountingSemaphore
===================================
diff --git a/pw_sync_threadx/public/pw_sync_threadx/thread_notification_inline.h b/pw_sync_threadx/public/pw_sync_threadx/thread_notification_inline.h
deleted file mode 100644
index db2f5a8..0000000
--- a/pw_sync_threadx/public/pw_sync_threadx/thread_notification_inline.h
+++ /dev/null
@@ -1,58 +0,0 @@
-// Copyright 2021 The Pigweed Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License"); you may not
-// use this file except in compliance with the License. You may obtain a copy of
-// the License at
-//
-// https://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
-// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
-// License for the specific language governing permissions and limitations under
-// the License.
-#pragma once
-
-#include <mutex>
-
-#include "pw_assert/assert.h"
-#include "pw_interrupt/context.h"
-#include "pw_sync/thread_notification.h"
-#include "tx_api.h"
-
-namespace pw::sync {
-
-inline ThreadNotification::ThreadNotification()
- : native_type_{
- .blocked_thread = nullptr,
- .notified = false,
- } {}
-
-inline ThreadNotification::~ThreadNotification() = default;
-
-inline bool ThreadNotification::try_acquire() {
- // Enforce the pw::sync::ThreadNotification IRQ contract.
- PW_DASSERT(!interrupt::InInterruptContext());
- {
- std::lock_guard lock(backend::thread_notification_isl);
- const bool notified = native_type_.notified;
- native_type_.notified = false;
- return notified;
- }
-}
-
-inline void ThreadNotification::release() {
- std::lock_guard lock(backend::thread_notification_isl);
- if (native_type_.blocked_thread != nullptr) {
- PW_ASSERT(tx_thread_wait_abort(native_type_.blocked_thread) == TX_SUCCESS);
- native_type_.blocked_thread = nullptr;
- }
- native_type_.notified = true;
-}
-
-inline ThreadNotification::native_handle_type
-ThreadNotification::native_handle() {
- return native_type_;
-}
-
-} // namespace pw::sync
diff --git a/pw_sync_threadx/public/pw_sync_threadx/thread_notification_native.h b/pw_sync_threadx/public/pw_sync_threadx/thread_notification_native.h
deleted file mode 100644
index fc04b01..0000000
--- a/pw_sync_threadx/public/pw_sync_threadx/thread_notification_native.h
+++ /dev/null
@@ -1,29 +0,0 @@
-// Copyright 2021 The Pigweed Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License"); you may not
-// use this file except in compliance with the License. You may obtain a copy of
-// the License at
-//
-// https://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
-// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
-// License for the specific language governing permissions and limitations under
-// the License.
-#pragma once
-
-#include "pw_sync/interrupt_spin_lock.h"
-#include "tx_api.h"
-
-namespace pw::sync::backend {
-
-extern InterruptSpinLock thread_notification_isl;
-
-struct NativeThreadNotification {
- TX_THREAD* blocked_thread;
- bool notified;
-};
-using NativeThreadNotificationHandle = NativeThreadNotification&;
-
-} // namespace pw::sync::backend
diff --git a/pw_sync_threadx/public/pw_sync_threadx/timed_thread_notification_inline.h b/pw_sync_threadx/public/pw_sync_threadx/timed_thread_notification_inline.h
deleted file mode 100644
index 543fac8..0000000
--- a/pw_sync_threadx/public/pw_sync_threadx/timed_thread_notification_inline.h
+++ /dev/null
@@ -1,29 +0,0 @@
-
-// Copyright 2021 The Pigweed Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License"); you may not
-// use this file except in compliance with the License. You may obtain a copy of
-// the License at
-//
-// https://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
-// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
-// License for the specific language governing permissions and limitations under
-// the License.
-#pragma once
-
-#include "pw_chrono/system_clock.h"
-#include "pw_sync/timed_thread_notification.h"
-
-namespace pw::sync {
-
-inline bool TimedThreadNotification::try_acquire_until(
- chrono::SystemClock::time_point deadline) {
- // Note that if this deadline is in the future, it will get rounded up by
- // one whole tick due to how try_lock_for is implemented.
- return try_acquire_for(deadline - chrono::SystemClock::now());
-}
-
-} // namespace pw::sync
diff --git a/pw_sync_threadx/public_overrides/thread_notification/pw_sync_backend/thread_notification_inline.h b/pw_sync_threadx/public_overrides/thread_notification/pw_sync_backend/thread_notification_inline.h
deleted file mode 100644
index cba88e3..0000000
--- a/pw_sync_threadx/public_overrides/thread_notification/pw_sync_backend/thread_notification_inline.h
+++ /dev/null
@@ -1,16 +0,0 @@
-// Copyright 2021 The Pigweed Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License"); you may not
-// use this file except in compliance with the License. You may obtain a copy of
-// the License at
-//
-// https://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
-// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
-// License for the specific language governing permissions and limitations under
-// the License.
-#pragma once
-
-#include "pw_sync_threadx/thread_notification_inline.h"
diff --git a/pw_sync_threadx/public_overrides/thread_notification/pw_sync_backend/thread_notification_native.h b/pw_sync_threadx/public_overrides/thread_notification/pw_sync_backend/thread_notification_native.h
deleted file mode 100644
index 7df0e9a..0000000
--- a/pw_sync_threadx/public_overrides/thread_notification/pw_sync_backend/thread_notification_native.h
+++ /dev/null
@@ -1,16 +0,0 @@
-// Copyright 2021 The Pigweed Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License"); you may not
-// use this file except in compliance with the License. You may obtain a copy of
-// the License at
-//
-// https://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
-// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
-// License for the specific language governing permissions and limitations under
-// the License.
-#pragma once
-
-#include "pw_sync_threadx/thread_notification_native.h"
diff --git a/pw_sync_threadx/public_overrides/timed_thread_notification/pw_sync_backend/timed_thread_notification_inline.h b/pw_sync_threadx/public_overrides/timed_thread_notification/pw_sync_backend/timed_thread_notification_inline.h
deleted file mode 100644
index ff4a908..0000000
--- a/pw_sync_threadx/public_overrides/timed_thread_notification/pw_sync_backend/timed_thread_notification_inline.h
+++ /dev/null
@@ -1,16 +0,0 @@
-// Copyright 2021 The Pigweed Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License"); you may not
-// use this file except in compliance with the License. You may obtain a copy of
-// the License at
-//
-// https://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
-// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
-// License for the specific language governing permissions and limitations under
-// the License.
-#pragma once
-
-#include "pw_sync_threadx/timed_thread_notification_inline.h"
diff --git a/pw_sync_threadx/thread_notification.cc b/pw_sync_threadx/thread_notification.cc
deleted file mode 100644
index b9c6de8..0000000
--- a/pw_sync_threadx/thread_notification.cc
+++ /dev/null
@@ -1,59 +0,0 @@
-// Copyright 2020 The Pigweed Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License"); you may not
-// use this file except in compliance with the License. You may obtain a copy of
-// the License at
-//
-// https://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
-// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
-// License for the specific language governing permissions and limitations under
-// the License.
-
-#include "pw_sync/thread_notification.h"
-
-#include <mutex>
-
-#include "pw_assert/check.h"
-#include "pw_interrupt/context.h"
-#include "pw_sync/interrupt_spin_lock.h"
-#include "tx_api.h"
-
-namespace pw::sync {
-namespace backend {
-
-InterruptSpinLock thread_notification_isl;
-
-} // namespace backend
-
-void ThreadNotification::acquire() {
- // Enforce the pw::sync::ThreadNotification IRQ contract.
- PW_DCHECK(!interrupt::InInterruptContext());
-
- // Enforce that only a single thread can block at a time.
- PW_DCHECK(native_type_.blocked_thread == nullptr);
-
- {
- std::lock_guard lock(backend::thread_notification_isl);
- if (native_type_.notified) {
- native_type_.notified = false;
- return;
- }
- // Not notified yet, register the thread for a one-time notification.
- native_type_.blocked_thread = tx_thread_identify();
- }
-
- PW_CHECK_UINT_EQ(tx_thread_sleep(TX_WAIT_FOREVER), TX_WAIT_ABORTED);
- {
- // The thread pointer was cleared by the notifier.
- // Note that this may hide another notification, however this is considered
- // a form of notification saturation just like as if this happened before
- // acquire() was invoked.
- std::lock_guard lock(backend::thread_notification_isl);
- native_type_.notified = false;
- }
-}
-
-} // namespace pw::sync
diff --git a/pw_sync_threadx/timed_thread_notification.cc b/pw_sync_threadx/timed_thread_notification.cc
deleted file mode 100644
index 882147b..0000000
--- a/pw_sync_threadx/timed_thread_notification.cc
+++ /dev/null
@@ -1,103 +0,0 @@
-// Copyright 2020 The Pigweed Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License"); you may not
-// use this file except in compliance with the License. You may obtain a copy of
-// the License at
-//
-// https://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
-// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
-// License for the specific language governing permissions and limitations under
-// the License.
-
-#include "pw_sync/timed_thread_notification.h"
-
-#include <mutex>
-
-#include "pw_assert/check.h"
-#include "pw_chrono/system_clock.h"
-#include "pw_chrono_threadx/system_clock_constants.h"
-#include "pw_interrupt/context.h"
-#include "tx_api.h"
-
-namespace pw::sync {
-namespace {
-
-using pw::chrono::SystemClock;
-
-} // namespace
-
-bool TimedThreadNotification::try_acquire_for(SystemClock::duration timeout) {
- // Enforce the TimedThreadNotification IRQ contract.
- PW_DCHECK(!interrupt::InInterruptContext());
-
- // Enforce that only a single thread can block at a time.
- PW_DCHECK(native_handle().blocked_thread == nullptr);
-
- {
- std::lock_guard lock(backend::thread_notification_isl);
- const bool notified = native_handle().notified;
- // Don't block for negative or zero length durations.
- if (notified || timeout <= SystemClock::duration::zero()) {
- native_handle().notified = false;
- return notified;
- }
- // Not notified yet, register the thread for a one-time notification.
- native_handle().blocked_thread = tx_thread_identify();
- }
-
- const bool notified = [&]() {
- // In case the timeout is too long for us to express through the native
- // ThreadX API, we repeatedly wait with shorter durations. Note that on a
- // tick based kernel we cannot tell how far along we are on the current
- // tick, ergo we add one whole tick to the final duration. However, this
- // also means that the loop must ensure that timeout + 1 is less than the
- // max timeout.
- constexpr SystemClock::duration kMaxTimeoutMinusOne =
- pw::chrono::threadx::kMaxTimeout - SystemClock::duration(1);
- // In case the timeout is too long for us to express through the native
- // ThreadX API, we repeatedly wait with shorter durations.
- while (timeout > kMaxTimeoutMinusOne) {
- const UINT result =
- tx_thread_sleep(static_cast<ULONG>(kMaxTimeoutMinusOne.count()));
- if (result != TX_SUCCESS) {
- PW_CHECK_UINT_EQ(TX_WAIT_ABORTED, result);
- return true;
- }
- timeout -= kMaxTimeoutMinusOne;
- }
-
- // On a tick based kernel we cannot tell how far along we are on the current
- // tick, ergo we add one whole tick to the final duration.
- const UINT result =
- tx_thread_sleep(static_cast<ULONG>(timeout.count() + 1));
- if (result == TX_SUCCESS) {
- return false;
- }
- PW_CHECK_UINT_EQ(TX_WAIT_ABORTED, result);
- return true;
- }();
-
- {
- std::lock_guard lock(backend::thread_notification_isl);
- if (notified) {
- // Note that this may hide another notification, however this is
- // considered form of notification saturation just like as if this
- // happened before acquire() was invoked.
- native_handle().notified = false;
- // The thread pointer was cleared by the notifier.
- } else {
- // Note that we do NOT want to clear the notified value so the next call
- // can detect the notification which came after we timed out but before
- // this critical section.
- //
- // However, we do need to clear the thread pointer if we weren't notified.
- native_handle().blocked_thread = nullptr;
- }
- }
- return notified;
-}
-
-} // namespace pw::sync
