Skip to content

kernel: grant: do not pass T::default()#2003

Merged
bors[bot] merged 1 commit into
masterfrom
kernel-grant-alloc-no-stack
Jul 10, 2020
Merged

kernel: grant: do not pass T::default()#2003
bors[bot] merged 1 commit into
masterfrom
kernel-grant-alloc-no-stack

Conversation

@bradjc

@bradjc bradjc commented Jul 6, 2020

Copy link
Copy Markdown
Contributor

This changes how grant allocation is structured in grant.rs so that the
allocation function does not take a copy of the data to be written when
creating a new grant. This data can be large if the grant region is
large, and it can cause a stack overflow when the allocate function is
called.

The change is pretty simple, but I'm not sure if there is some subtle reason to not do it this way.

Fixes the IPC stack overflow issue found in #1933. Replaces #1976.

Testing Strategy

Running the hail app on hail.

TODO or Help Wanted

n/a

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make prepush.

This changes how grant allocation is structured in grant.rs so that the
allocation function does not take a copy of the data to be written when
creating a new grant. This data can be large if the grant region is
large, and it can cause a stack overflow when the allocate function is
called.
@bradjc bradjc added the kernel label Jul 6, 2020
Comment thread kernel/src/grant.rs
// We use `ptr::write` to avoid `Drop`ping the uninitialized memory in
// case `T` implements the `Drop` trait.
write(ptr.as_ptr(), data);
write(ptr.as_ptr(), T::default());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm pretty uncertain why this change would actually solve the "grants temporarily take up a ton of stack space" problem. T::default() is still going to return a T, which will be its full size and on the stack. Is passing it into the alloc_unowned() function taking double the size of T on the stack or something?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Per a slack discussion with bradjc, this change greatly increases the possible size of IPC grants before a kernel stack overflow occurs (previously 364 bytes increases to 2364 bytes). I'm guessing some compiler optimizations are making the magic happen here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right, presumably the compiler can better guess to just allocate the space for the default value directly at ptr.as_ptr() as opposed to on the stack first.

@alevy alevy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@bradjc this is dope

@alevy

alevy commented Jul 10, 2020

Copy link
Copy Markdown
Member

bors r+

@bors

bors Bot commented Jul 10, 2020

Copy link
Copy Markdown
Contributor

@bors bors Bot merged commit 303c824 into master Jul 10, 2020
@bors bors Bot deleted the kernel-grant-alloc-no-stack branch July 10, 2020 15:47
sirchnik pushed a commit to sirchnik/tock that referenced this pull request May 12, 2026
2003: kernel: grant: do not pass T::default() r=alevy a=bradjc

This changes how grant allocation is structured in grant.rs so that the
allocation function does not take a copy of the data to be written when
creating a new grant. This data can be large if the grant region is
large, and it can cause a stack overflow when the allocate function is
called.

The change is pretty simple, but I'm not sure if there is some subtle reason to not do it this way.

Fixes the IPC stack overflow issue found in tock#1933. Replaces tock#1976.


### Testing Strategy

Running the hail app on hail.


### TODO or Help Wanted

n/a


### Documentation Updated

- [x] Updated the relevant files in `/docs`, or no updates are required.

### Formatting

- [x] Ran `make prepush`.


Co-authored-by: Brad Campbell <bradjc5@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants