Re: [PATCH 06/10] rust: alloc: introduce the `BoxExt` trait

[Wedson Almeida Filho]

On Tue, 26 Mar 2024 at 10:30, Benno Lossin <benno.lossin@xxxxxxxxx> wrote:
>
> On 26.03.24 01:17, Wedson Almeida Filho wrote:
> > On Mon, 25 Mar 2024 at 19:37, Benno Lossin <benno.lossin@xxxxxxxxx> wrote:
> >>
> >> On 25.03.24 20:54, Wedson Almeida Filho wrote:
> >>> From: Wedson Almeida Filho <walmeida@xxxxxxxxxxxxx>
> >>>
> >>> 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@xxxxxxxxxxxxx>
> >>> ---
> >>>    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;
>
> One thing I forgot to say: I think these modules should be named
> `box_ext` and `vec_ext`. It fits better with the usual style.
>
> >>>
> >>>    /// 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...
>
> Ah I see, I think it would be nice to not need this. Let's see what the
> new build system can do here.
>
> >
> >>> +
> >>> +    #[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.
>
> Thanks for the explanation, again it would be nice to be able to just
> write `Box::new`.

Indeed, that's what I had originally but we don't want to break rusttest.