Message-ID: <Zryhae9OXD_dqfR8@pollux>
Date: Wed, 14 Aug 2024 14:22:01 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: ojeda@...nel.org, alex.gaynor@...il.com, wedsonaf@...il.com,
	boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com,
	benno.lossin@...ton.me, a.hindborg@...sung.com,
	akpm@...ux-foundation.org, daniel.almeida@...labora.com,
	faith.ekstrand@...labora.com, boris.brezillon@...labora.com,
	lina@...hilina.net, mcanal@...lia.com, zhiw@...dia.com,
	cjia@...dia.com, jhubbard@...dia.com, airlied@...hat.com,
	ajanulgu@...hat.com, lyude@...hat.com, linux-kernel@...r.kernel.org,
	rust-for-linux@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v5 09/26] rust: alloc: implement kernel `Box`

On Wed, Aug 14, 2024 at 10:26:10AM +0200, Alice Ryhl wrote:
> On Mon, Aug 12, 2024 at 8:25 PM Danilo Krummrich <dakr@...nel.org> wrote:
> >
> > `Box` provides the simplest way to allocate memory for a generic type
> > with one of the kernel's allocators, e.g. `Kmalloc`, `Vmalloc` or
> > `KVmalloc`.
> >
> > In contrast to Rust's `Box` type, the kernel `Box` type considers the
> > kernel's GFP flags for all appropriate functions, always reports
> > allocation failures through `Result<_, AllocError>` and remains
> > independent from unstable features.
> >
> > Signed-off-by: Danilo Krummrich <dakr@...nel.org>
> > ---
> >  rust/kernel/alloc.rs      |   6 +
> >  rust/kernel/alloc/kbox.rs | 423 ++++++++++++++++++++++++++++++++++++++
> >  rust/kernel/prelude.rs    |   2 +-
> >  3 files changed, 430 insertions(+), 1 deletion(-)
> >  create mode 100644 rust/kernel/alloc/kbox.rs
> >
> > diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> > index 295107777a12..ed46b69204d0 100644
> > --- a/rust/kernel/alloc.rs
> > +++ b/rust/kernel/alloc.rs
> > @@ -5,6 +5,7 @@
> >  #[cfg(not(any(test, testlib)))]
> >  pub mod allocator;
> >  pub mod box_ext;
> > +pub mod kbox;
> >  pub mod vec_ext;
> >
> >  #[cfg(any(test, testlib))]
> > @@ -13,6 +14,11 @@
> >  #[cfg(any(test, testlib))]
> >  pub use self::allocator_test as allocator;
> >
> > +pub use self::kbox::Box;
> > +pub use self::kbox::KBox;
> > +pub use self::kbox::KVBox;
> > +pub use self::kbox::VBox;
> > +
> >  /// Indicates an allocation error.
> >  #[derive(Copy, Clone, PartialEq, Eq, Debug)]
> >  pub struct AllocError;
> > diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
> > new file mode 100644
> > index 000000000000..67bdfc0712d2
> > --- /dev/null
> > +++ b/rust/kernel/alloc/kbox.rs
> > @@ -0,0 +1,423 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Implementation of [`Box`].
> > +
> > +use super::{AllocError, Allocator, Flags};
> > +use core::fmt;
> > +use core::marker::PhantomData;
> > +use core::mem::ManuallyDrop;
> > +use core::mem::MaybeUninit;
> > +use core::ops::{Deref, DerefMut};
> > +use core::pin::Pin;
> > +use core::ptr::NonNull;
> > +use core::result::Result;
> > +
> > +use crate::init::{InPlaceInit, Init, PinInit};
> > +use crate::types::ForeignOwnable;
> > +
> > +/// The kernel's [`Box`] type - a heap allocation for a single value of type `T`.
> > +///
> > +/// This is the kernel's version of the Rust stdlib's `Box`. There are a couple of differences,
> > +/// for example no `noalias` attribute is emitted and partially moving out of a `Box` is not
> > +/// supported.
> > +///
> > +/// `Box` works with any of the kernel's allocators, e.g. [`super::allocator::Kmalloc`],
> > +/// [`super::allocator::Vmalloc`] or [`super::allocator::KVmalloc`]. There are aliases for `Box`
> > +/// with these allocators ([`KBox`], [`VBox`], [`KVBox`]).
> > +///
> > +/// When dropping a [`Box`], the value is also dropped and the heap memory is automatically freed.
> > +///
> > +/// # Examples
> > +///
> > +/// ```
> > +/// let b = KBox::<u64>::new(24_u64, GFP_KERNEL)?;
> > +///
> > +/// assert_eq!(*b, 24_u64);
> > +///
> > +/// # Ok::<(), Error>(())
> 
> This is a minor nit, but when hiding lines in examples you should
> avoid having the rendered docs have empty lines at the beginning/end.
> There are also several examples of this below with the
> kernel::bindings import.

Makes sense, gonna fix it.

> 
> > +/// ```
> > +///
> > +/// ```
> > +/// # use kernel::bindings;
> > +///
> > +/// const SIZE: usize = bindings::KMALLOC_MAX_SIZE as usize + 1;
> > +/// struct Huge([u8; SIZE]);
> > +///
> > +/// assert!(KBox::<Huge>::new_uninit(GFP_KERNEL | __GFP_NOWARN).is_err());
> > +/// ```
> > +///
> > +/// ```
> > +/// # use kernel::bindings;
> > +///
> > +/// const SIZE: usize = bindings::KMALLOC_MAX_SIZE as usize + 1;
> > +/// struct Huge([u8; SIZE]);
> > +///
> > +/// assert!(KVBox::<Huge>::new_uninit(GFP_KERNEL).is_ok());
> > +/// ```
> > +///
> > +/// # Invariants
> > +///
> > +/// The [`Box`]' pointer always properly aligned and either points to memory allocated with `A` or,
> > +/// for zero-sized types, is a dangling pointer.
> > +pub struct Box<T: ?Sized, A: Allocator>(NonNull<T>, PhantomData<A>);
> 
> I was about to say this needs a PhantomData<T> too, but I guess it
> isn't necessary anymore.
> https://doc.rust-lang.org/nomicon/phantom-data.html#generic-parameters-and-drop-checking
> 
> > +// SAFETY: `Box` is `Send` if `T` is `Send` because the data referenced by `self.0` is unaliased.
> 
> Instead of "unaliased" I would probably just say "because the Box owns a T".

I'm fine with either one, if that's preferred, I'll change it.

> 
> > +
> > +// SAFETY: `Box` is `Sync` if `T` is `Sync` because the data referenced by `self.0` is unaliased.
> > +unsafe impl<T, A> Sync for Box<T, A>
> > +where
> > +    T: Send + ?Sized,
> > +    A: Allocator,
> 
> This needs to say `T: Sync` instead of `T: Send`. That matches the std Box.

Good catch, pretty sure `Vec` has the same copy-paste mistake.

> 
> > +
> > +impl<T, A> Box<T, A>
> > +where
> > +    T: ?Sized,
> > +    A: Allocator,
> > +{
> > +    /// Creates a new `Box<T, A>` from a raw pointer.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// `raw` must point to valid memory, previously be allocated with `A`, and provide at least
> > +    /// the size of type `T`. For ZSTs `raw` must be a dangling pointer.
> 
> Hmm. I don't love this wording. How about this?
> 
> For non-ZSTs, `raw` must point at a live allocation allocated with `A`
> that is sufficiently aligned for and holds a valid `T`. The caller
> passes ownership of the allocation to the `Box`.

I'm fine taking that, I'm not so sure about "live allocation" though, we don't
use this wording anywhere else. It's probably fine to just say "allocation",
since once it's freed it technically isn't an allocation anymore anyways.

> For ZSTs, the pointer must be non-null and aligned.

Technically, for ZSTs any pointer is aligned and NULL should be valid as well.
Maybe we just don't mentioned ZSTs at all, since we don't really need to enforce
anything for ZSTs.

> 
> > +impl<T, A> From<Box<T, A>> for Pin<Box<T, A>>
> > +where
> > +    T: ?Sized,
> > +    A: Allocator,
> > +{
> > +    /// Converts a `Box<T, A>` into a `Pin<Box<T, A>>`. If `T` does not implement [`Unpin`], then
> > +    /// `*b` will be pinned in memory and can't be moved.
> > +    ///
> > +    /// See [`Box::into_pin`] for more details.
> > +    fn from(b: Box<T, A>) -> Self {
> > +        Box::into_pin(b)
> 
> I still think it makes more sense to match std and only provide From
> and not an into_pin, but it's not a blocker.

Yeah, I just kept it since I'm not (yet) entirely sure what to think of the
`From` and `Into` stuff in some cases.

I don't really like that, depending on the context, it may hide relevant
details.

In the kernel, no matter how well documented an API is, I think it's rather
common to look at the code for some implementation details before using it.

Sometimes it might not be super trivial for the "occasional" reader to figure
out what's the type of some variable. Calling `into_pin` vs. just `into`
immediately tells the reader that things need to be pinned from there on.

However, I had no specific example in my mind and I'm also not overly concerned
to remove `into_pin`, but I want to at least share the reason why I kept it in
the first place.

> 
> Alice
> 

Powered by blists - more mailing lists