Double stack buffer for i.MX RT-based boards, prune panic! allocations #2400
Conversation
This file contains hidden or 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
Board-based peripheral initialization requires at least core::mem::size_of::<Imxrt10xxDefaultPeripherals>() bytes on the stack to initialize the static peripherals. The majority of the space comes from the GPIO ports and pins. As of this writing, that struct size is around 2K, which was half of the previous stack space. We're running out of stack space, so this commit doubles the stack size. I didn't notice the issue until we updated the Rust compiler to 2021-01-07. Before that update, the kernel would initialize, and apps would run. But, a panic would fail. The panic handler is additionally allocating a ~2K gpio::Ports struct on the stack, which ends up exceeding the stack size. This is still true as of this commit.
After implementing board-based peripheral initialization, we ended up allocating a big GPIO ports structure in the panic handler. This commit changes the panic handlers to allocate a single pin for the panic handler LED.
hudson-ayers
approved these changes
Feb 2, 2021
There was a problem hiding this comment.
This all looks right to me. I still mean to get around to finding a way to initialize peripherals incrementally, so initialization does not require so much stack space (the stack allocation should only be temporary anyway, the actual objects end up stored in lazily initialized globals).
Comment on lines
-51
to
+52
| let ports = imxrt10xx::gpio::Ports::new(&ccm); | ||
| let led = &mut led::LedHigh::new(ports.pin(gpio::PinId::B0_03)); | ||
| let pin = crate::imxrt1060::gpio::Pin::from_pin_id(gpio::PinId::B0_03); | ||
| let led = &mut led::LedHigh::new(&pin); |
There was a problem hiding this comment.
this is an important change that we should definitely extend to other boards
|
We should merge this so the board works, but it's a bummer to double the stack RAM just for bootup. |
|
bors r+ |
|
An RFC for approaches to fix the issue that necessitated this is here: #2425 |
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.
Pull Request Overview
This pull request doubles the stack buffer for boards that use i.MX RT chips. #2311 added board-based peripheral initialization for i.MX RT boards. We use more stack space (at least ~2KiB, most memory for GPIO pins) to initialize the static peripherals, which might exceed the stack buffer once the kernel starts running. Doubling the stack size seems to suffice for running the kernel and two applications on a Teensy 4.0. This approach is similar to #2223, which noted the issue in STM32 boards.
I first bisected the issue down to a commit in #2334. After that compiler update, I couldn't run the Tock kernel on the Teensy 4.0. But it turns out that, after #2311 and before the updated compiler, a
panic!()would overflow the stack. So this seems to be a latent issue that manifested with a compiler update. The boards' panic handlers were allocating large GPIO ports structures just to access a single LED. This PR fixes that by exposing a constructor for individual GPIO pins, and using that constructor in the panic handler.Testing Strategy
This pull request was tested by rebasing onto trunk, compiling the kernel for a Teensy 4.0, and using the kernel to run two
libtock-cexample apps. Then, I added apanic!()just before the main kernel loop, and showed that the panic handler worked as expected.TODO or Help Wanted
There may be other boards that allocate large objects (GPIO ports and pins) in the panic handler. See the stm32f3discovery board and the stm32f412gdiscovery board for two examples. Are we aware of this behaviors in these boards, or in other boards?
Documentation Updated
/docs, or no updates are required.Formatting
make prepush.