Message-ID: <a69e7280-7291-49f7-a46f-1ad465efce04@proton.me>
Date: Wed, 14 Aug 2024 17:01:34 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Danilo Krummrich <dakr@...nel.org>, ojeda@...nel.org, alex.gaynor@...il.com, wedsonaf@...il.com, boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com, a.hindborg@...sung.com, aliceryhl@...gle.com, akpm@...ux-foundation.org
Cc: 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 12.08.24 20:22, Danilo Krummrich wrote:
> +/// 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.

I would add "But otherwise it works the same." (I don't know if there is
a comma needed after the "otherwise").
Also I remember that there was one more difference with a custom box
compared to the stdlib, but I forgot what that was, does someone else
remember? We should also put that here.

> +///
> +/// `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>(())
> +/// ```
> +///
> +/// ```
> +/// # 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,

"pointer always properly" -> "pointer is properly"

> +/// for zero-sized types, is a dangling pointer.

I think this section would look nicer, if it were formatted using bullet
points (that way the bracketing of the "or" is also unambiguous).

Additionally, this is missing that the pointer is valid for reads and
writes.

> +pub struct Box<T: ?Sized, A: Allocator>(NonNull<T>, PhantomData<A>);

Why no `repr(transparent)`?

> +
> +/// Type alias for `Box` with a `Kmalloc` allocator.

I think we should add that this is only designed for small values.

> +///
> +/// # Examples
> +///
> +/// ```
> +/// let b = KBox::new(24_u64, GFP_KERNEL)?;
> +///
> +/// assert_eq!(*b, 24_u64);
> +///
> +/// # Ok::<(), Error>(())
> +/// ```
> +pub type KBox<T> = Box<T, super::allocator::Kmalloc>;
> +
> +/// Type alias for `Box` with a `Vmalloc` allocator.

Same here, add that this is supposed to be used for big values (or is
this also a general-purpose allocator, just not guaranteeing that the
memory is physically contiguous? in that case I would document it
here and also on `Vmalloc`).

> +///
> +/// # Examples
> +///
> +/// ```
> +/// let b = VBox::new(24_u64, GFP_KERNEL)?;
> +///
> +/// assert_eq!(*b, 24_u64);
> +///
> +/// # Ok::<(), Error>(())
> +/// ```
> +pub type VBox<T> = Box<T, super::allocator::Vmalloc>;
> +
> +/// Type alias for `Box` with a `KVmalloc` allocator.

Ditto.

> +///
> +/// # Examples
> +///
> +/// ```
> +/// let b = KVBox::new(24_u64, GFP_KERNEL)?;
> +///
> +/// assert_eq!(*b, 24_u64);
> +///
> +/// # Ok::<(), Error>(())
> +/// ```
> +pub type KVBox<T> = Box<T, super::allocator::KVmalloc>;
> +
> +// SAFETY: `Box` is `Send` if `T` is `Send` because the data referenced by `self.0` is unaliased.
> +unsafe impl<T, A> Send for Box<T, A>
> +where
> +    T: Send + ?Sized,
> +    A: Allocator,
> +{
> +}
> +
> +// 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,
> +{
> +}
> +
> +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.
> +    #[inline]
> +    pub const unsafe fn from_raw(raw: *mut T) -> Self {
> +        // INVARIANT: Validity of `raw` is guaranteed by the safety preconditions of this function.
> +        // SAFETY: By the safety preconditions of this function, `raw` is not a NULL pointer.
> +        Self(unsafe { NonNull::new_unchecked(raw) }, PhantomData::<A>)
> +    }
> +
> +    /// Consumes the `Box<T, A>` and returns a raw pointer.
> +    ///
> +    /// This will not run the destructor of `T` and for non-ZSTs the allocation will stay alive
> +    /// indefinitely. Use [`Box::from_raw`] to recover the [`Box`], drop the value and free the
> +    /// allocation, if any.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// let x = KBox::new(24, GFP_KERNEL)?;
> +    /// let ptr = KBox::into_raw(x);
> +    /// let x = unsafe { KBox::from_raw(ptr) };
> +    ///
> +    /// assert_eq!(*x, 24);
> +    ///
> +    /// # Ok::<(), Error>(())
> +    /// ```
> +    #[inline]
> +    pub fn into_raw(b: Self) -> *mut T {
> +        let b = ManuallyDrop::new(b);
> +
> +        b.0.as_ptr()
> +    }
> +
> +    /// Consumes and leaks the `Box<T, A>` and returns a mutable reference.
> +    ///
> +    /// See [Box::into_raw] for more details.
> +    #[inline]
> +    pub fn leak<'a>(b: Self) -> &'a mut T
> +    where
> +        T: 'a,

This bound shouldn't be needed, it is implicit, since `T` appears in the
type `&'a mut T`.

> +    {
> +        // SAFETY: `Box::into_raw` always returns a properly aligned and dereferenceable pointer
> +        // which points to an initialized instance of `T`.
> +        unsafe { &mut *Box::into_raw(b) }
> +    }
> +
> +    /// 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.
> +    ///
> +    /// This moves `b` into `Pin` without moving `*b` or allocating and copying any memory.
> +    #[inline]
> +    pub fn into_pin(b: Self) -> Pin<Self> {

I would agree with Alice, that this is not really needed, since you can
just as well write `Pin::from(my_box)`.

> +        // SAFETY: The value wrapped inside a `Pin<Box<T, A>>` cannot be moved or replaced as long
> +        // as `T` does not implement `Unpin`.
> +        unsafe { Pin::new_unchecked(b) }
> +    }
> +}
> +
> +impl<T, A> Box<MaybeUninit<T>, A>
> +where
> +    A: Allocator,
> +{
> +    /// Converts a `Box<MaybeUninit<T>, A>` to a `Box<T, A>`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that the value inside of `b` is in an initialized state. It is
> +    /// undefined behavior to call this function while the value inside of `b` is not yet fully
> +    /// initialized.

The second sentence is unnecessary safety documentation. It might still
be useful as normal documentation though.

> +    pub unsafe fn assume_init(b: Self) -> Box<T, A> {
> +        let raw = Self::into_raw(b);
> +
> +        // SAFETY: `raw` comes from a previous call to `Box::into_raw`. By the safety requirements
> +        // of this function, the value inside the `Box` is in an initialized state. Hence, it is
> +        // safe to reconstruct the `Box` as `Box<T, A>`.
> +        unsafe { Box::from_raw(raw as *mut T) }
> +    }
> +
> +    /// Writes the value and converts to `Box<T, A>`.
> +    pub fn write(mut b: Self, value: T) -> Box<T, A> {
> +        (*b).write(value);
> +        // SAFETY: We've just initialized `boxed`'s value.
> +        unsafe { Self::assume_init(b) }
> +    }
> +}
> +
> +impl<T, A> Box<T, A>
> +where
> +    A: Allocator,
> +{
> +    fn is_zst() -> bool {
> +        core::mem::size_of::<T>() == 0
> +    }
> +
> +    /// Creates a new `Box<T, A>` and initializes its contents with `x`.
> +    ///
> +    /// New memory is allocated with `a`. The allocation may fail, in which case an error is

Wrong case on "`a`" (can this also be a link?).

> +    /// returned. For ZSTs no memory is allocated.
> +    pub fn new(x: T, flags: Flags) -> Result<Self, AllocError> {
> +        let b = Self::new_uninit(flags)?;
> +        Ok(Box::write(b, x))
> +    }
> +
> +    /// Creates a new `Box<T, A>` with uninitialized contents.
> +    ///
> +    /// New memory is allocated with `a`. The allocation may fail, in which case an error is

Ditto.

> +    /// returned. For ZSTs no memory is allocated.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// let b = KBox::<u64>::new_uninit(GFP_KERNEL)?;
> +    /// let b = KBox::write(b, 24);
> +    ///
> +    /// assert_eq!(*b, 24_u64);
> +    ///
> +    /// # Ok::<(), Error>(())
> +    /// ```
> +    pub fn new_uninit(flags: Flags) -> Result<Box<MaybeUninit<T>, A>, AllocError> {
> +        let ptr = if Self::is_zst() {
> +            NonNull::dangling()
> +        } else {
> +            let layout = core::alloc::Layout::new::<MaybeUninit<T>>();
> +            let ptr = A::alloc(layout, flags)?;
> +
> +            ptr.cast()
> +        };
> +
> +        Ok(Box(ptr, PhantomData::<A>))

Missing INVARIANT comment.

---
Cheers,
Benno

> +    }

Powered by blists - more mailing lists