8000 kernel: scheduler: Use NonZeroU32 instead of u32 for `start()` and `get_remaining_us()` by Ioan-Cristian · Pull Request #4254 · tock/tock · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

kernel: scheduler: Use NonZeroU32 instead of u32 for start() and get_remaining_us() #4254

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 28, 2024
Merged

kernel: scheduler: Use NonZeroU32 instead of u32 for start() and get_remaining_us() #4254

merged 1 commit into from
Nov 28, 2024

Conversation

Ioan-Cristian
Copy link
Contributor
@Ioan-Cristian Ioan-Cristian commented Nov 26, 2024 •
edited
Loading

Pull Request Overview

This pull request changes the type of the us argument for SchedulerTimer::start() and SchedulerTimer::get_remaining_us() return type. Using NonZeroU32 has two benefits:

  1. Convey the meaning that the scheduler timer cannot be started with 0 micro seconds. All use cases present in Tock expect the scheduler timer to be started with a non-null value and some of them even assume that the value is non-null. See code comments. Using the NonZeroU32 type also forces the programmer to check for null values, thus it provides better type safety.
  2. The memory layout of Option<NonZeroU32> can be optimized, thus reducing the memory footprint of the kernel, albeit by a few bytes:
# Before the change

## Debug
   text	   data	    bss	    dec	    hex	filename
 126952	     32	  13500	 140484	  224c4	../../target/thumbv6m-none-eabi/debug/raspberry_pi_pico.elf

## Release
   text	   data	    bss	    dec	    hex	filename
  98272	     32	  13500	 111804	  1b4bc	../../target/thumbv6m-none-eabi/release/raspberry_pi_pico.elf


# After the change

## Debug
   text	   data	    bss	    dec	    hex	filename
 126940	     32	  13500	 140472	  224b8	../../target/thumbv6m-none-eabi/debug/raspberry_pi_pico.elf

## Release
   text	   data	    bss	    dec	    hex	filename
  98280	     32	  13500	 111812	  1b4c4	../../target/thumbv6m-none-eabi/release/raspberry_pi_pico.elf

As a side effect, this PR requires the change of the SchedulingDecision::RunProcess variant since the scheduler may never decide to run a process for 0 micro seconds.

Testing Strategy

This pull request was tested by running c_hello and blink applications on Raspberry Pi Pico using the round robin scheduler.

TODO or Help Wanted

No help needed.

Documentation Updated

  • No updates are required.

Formatting

  • Ran make prepush.

Ioan-Cristian
Ioan-Cristian commented Nov 26, 2024
View reviewed changes
kernel/src/scheduler/round_robin.rs

SchedulingDecision::RunProcess((next, Some(timeslice)))
Copy link
Contributor Author
@Ioan-Cristian Ioan-Cristian Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this assert required in production code? It looks odd. I would rather add an abstraction types such as NonZeroU32 to show that timeslice may never be 0.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this code is really not what we would like. If RoundRobinSched::time_remaining was a NonZeroU32 we could do proper checks and avoid the chance of a panic.

@Ioan-Cristian
Copy link
Contributor Author

This is a draft until I get access to a Raspberry Pi Pico board and ensure nothing is broken.

@Ioan-Cristian
Copy link
Contributor Author

I notice now that the footprint in release mode is increased by 8 bytes. Strange though. The sole explanation I have right now is NonZeroU32::new() in systick.rs, clint.rs and scheduler_timer.rs.

@Ioan-Cristian
Copy link
Contributor Author
Ioan-Cristian commented Nov 26, 2024 •
edited
Loading

I checked the PR against blink and c_hello applications loaded on Raspberry Pi Pico and using the round robin scheduler.

@Ioan-Cristian Ioan-Cristian marked this pull request as ready for review November 26, 2024 17:43
@bradjc bradjc changed the title Use NonZeroU32 instead of u32 for start() and get_remaining_us() kernel: scheduler: Use NonZeroU32 instead of u32 for start() and get_remaining_us() Nov 26, 2024
bradjc
bradjc approved these changes Nov 26, 2024
View reviewed changes
kernel/src/scheduler/round_robin.rs

SchedulingDecision::RunProcess((next, Some(timeslice)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this code is really not what we would like. If RoundRobinSched::time_remaining was a NonZeroU32 we could do proper checks and avoid the chance of a panic.

@alevy alevy added the last-call Final review period for a pull request. label Nov 27, 2024
@lschuermann lschuermann added this pull request to the merge queue Nov 28, 2024
Merged via the queue into tock:master with commit 4835dbd Nov 28, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alevy alevy alevy approved these changes

@bradjc bradjc bradjc approved these changes

@lschuermann lschuermann lschuermann approved these changes

Assignees

@hudson-ayers hudson-ayers

Labels
kernel last-call Final review period for a pull request.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Ioan-Cristian @alevy @bradjc @lschuermann @hudson-ayers
0