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

[Danilo Krummrich <dakr@redhat.com>]

On Wed, May 01, 2024 at 03:46:31PM +0000, Benno Lossin wrote:
> 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`.

Indeed, good catch.

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

Ok, so I guess we keep BoxExt<T> for everthing and just separate VecExt<T> and
VecExtAlloc<T, A>.

- Danilo

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