core/thread_flags_group: use portable implementation #21895

maribu · 2025-11-18T13:45:41Z

Contribution description

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).

Testing procedure

In `master`

$ make BOARD=arduino-mega2560 -C tests/core/thread_flags_group flash test
make: Entering directory '/home/[email protected]/Repos/software/RIOT/master/tests/core/thread_flags_group'
Building application "tests_thread_flags_group" for "arduino-mega2560" with CPU "atmega2560".

[...]

Help: Press s to start test, r to print it is ready
READY
s
START
main(): This is RIOT! (Version: 2026.01-devel-144-gbcd09d)
START
waiting forever-waiter
waiting waiter
waiting waiter
waking up!
woken up waiter
woken up waiter
/home/[email protected]/Repos/software/RIOT/master/tests/core/thread_flags_group/main.c:73 => failed condition
*** RIOT kernel panic:
CONDITION FAILED.

*** halted.

In this PR

$ make BOARD=arduino-mega2560 -C tests/core/thread_flags_group flash test
make: Entering directory '/home/[email protected]/Repos/software/RIOT/master/tests/core/thread_flags_group'
Building application "tests_thread_flags_group" for "arduino-mega2560" with CPU "atmega2560".

[...]

Help: Press s to start test, r to print it is ready
READY
s
START
main(): This is RIOT! (Version: 2026.01-devel-137-g8c63a-core/thread_flags_group/fix)
START
waiting forever-waiter
waiting waiter
waiting waiter
waking up!
woken up waiter
woken up waiter
SUCCESS

make: Leaving directory '/home/[email protected]/Repos/software/RIOT/master/tests/core/thread_flags_group'

Issues/PRs references

None

maribu · 2025-11-18T13:46:32Z

@derMihai: Would you mind to also take a look?

riot-ci · 2025-11-18T14:35:19Z

Murdock results

✔️ PASSED

55f7494 fixup! core/thread_flags: change internal API

Success	Failures	Total	Runtime
10932	0	10932	19m:28s

Artifacts

Documentation preview

crasbe · 2025-11-18T14:37:20Z

Is this a 2 ACK kind of PR?

mguetschow · 2025-11-18T15:40:31Z

Is this a 2 ACK kind of PR?

I'd say so, as it touches core. I though CI would label accordingly 🤔

derMihai · 2025-11-18T16:00:07Z

core/thread_flags_group.c

    }

    irq_restore(irq_state);
+    if (yield) {


can't this be called unconditionally?

Yes, it will be faster in case all threads of the group either where already pending or not currently actively waiting for one of the flags in mask.

derMihai · 2025-11-18T16:00:41Z

Huh, I thought this was sorted out: #21123 (comment)

But if not, then you're totally right and this PR looks fine.

maribu · 2025-11-18T17:34:25Z

I would assume that @kaspar030 was referring to the crash with "sorted out". And indeed, that crash did not happen anymore.

Just sending out PendSV (or similar) in thread_yield_higher() is nothing you can do on platforms other than Cortex M. At least AVR and MSP430 do not have any service IRQs. I don't know how the situation is on RISC-V and Xtensa.

We certainly cannot assume that each and every architecture has a software IRQ usable for context switching without limiting portability.

core/thread_flags_group.c

mguetschow

Thanks for finding and fixing!

There is quite a lot of code that is just needed for debugging, but which makes the function look more complex than it is. I don't have a good suggestion how, but maybe you have an idea how to leave it more concise?

core/thread_flags_group.c

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.

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).

mguetschow · 2025-11-20T13:27:28Z

core/thread_flags.c

+bool thread_flags_set_internal(thread_t *thread, thread_flags_t mask)
+{
+    thread->flags |= mask;
+    return _thread_flags_wake(thread);
+}


Now that's a nice solution, thanks! Maybe remove thread_flags_wake though - it's not exposed through the header file anymore anyways.

RIOT/core/thread_flags.c

Lines 56 to 59 in a23948c

int thread_flags_wake(thread_t *thread)

{

return _thread_flags_wake(thread);

}

That function has multiple callers...

_thread_flags_wake still has, but thread_flags_wake does not, does it? At least its not exposed through the headerfile anymore and all calls inside thread_flags.c should use the static (_-prefixed) function anyways?

So I'd say thread_flags_wake should be dead code as it stands now.

Ah, OK. I could have sworn I dropped it. Maybe reordering the commits didn't work as expected

maribu requested a review from kaspar030 as a code owner November 18, 2025 13:45

github-actions bot added the Area: core Area: RIOT kernel. Handle PRs marked with this with care! label Nov 18, 2025

maribu requested review from benpicco, crasbe and mguetschow November 18, 2025 13:46

maribu force-pushed the core/thread_flags_group/fix branch from 8c63a9b to 150db5b Compare November 18, 2025 13:48

derMihai reviewed Nov 18, 2025

View reviewed changes

maribu added the Process: needs >1 ACK Integration Process: This PR requires more than one ACK label Nov 18, 2025

github-actions bot added the Process: missing approvals Integration Process: PR needs more ACKS (handled by action) label Nov 18, 2025

maribu mentioned this pull request Nov 18, 2025

[Tracking] Failures found with testing on real hardware for release #21882

Open

37 tasks

maribu force-pushed the core/thread_flags_group/fix branch from d705c34 to aa010b8 Compare November 19, 2025 08:28

AnnsAnns reviewed Nov 19, 2025

View reviewed changes

core/thread_flags_group.c Show resolved Hide resolved

mguetschow reviewed Nov 20, 2025

View reviewed changes

core/thread_flags_group.c Show resolved Hide resolved

core/thread_flags_group.c Outdated Show resolved Hide resolved

maribu added 2 commits November 20, 2025 11:19

core/thread_flags: change internal API

39bf317

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.

maribu force-pushed the core/thread_flags_group/fix branch from e6c7979 to d873852 Compare November 20, 2025 10:19

mguetschow reviewed Nov 20, 2025

View reviewed changes

fixup! core/thread_flags: change internal API

55f7494

	int thread_flags_wake(thread_t *thread)
	{
	return _thread_flags_wake(thread);
	}

core/thread_flags_group: use portable implementation #21895

Are you sure you want to change the base?

core/thread_flags_group: use portable implementation #21895

Conversation

maribu commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

In master

In this PR

Issues/PRs references

Uh oh!

maribu commented Nov 18, 2025

Uh oh!

riot-ci commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Murdock results

Artifacts

Uh oh!

crasbe commented Nov 18, 2025

Uh oh!

mguetschow commented Nov 18, 2025

Uh oh!

derMihai Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

maribu Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

derMihai commented Nov 18, 2025

Uh oh!

maribu commented Nov 18, 2025

Uh oh!

Uh oh!

mguetschow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mguetschow Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

maribu Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

mguetschow Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

maribu Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

maribu commented Nov 18, 2025 •

edited

Loading

In `master`

riot-ci commented Nov 18, 2025 •

edited

Loading