Message-ID: <4HXcIXSpP2nHdpuTb2aAIwspZ8Az9XYU3KUp18oUlsi3yIqXNPvbe80x57P0hLIYHrlf67DUCxD96blhqCsGmR3oecopCnaZA6kRlvKCl5g=@proton.me>
Date: Tue, 26 Mar 2024 13:30:23 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Wedson Almeida Filho <wedsonaf@...il.com>
Cc: rust-for-linux@...r.kernel.org, Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...sung.com>, Alice Ryhl <aliceryhl@...gle.com>, linux-kernel@...r.kernel.org, Wedson Almeida Filho <walmeida@...rosoft.com>
Subject: Re: [PATCH 06/10] rust: alloc: introduce the `BoxExt` trait

On 26.03.24 01:17, Wedson Almeida Filho wrote:
> On Mon, 25 Mar 2024 at 19:37, Benno Lossin <benno.lossin@...ton.me> wrote:
>>
>> On 25.03.24 20:54, Wedson Almeida Filho wrote:
>>> From: Wedson Almeida Filho <walmeida@...rosoft.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@...rosoft.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;

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

-- 
Cheers,
Benno

Powered by blists - more mailing lists