Message-ID: <ZuCEneJeXcRo5qs8@pollux>
Date: Tue, 10 Sep 2024 19:40:45 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: 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, 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 v6 09/26] rust: alloc: implement kernel `Box`

On Sat, Aug 31, 2024 at 05:39:07AM +0000, Benno Lossin wrote:
> On 16.08.24 02:10, Danilo Krummrich wrote:
> > diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
> > new file mode 100644
> > index 000000000000..93b1ab9de6e8
> > --- /dev/null
> > +++ b/rust/kernel/alloc/kbox.rs
> > @@ -0,0 +1,480 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Implementation of [`Box`].
> > +
> > +#[allow(unused_imports)] // Used in doc comments.
> > +use super::allocator::{KVmalloc, Kmalloc, Vmalloc};
> > +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, InPlaceWrite, Init, PinInit};
> > +use crate::types::ForeignOwnable;
> > +
> > +/// The kernel's [`Box`] type - a heap allocation for a single value of type `T`.
> 
> A single `-` doesn't really render nicely in markdown, instead use a
> double or triple dash (`--` or `---`).
> 
> > +///
> > +/// This is the kernel's version of the Rust stdlib's `Box`. There are several of differences,
> > +/// for example no `noalias` attribute is emitted and partially moving out of a `Box` is not
> > +/// supported. There are also several API differences, e.g. `Box` always requires an [`Allocator`]
> > +/// implementation to be passed as generic, page [`Flags`] when allocating memory and all functions
> > +/// that may allocate memory are failable.
> 
> Do you mean fallible?
> 
> > +///
> > +/// `Box` works with any of the kernel's allocators, e.g. [`Kmalloc`], [`Vmalloc`] or [`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());
> > +/// ```
> 
> It would be nice if you could add something like "KBox can't handle big
> allocations:" above this example, so that people aren't confused why
> this example expects an error.

I don't think that's needed, it's implied by
`SIZE == bindings::KMALLOC_MAX_SIZE + 1`.

Surely, we could add it nevertheless, but it's not very precise to just say "big
allocations". And I think this isn't the place for lengthy explanations of
`Kmalloc` behavior.

> 
> > +///
> > +/// ```
> > +/// # 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());
> > +/// ```
> 
> Similarly, you could then say above this one "Instead use either `VBox`
> or `KVBox`:"
> 
> > +///
> > +/// # Invariants
> > +///
> > +/// The [`Box`]' pointer is always properly aligned and either points to memory allocated with `A`
> 
> Please use `self.0` instead of "[`Box`]'".
> 
> > +/// or, for zero-sized types, is a dangling pointer.
> 
> Probably "dangling, well aligned pointer.".

Does this add any value? For ZSTs everything is "well aligned", isn't it?

> 
> > +#[repr(transparent)]
> > +pub struct Box<T: ?Sized, A: Allocator>(NonNull<T>, PhantomData<A>);
> > +
> > +/// Type alias for `Box` with a [`Kmalloc`] allocator.
> 
> Can you make these `Box` references links?
> 
> > +///
> > +/// # 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.
> > +///
> > +/// # 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.
> > +///
> > +/// # 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 `Box` owns a `T`.
> > +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 `Box` owns a `T`.
> > +unsafe impl<T, A> Sync for Box<T, A>
> > +where
> > +    T: Sync + ?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
> > +    ///
> > +    /// For non-ZSTs, `raw` must point at an allocation allocated with `A`that is sufficiently
> > +    /// aligned for and holds a valid `T`. The caller passes ownership of the allocation to the
> > +    /// `Box`.
> 
> You don't say what must happen for ZSTs.

Because we don't require anything for a ZST, do we?

> 
> > +    #[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 {
> > +        // 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) }
> > +    }
> > +}
> > +
> > +impl<T, A> Box<MaybeUninit<T>, A>
> > +where
> > +    A: Allocator,
> > +{
> > +    /// Converts a `Box<MaybeUninit<T>, A>` to a `Box<T, A>`.
> > +    ///
> > +    /// It is undefined behavior to call this function while the value inside of `b` is not yet
> > +    /// fully initialized.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// Callers must ensure that the value inside of `b` is in an initialized state.
> > +    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) }
> 
> You should be able to use `.cast()` instead.
> 
> > +    }
> > +
> > +    /// 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.
> 
> The variable is called `b`.
> 
> > +        unsafe { Self::assume_init(b) }
> > +    }
> > +}
> 
> [...]
> 
> > +impl<T, A> Drop for Box<T, A>
> > +where
> > +    T: ?Sized,
> > +    A: Allocator,
> > +{
> > +    fn drop(&mut self) {
> > +        let size = core::mem::size_of_val::<T>(self);
> > +
> > +        // SAFETY: We need to drop `self.0` in place, before we free the backing memory.
> 
> This is the reason you are calling this function, not the justification
> why it is OK to do so. (the pointer is valid)
> 
> > +        unsafe { core::ptr::drop_in_place(self.0.as_ptr()) };
> 
> Instead of using the raw pointer directly, you can also just use
> `deref_mut`.
> 
> > +
> > +        if size != 0 {
> > +            // SAFETY: `ptr` was previously allocated with `A`.
> 
> There is no variable `ptr`, this is guaranteed by the type invariant of
> `Self`.
> 
> ---
> Cheers,
> Benno
> 
> > +            unsafe { A::free(self.0.cast()) };
> > +        }
> > +    }
> > +}
> > diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
> > index 4571daec0961..a9210634a8c3 100644
> > --- a/rust/kernel/prelude.rs
> > +++ b/rust/kernel/prelude.rs
> > @@ -14,7 +14,7 @@
> >  #[doc(no_inline)]
> >  pub use core::pin::Pin;
> > 
> > -pub use crate::alloc::{box_ext::BoxExt, flags::*, vec_ext::VecExt};
> > +pub use crate::alloc::{box_ext::BoxExt, flags::*, vec_ext::VecExt, KBox, KVBox, VBox};
> > 
> >  #[doc(no_inline)]
> >  pub use alloc::{boxed::Box, vec::Vec};
> > --
> > 2.46.0
> > 
> 

Powered by blists - more mailing lists