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

Benno Lossin
2024-03-26T13:30:42Z

On 26.03.24 01:17, Wedson Almeida Filho wrote:
> 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;

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