Message-ID: <CAH5fLgiyDtMWKe0db+uvKGPoPRepjiSxddSrBh8jF1N1COpdDg@mail.gmail.com>
Date: Wed, 14 Aug 2024 10:26:10 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Danilo Krummrich <dakr@...nel.org>
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 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.

> +/// ```
> +///
> +/// ```
> +/// # 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".

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

> +
> +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`. For ZSTs, the pointer
must be non-null and aligned.

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

Alice

Powered by blists - more mailing lists