Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(threads/cortex_m): don't save high regs on first schedule #369

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

elenaf9
Copy link
Collaborator

@elenaf9 elenaf9 commented Jul 26, 2024

Description

I noticed that threading on the nrf5340 is currently broken/ hard faults.

Some debugging showed that the problem is that on run, the PendSV tries to stored the r4-r11 registers in a null pointer.
This didn't cause any problems on the rpi-pico or nrf54840, but apparently for the nrf5340/ CortexM33 it's problematic.

To fix this, we have to 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.

An alternative would be to directly in the sched code when current_pid.is_none() return 0 so that no context save/ reload happens, but then the psp update would also be skipped and we'd manually have to do that before returning the 0.

The PR also includes some small optimizations/ clean-ups in sched.

Issues/PRs references

The bug was a side-effekt of #262.

Open Questions

Change checklist

  • I have cleaned up my commit history and squashed fixup commits.
  • I have followed the Coding Conventions.
  • I have performed a self-review of my own code.
  • I have made corresponding changes to the documentation.

@elenaf9 elenaf9 changed the title fix(threads/armv8): don't save high regs on first schedule fix(threads/cortex_m): don't save high regs on first schedule Jul 26, 2024
@kaspar030
Copy link
Collaborator

This looks lije a special case for the first schedule, which hits only once. How does the cycle count look for this? If it is measurably slower, I'd rather have some more logic in start_threading().

@kaspar030
Copy link
Collaborator

But nice catch! :)

@elenaf9
Copy link
Collaborator Author

elenaf9 commented Jul 26, 2024

This looks lije a special case for the first schedule, which hits only once. How does the cycle count look for this? If it is measurably slower, I'd rather have some more logic in start_threading().

Not sure if it's possible to solve it in start_threading. It could be solved it in the else branch inside sched when we check if current_pid is None: e61ea5d. But this impacts compiler optimizations.

Depending on which board (so far only rpi-pico and nrf53840dk; don't have an nrf52840dk with me right now) I test, one or the other solution performs better. But both variations overall don't negatively impact performance, because the PR includes some other small optimizations within sched. On rpi-pico bench_sched_yield 293 -> 291 ticks per context switch.

@kaspar030
Copy link
Collaborator

LGTM, please squash!

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.
@kaspar030 kaspar030 added this pull request to the merge queue Sep 13, 2024
Merged via the queue into future-proof-iot:main with commit 0b09e96 Sep 13, 2024
26 checks passed
@elenaf9 elenaf9 deleted the threads/fix-first-sched branch September 13, 2024 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants