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

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

Merged
merged 1 commit into from Jul 10, 2020
Merged

Conversation

bradjc
Copy link
Contributor

@bradjc bradjc commented Jul 6, 2020

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
@@ -102,7 +102,7 @@ impl Allocator {

// 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
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
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
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.

Copy link
Member

@alevy alevy left a comment

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
Copy link
Member

alevy commented Jul 10, 2020

bors r+

@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
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.

None yet

5 participants