Received-SPF: pass (google.com: domain of linux-kernel+bounces-118209-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240325195418.166013-1-wedsonaf@gmail.com> <20240325195418.166013-7-wedsonaf@gmail.com>
 <9AmQ4moOGHvMXp-MH65qG1fS7v14hypIjEwsPlYLdLJD2NMVOQFwFvzpETrxHSSMt-Y6Gz_TZfwXgROJys72ZIpB-Je4obAuZ2knpT9R3yo=@proton.me>
In-Reply-To: <9AmQ4moOGHvMXp-MH65qG1fS7v14hypIjEwsPlYLdLJD2NMVOQFwFvzpETrxHSSMt-Y6Gz_TZfwXgROJys72ZIpB-Je4obAuZ2knpT9R3yo=@proton.me>
From: Wedson Almeida Filho <wedsonaf@gmail.com>
Date: Mon, 25 Mar 2024 21:17:52 -0300
Message-ID: <CANeycqrbuzDwDhUjz+rZv2Q_peK54L1yPG6A1L6-PwjyLKiSAw@mail.gmail.com>
Subject: Re: [PATCH 06/10] rust: alloc: introduce the `BoxExt` trait
To: Benno Lossin <benno.lossin@proton.me>
Cc: rust-for-linux@vger.kernel.org, Miguel Ojeda <ojeda@kernel.org>, 
	Alex Gaynor <alex.gaynor@gmail.com>, Boqun Feng <boqun.feng@gmail.com>, 
	Gary Guo <gary@garyguo.net>, =?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= <bjorn3_gh@protonmail.com>, 
	Andreas Hindborg <a.hindborg@samsung.com>, Alice Ryhl <aliceryhl@google.com>, 
	linux-kernel@vger.kernel.org, Wedson Almeida Filho <walmeida@microsoft.com>
Content-Type: text/plain; charset="UTF-8"

On Mon, 25 Mar 2024 at 19:37, Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 25.03.24 20:54, Wedson Almeida Filho wrote:
> > From: Wedson Almeida Filho <walmeida@microsoft.com>
> >
> > Make fallible versions of `new` and `new_uninit` methods available in
> > `Box` even though it doesn't implement them because we build `alloc`
> > with the `no_global_oom_handling` config.
> >
> > They also have an extra `flags` parameter that allows callers to pass
> > flags to the allocator.
> >
> > Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> > ---
> >   rust/kernel/alloc.rs           |  1 +
> >   rust/kernel/alloc/allocator.rs |  6 +++-
> >   rust/kernel/alloc/boxext.rs    | 61 ++++++++++++++++++++++++++++++++++
> >   rust/kernel/init.rs            | 13 ++++----
> >   rust/kernel/prelude.rs         |  2 +-
> >   rust/kernel/sync/arc.rs        |  3 +-
> >   6 files changed, 77 insertions(+), 9 deletions(-)
> >   create mode 100644 rust/kernel/alloc/boxext.rs
> >
> > diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> > index ad48ac8dc13d..5712c81b1308 100644
> > --- a/rust/kernel/alloc.rs
> > +++ b/rust/kernel/alloc.rs
> > @@ -5,6 +5,7 @@
> >   #[cfg(not(test))]
> >   #[cfg(not(testlib))]
> >   mod allocator;
> > +pub mod boxext;
> >   pub mod vecext;
> >
> >   /// Flags to be used when allocating memory.
> > diff --git a/rust/kernel/alloc/allocator.rs b/rust/kernel/alloc/allocator.rs
> > index 01ad139e19bc..fc0439455faa 100644
> > --- a/rust/kernel/alloc/allocator.rs
> > +++ b/rust/kernel/alloc/allocator.rs
> > @@ -15,7 +15,11 @@
> >   ///
> >   /// - `ptr` can be either null or a pointer which has been allocated by this allocator.
> >   /// - `new_layout` must have a non-zero size.
> > -unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: bindings::gfp_t) -> *mut u8 {
> > +pub(crate) unsafe fn krealloc_aligned(
> > +    ptr: *mut u8,
> > +    new_layout: Layout,
> > +    flags: bindings::gfp_t,
> > +) -> *mut u8 {
> >       // Customized layouts from `Layout::from_size_align()` can have size < align, so pad first.
> >       let layout = new_layout.pad_to_align();
> >
> > diff --git a/rust/kernel/alloc/boxext.rs b/rust/kernel/alloc/boxext.rs
> > new file mode 100644
> > index 000000000000..26a918df7acf
> > --- /dev/null
> > +++ b/rust/kernel/alloc/boxext.rs
> > @@ -0,0 +1,61 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Extensions to [`Box`] for fallible allocations.
> > +
> > +use super::Flags;
> > +use alloc::boxed::Box;
> > +use core::alloc::AllocError;
> > +use core::mem::MaybeUninit;
> > +use core::result::Result;
> > +
> > +/// Extensions to [`Box`].
> > +pub trait BoxExt<T>: Sized {
> > +    /// Allocates a new box.
> > +    ///
> > +    /// The allocation may fail, in which case an error is returned.
> > +    fn new(x: T, flags: Flags) -> Result<Self, AllocError>;
> > +
> > +    /// Allocates a new uninitialised box.
> > +    ///
> > +    /// The allocation may fail, in which case an error is returned.
> > +    fn new_uninit(flags: Flags) -> Result<Box<MaybeUninit<T>>, AllocError>;
> > +}
> > +
> > +impl<T> BoxExt<T> for Box<T> {
> > +    #[cfg(any(test, testlib))]
> > +    fn new(x: T, _flags: Flags) -> Result<Self, AllocError> {
> > +        Ok(Box::new(x))
> > +    }
>
> When running under `cfg(test)`, are we using the normal standard
> library? Or why is this needed?

Because it uses 34 other crates that rely on `Box::new` and friends.

I discussed this with Miguel recently and once he's done with the
build system changes, he will think about what to do with tests. It
may be that we abandon the current method of running standalone tests
and run everything in kunit, or perhaps we'll find a way to exclude
code that won't run in standalone tests anyway...

> > +
> > +    #[cfg(not(any(test, testlib)))]
> > +    fn new(x: T, flags: Flags) -> Result<Self, AllocError> {
> > +        let ptr = if core::mem::size_of::<T>() == 0 {
> > +            core::ptr::NonNull::<T>::dangling().as_ptr()
> > +        } else {
> > +            let layout = core::alloc::Layout::new::<T>();
> > +
> > +            // SAFETY: Memory is being allocated (first arg is null). The only other source of
> > +            // safety issues is sleeping on atomic context, which is addressed by klint.
>
> The `krealloc_aligned` function states:
>
> /// # Safety
> ///
> /// - `ptr` can be either null or a pointer which has been allocated by this allocator.
> /// - `new_layout` must have a non-zero size.
>
> So it should also mention that you checked for `layout.size() > 0`
> above.

Good point. I mentioned this in the VecExt version but not here. I
will update this for v2.

> > +            let ptr = unsafe {
> > +                super::allocator::krealloc_aligned(core::ptr::null_mut(), layout, flags.0)
> > +            };
> > +            if ptr.is_null() {
> > +                return Err(AllocError);
> > +            }
> > +
> > +            let ptr = ptr.cast::<T>();
> > +
> > +            // SAFETY: We just allocated the memory above, it is valid for write.
> > +            unsafe { ptr.write(x) };
> > +            ptr
> > +        };
> > +
> > +        // SAFETY: For non-zero-sized types, we allocate above using the global allocator. For
> > +        // zero-sized types, we use `NonNull::dangling`.
> > +        Ok(unsafe { Box::from_raw(ptr) })
> > +    }
> > +
> > +    fn new_uninit(flags: Flags) -> Result<Box<MaybeUninit<T>>, AllocError> {
> > +        <Box<_> as BoxExt<_>>::new(MaybeUninit::<T>::uninit(), flags)
>
> Why do you use the extended syntax? I tried to use `Box::new` and it
> compiled.

It works when compiling the kernel but fails when compiling for
userspace with regular (no_global_oom_handling disabled) `alloc` when
running `make rusttest`. In the latter case, it chooses the inherent
version of `Box::new` which is infallible and doesn't take flags so it
fails to compile.

Using the extended syntax allows it always pick the right version,
regardless of how `alloc` is compiled.

There are 5 places in existing code that required this change and this
is limited to the kernel crate (e.g., drivers, samples and
documentation examples can continue to use `Box::new`). So we thought
it was ok until Miguel figures out what we want to do with tests.