core/thread_flags_group: use portable implementation by maribu · Pull Request #21895 · RIOT-OS/RIOT

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

f396456 core/thread_flags_group: use portable implementation

Success	Failures	Total	Runtime
10931	0	10932	12m:04s

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

core/thread_flags.c

benpicco

looks good to me

mguetschow

Please squash :)

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

maribu · 2025-11-21T22:47:29Z

Thx a lot ❤️

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

core/thread_flags.c Show resolved Hide resolved

benpicco approved these changes Nov 21, 2025

View reviewed changes

mguetschow approved these changes Nov 21, 2025

View reviewed changes

maribu added 2 commits November 21, 2025 15:21

core/thread_flags: change internal API

50a1d28

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 55f7494 to f396456 Compare November 21, 2025 14:21

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

maribu enabled auto-merge November 21, 2025 14:21

maribu added this pull request to the merge queue Nov 21, 2025

Merged via the queue into RIOT-OS:master with commit 2c815f8 Nov 21, 2025
25 checks passed

maribu deleted the core/thread_flags_group/fix branch November 21, 2025 22:47

leandrolanzieri added this to the Release 2026.01 milestone Jan 13, 2026

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!

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

mguetschow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maribu commented Nov 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

maribu commented Nov 18, 2025 •

edited

Loading

In `master`

riot-ci commented Nov 18, 2025 •

edited

Loading