From 50a1d28b47a9cbeb6e839cb46126b82e4fcba3f3 Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Thu, 20 Nov 2025 11:17:40 +0100 Subject: [PATCH 1/2] core/thread_flags: change internal API All uses of thread_flags_wake() also had to set the flags anyway, so we can just combine those operations into a new thread_flags_set_internal() and update the users to use that instead. --- core/include/thread_flags.h | 25 ++++++++++++++++--------- core/msg.c | 6 ++---- core/thread_flags.c | 11 ++++++----- 3 files changed, 24 insertions(+), 18 deletions(-) diff --git a/core/include/thread_flags.h b/core/include/thread_flags.h index 93bc0c5504..90937bc7a8 100644 --- a/core/include/thread_flags.h +++ b/core/include/thread_flags.h @@ -182,19 +182,26 @@ thread_flags_t thread_flags_wait_all(thread_flags_t mask); thread_flags_t thread_flags_wait_one(thread_flags_t mask); /** - * @brief Possibly Wake up thread waiting for flags - * - * Wakes up a thread if it is thread flag blocked and its condition is met. - * Has to be called with interrupts disabled. - * Does not trigger yield. + * @brief Set the flags of the given thread and update its state, but do not + * yield * * @internal * - * @param[in] thread thread to possibly wake up - * @return 1 if @p thread has been woken up - * 0 otherwise + * @warning This is not a stable API intended for external use. + * + * @param[in,out] thread Thread to modify + * @param[in] mask Bitmask containing the flags to set + * + * @retval true The thread in @p thread has changed its state to pending, + * the caller needs to yield + * @retval false The thread in @p has not changed its state. It may already + * be pending or is not waiting on the flags in @p mask + * + * @pre The caller has IRQ disabled. + * @pre @p thread is not `NULL` + * @pre The caller is prepared to yield if this function returns `true` */ -int thread_flags_wake(thread_t *thread); +bool thread_flags_set_internal(thread_t *thread, thread_flags_t mask); #ifdef __cplusplus } diff --git a/core/msg.c b/core/msg.c index 43c7cb6456..11608aefcb 100644 --- a/core/msg.c +++ b/core/msg.c @@ -58,8 +58,7 @@ static int queue_msg(thread_t *target, const msg_t *m) *dest = *m; #if MODULE_CORE_THREAD_FLAGS - target->flags |= THREAD_FLAG_MSG_WAITING; - thread_flags_wake(target); + thread_flags_set_internal(target, THREAD_FLAG_MSG_WAITING); #endif return 1; } @@ -157,8 +156,7 @@ static int _msg_send(msg_t *m, kernel_pid_t target_pid, bool block, thread_add_to_list(&(target->msg_waiters), me); #if MODULE_CORE_THREAD_FLAGS - target->flags |= THREAD_FLAG_MSG_WAITING; - thread_flags_wake(target); + thread_flags_set_internal(target, THREAD_FLAG_MSG_WAITING); #endif irq_restore(state); diff --git a/core/thread_flags.c b/core/thread_flags.c index be3c1bf44f..0d8a92ef67 100644 --- a/core/thread_flags.c +++ b/core/thread_flags.c @@ -53,11 +53,6 @@ static inline int __attribute__((always_inline)) _thread_flags_wake( return wakeup; } -int thread_flags_wake(thread_t *thread) -{ - return _thread_flags_wake(thread); -} - static thread_flags_t _thread_flags_clear_atomic(thread_t *thread, thread_flags_t mask) { @@ -142,6 +137,12 @@ thread_flags_t thread_flags_wait_all(thread_flags_t mask) return _thread_flags_clear_atomic(me, mask); } +bool thread_flags_set_internal(thread_t *thread, thread_flags_t mask) +{ + thread->flags |= mask; + return _thread_flags_wake(thread); +} + void thread_flags_set(thread_t *thread, thread_flags_t mask) { DEBUG("thread_flags_set(): setting 0x%08x for pid %" PRIkernel_pid "\n", From f39645642365b50a7f3e09b20fedb7c23c2dad37 Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Tue, 18 Nov 2025 14:26:47 +0100 Subject: [PATCH 2/2] core/thread_flags_group: use portable implementation The previous implementation relied on `thread_flag_set()` to defer the context switch when called with IRQs disabled until `irq_restore()` is called. This however can only be the case when `thread_yield_higher` triggers an IRQ and performs the context switch within the ISR. This allowed the previous implementation to continue calling `thread_flag_set()` for the remaining group members. This however is an implementation detail that is not part of the API contract. Platforms that do not have a service request IRQ may have to use other means for context switching that do not get deferred until an `irq_restore()` is called. In that case, the first call to `thread_flags_set()` even with IRQs disabled may directly trigger a context switch to the unblocked thread, even if other group members would also be unblocked and have a higher priority. This changes the implementation to manually set the flags and update the thread status without yielding and keep track whether any thread has been awoken. Only once the states of all threads have been updated, the adapted implementation will now call `thread_yield()` (unless no thread was awoken). --- core/thread_flags_group.c | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/core/thread_flags_group.c b/core/thread_flags_group.c index 2bad93350c..97d1e3fde2 100644 --- a/core/thread_flags_group.c +++ b/core/thread_flags_group.c @@ -18,17 +18,26 @@ * @} */ + #include + #include "bitarithm.h" #include "irq.h" #include "thread.h" #include "thread_flags_group.h" +#define ENABLE_DEBUG 0 +#include "debug.h" + void thread_flags_group_set(thread_flags_group_t *group, thread_flags_t mask) { /* Interrupts must be disabled because the threads are not ordered by * priority. */ unsigned irq_state = irq_disable(); + DEBUG("thread_flags_group_set(%p, %x):\n", (void *)group, (unsigned)mask); + DEBUG("| TID | Flags | Status (old) | Status (new) |\n"); + + bool yield = false; for (kernel_pid_t i = 0; i < (kernel_pid_t)ARRAY_SIZE(group->members); i++) { unsigned pid_block = group->members[i]; kernel_pid_t const pid_base = i * UINT_WIDTH; @@ -36,9 +45,31 @@ void thread_flags_group_set(thread_flags_group_t *group, thread_flags_t mask) while (pid_block) { pid_block = bitarithm_test_and_clear(pid_block, &pid_offs); - thread_flags_set(thread_get(pid_base + pid_offs), mask); + kernel_pid_t target_pid = pid_base + pid_offs; + thread_t *target = thread_get(target_pid); + if (!target) { + DEBUG("| %02u | n/a | n/a (dead) | n/a (dead) |\n", + target_pid); + continue; + } + thread_status_t old_status = target->status; + bool awoken = thread_flags_set_internal(target, mask); + thread_status_t new_status = target->status; + /* NOTE: wait_data is shared by thread_flags with other + * mechanisms and may e.g. be a pointer to an `msg_t`. + * Some interpretation of the output is needed by the + * user of the debug output. */ + thread_flags_t wait_data = (uint16_t)(uintptr_t)target->wait_data; + DEBUG("| %02u | %04x | %12s | %12s |\n", + (unsigned)target_pid, (unsigned)wait_data, + thread_state_to_string(old_status), + thread_state_to_string(new_status)); + yield = yield || awoken; } } irq_restore(irq_state); + if (yield) { + thread_yield_higher(); + } }