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.

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

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

Merged
merged 1 commit into from
Jul 10, 2020
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions kernel/src/grant.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,16 +81,16 @@ impl<T: ?Sized> DerefMut for Owned<T> {
}

impl Allocator {
pub fn alloc<T>(&mut self, data: T) -> Result<Owned<T>, Error> {
pub fn alloc<T: Default>(&mut self) -> Result<Owned<T>, Error> {
unsafe {
let ptr = self.alloc_unowned(data)?;
let ptr = self.alloc_unowned()?;
Ok(Owned::new(ptr, self.appid))
}
}

// Like `alloc`, but the caller is responsible for free-ing the allocated
// memory, as it is not wrapped in a type that implements `Drop`
unsafe fn alloc_unowned<T>(&mut self, data: T) -> Result<NonNull<T>, Error> {
unsafe fn alloc_unowned<T: Default>(&mut self) -> Result<NonNull<T>, Error> {
self.appid
.kernel
.process_map_or(Err(Error::NoSuchApp), self.appid, |process| {
Expand All @@ -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.


Ok(ptr)
},
Expand Down Expand Up @@ -223,7 +223,7 @@ impl<T: Default> Grant<T> {
// Note: This allocation is intentionally never
// freed. A grant region is valid once allocated
// for the lifetime of the process.
let new_region = allocator.alloc_unowned(T::default())?;
let new_region = allocator.alloc_unowned()?;

// Update the grant pointer in the process. Again,
// since the process struct does not know about the
Expand Down