riot-rs-threads: always set threads.current_thread in sched()
#262
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
elenaf9
approved these changes
Apr 16, 2024
There was a problem hiding this comment.
Seems like the compiler previously optimized the else path away, which is now not possible anymore. I played around a bit, and when doing (on main) current_high_regs = core::hint::black_box(core::ptr::null()) in the else path it also takes 259 cycles.
Either way, as you also said in our out-of-band discussion @kaspar030, with 2% I think this is acceptable.
With the added line in sched() after the loop (instead of putting it in both cases), it'd go up to 261 cycles.
I kinda dislike that we now set it in both branches, but agree that performance should take priority over readability.
|
Thanks for taking a look! |
elenaf9
added a commit
to elenaf9/RIOT-rs
that referenced
this pull request
Jul 26, 2024
With future-proof-iot#262, the `current_pid` is not set before the scheduler is triggered for the first time, and therefore `None` on the first run. The pointer to `current_high_regs` is therefore set to a null pointer. This causes nrf52840 to hard fault when trying to store registers r4-r11. To fix this, we now skip storing/ loading of r4-r11 if the pointer is zero, i.e. when there was no previously running thread whose context has to be saved.
4 tasks
elenaf9
added a commit
to elenaf9/RIOT-rs
that referenced
this pull request
Sep 13, 2024
With future-proof-iot#262, the `current_pid` is not set before the scheduler is triggered for the first time, and therefore `None` on the first run. The pointer to `current_high_regs` is therefore set to a null pointer. This causes nrf52840 to hard fault when trying to store registers r4-r11. To fix this, we now skip storing/ loading of r4-r11 if the pointer is zero, i.e. when there was no previously running thread whose context has to be saved.
elenaf9
added a commit
to elenaf9/RIOT-rs
that referenced
this pull request
Sep 27, 2024
With future-proof-iot#262, the `current_pid` is not set before the scheduler is triggered for the first time, and therefore `None` on the first run. The pointer to `current_high_regs` is therefore set to a null pointer. This causes nrf52840 to hard fault when trying to store registers r4-r11. To fix this, we now skip storing/ loading of r4-r11 if the pointer is zero, i.e. when there was no previously running thread whose context has to be saved.
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.
This is taken from #182:
This changes makes the number of cycles needed for one context switch go up from 254 to 259 (according to tests/benchmarks/bench_sched_yield on nrf52840dk). With the added line in
sched()after the loop (instead of putting it in both cases), it'd go up to 261 cycles.Fixes #148.