Re: [PATCH WIP 6/8] rust: alloc: implement BoxExtAlloc

[Benno Lossin <benno.lossin@proton.me>]

On 01.05.24 15:06, Danilo Krummrich wrote:
> On Wed, May 01, 2024 at 08:53:51AM +0000, Benno Lossin wrote:
>> On 29.04.24 22:11, Danilo Krummrich wrote:
>>> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
>>> ---
>>>   rust/kernel/alloc/box_ext.rs | 40 +++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 39 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/rust/kernel/alloc/box_ext.rs b/rust/kernel/alloc/box_ext.rs
>>> index 76653d6f4257..74d6bb24e965 100644
>>> --- a/rust/kernel/alloc/box_ext.rs
>>> +++ b/rust/kernel/alloc/box_ext.rs
>>> @@ -2,7 +2,7 @@
>>>
>>>   //! Extensions to [`Box`] for fallible allocations.
>>>
>>> -use super::Flags;
>>> +use super::{AllocatorWithFlags, Flags};
>>>   use alloc::boxed::Box;
>>>   use core::alloc::AllocError;
>>>   use core::mem::MaybeUninit;
>>> @@ -21,6 +21,19 @@ pub trait BoxExt<T>: Sized {
>>>       fn new_uninit(flags: Flags) -> Result<Box<MaybeUninit<T>>, AllocError>;
>>>   }
>>>
>>> +/// Extensions to [`Box`].
>>> +pub trait BoxExtAlloc<T, A: AllocatorWithFlags>: Sized {
>>
>> I think it would be better to put all of this stuff into the `BoxExt`
>> trait. Otherwise we will end up with even more extension traits...
> 
> That was my inital idea as well, but it wasn't quite obvious for me how to make
> this happen.
> 
> I think for BoxExt<T>, it's rather easy, since we don't need an implementation
> for Box<T, A>. What do you think about this [1]?

In the `.._in` functions you're not passing the allocator to the `Box`.
This will lead to problems, since it will use the global allocator to
deallocate the value when you drop the `Box`.

> For VecExt<T>, I'm not quite sure how this can work though, since we'd need to
> implement VecExt<T> for both Vec<T> and Vec<T, A>.
> 
> For instance, VecExt<T>::reserve() needs to be implemented for both Vec<T> and
> Vec<T, A>, but VecExt<T>::with_capacity() can only be implemented for Vec<T> and
> VecExt<T>::with_capacity_in() can only be implemented for Vec<T, A>. But we have
> to implement all trait functions for both types.
> 
> Do I miss something here?

You're right, we need two traits for that...

-- 
Cheers,
Benno

> [1] https://gitlab.freedesktop.org/drm/nova/-/snippets/7784