cpu/native: fix thread_yield_higher() with IRQs disabled #16754
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kaspar030
added
Platform: native
kaspar030
requested review from
aabadie,
fjmolinas,
leandrolanzieri,
MichelRottleuthner,
miri64 and
smlng
as code owners
github-actions
bot
added
Area: cpu
benpicco
added
CI: run tests
benpicco
reviewed
Aug 18, 2021
tests/deferred_yield_higher/Makefile
Outdated
| @@ -1,3 +1,5 @@ | |||
| include ../Makefile.tests_common | |||
|
|
|||
| BOARD_WHITELIST := native | |||
There was a problem hiding this comment.
Why not
Suggested change
| BOARD_WHITELIST := native | |
| FEATURES_BLACKLIST += arch_avr8 arch_msp430 |
There was a problem hiding this comment.
hm, I'm not sure about the others. but yes, that's nicer
There was a problem hiding this comment.
done, I've also added esp, mips and riscv, until they're confirmed to work.
There was a problem hiding this comment.
don't we have esp on CI?
I have esp on my desk, I can do the test just now.
There was a problem hiding this comment.
esp8266
/home/benpicco/dev/RIOT/dist/tools/pyterm/pyterm -p "/dev/ttyUSB3" -b "115200" --no-reconnect --noprefix --no-repeat-command-on-empty-line
Twisted not available, please install it if you want to use pyterm's JSON capabilities
Connect to serial port /dev/ttyUSB3
Welcome to pyterm!
Type '/exit' to exit.
READY
s
START
main(): This is RIOT! (Version: 2021.10-devel-380-ge4b3a)
1. post yield
2. second thread scheduled
3. post irq enable
works
esp32
/home/benpicco/dev/RIOT/dist/tools/pyterm/pyterm -p "/dev/ttyUSB3" -b "115200" --no-reconnect --noprefix --no-repeat-command-on-empty-line
Twisted not available, please install it if you want to use pyterm's JSON capabilities
Connect to serial port /dev/ttyUSB3
Welcome to pyterm!
Type '/exit' to exit.
READY
s
START
main(): This is RIOT! (Version: 2021.10-devel-380-ge4b3a)
1. post yield
2. second thread scheduled
3. post irq enable
works
There was a problem hiding this comment.
Doesn't esp need to be whiteliseted now @kaspar030 ?
There was a problem hiding this comment.
thanks @benpicco! I've removed the esp blacklist.
fjmolinas
reviewed
Aug 18, 2021
|
200 tests passed frdm-k22fnucleo-f091rcnucleo-f303renucleo-f767zinucleo-l152rearduino-mega2560nucleo-l432kcsaml10-xprosaml11-xprosamr21-xpronrf52dkremote-revbnucleo-f207zgnucleo-l073rzslstk3401anucleo-g474renucleo-f411renucleo-f103rbfrdm-kw41zesp32-wroom-32z1arduino-dueslstk3400afrdm-k64f |
|
@riot-hil-bot is just trying things out. It wanted some real world test case. More info at the VMA. |
If you are looking for some input, if the posted results could link to the test logs (when failed) that would be awesome! |
|
@MrKevinWeiss only failure I find weird is |
fjmolinas
added
CI: ready for build
|
Please squash! |
This makes native behave like Cortex-M, which flags PENDSV, which then gets triggered once IRQs are re-enabled.
This test verifies that a thread can call `thread_yield_higher()` with IRQs disabled, and that the actual context switch is deferred until IRQs are re-enabled.
kaspar030
force-pushed
the
native_deferred_yield_higher
branch
from
0dfe049 to
fe1d998
Compare
kaspar030
removed
CI: ready for build
kaspar030
added
the
CI: ready for build
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
This PR allows
thread_yield_higher()to be called with IRQs disabled, on native. In that case, a deferred yield is executed once interrupts are re-enabled.That's the behavior on Cortex-M. IMO it would be nice to be able to depend on it.
A test is added for verifying this behavior.
The test verifies this behavior on all platforms though, and should thus be run on them.Should some platform not behave right, I suggest limiting the test to the working platforms, until this is sorted out.
At least msp430 and avr8 don't behave as required for this test, so I've marked it native only for now.
Testing procedure
As this was warned on and forbidden before, nothing should break. CI tests should be sufficient.
Issues/PRs references
The fix in #16751 could profit from this.