Re: [PATCH v3 00/10] Allocation APIs

[Danilo Krummrich]

On Thu, Apr 25, 2024 at 04:09:46PM +0000, Benno Lossin wrote:
> On 25.04.24 17:36, Danilo Krummrich wrote:
> > (adding folks from [1])
> > 
> > On Tue, Apr 23, 2024 at 05:43:08PM +0200, Danilo Krummrich wrote:
> >> Hi all,
> >>
> >> On 3/28/24 02:35, Wedson Almeida Filho wrote:
> >>> From: Wedson Almeida Filho <walmeida@xxxxxxxxxxxxx>
> >>>
> >>> Revamp how we use the `alloc` crate.
> >>>
> >>> We currently have a fork of the crate with changes to `Vec`; other
> >>> changes have been upstreamed (to the Rust project). This series removes
> >>> the fork and exposes all the functionality as extension traits.
> >>>
> >>> Additionally, it also introduces allocation flag parameters to all
> >>> functions that may result in allocations (e.g., `Box::new`, `Arc::new`,
> >>> `Vec::push`, etc.) without the `try_` prefix -- the names are available
> >>> because we build `alloc` with `no_global_oom_handling`.
> >>>
> >>> Lastly, the series also removes our reliance on the `allocator_api`
> >>> unstable feature.
> >>>
> >>> Long term, we still want to make such functionality available in
> >>> upstream Rust, but this allows us to make progress now and reduces our
> >>> maintainance burden.
> >>>
> >>> In summary:
> >>> 1. Removes `alloc` fork
> >>> 2. Removes use of `allocator_api` unstable feature
> >>> 3. Introduces flags (e.g., GFP_KERNEL, GFP_ATOMIC) when allocating
> >>
> >> With that series, how do we implement alternative allocators, such as
> >> (k)vmalloc or DMA coherent?
> >>
> >> For instance, I recently sketched up some firmware bindings we want to
> >> use in Nova providing
> >>
> >> fn copy<A: core::alloc::Allocator>(&self, alloc: A) -> Result<Vec<u8, A>>
> >> [1]
> >>
> >> making use of Vec::try_with_capacity_in(). How would I implement
> >> something similar now?
> > 
> > I want to follow up on this topic after also bringing it up in yesterday's
> > weekly Rust call.
> > 
> > In the call a few ideas were discussed, e.g. whether we could just re-enable the
> > allocator_api feature and try getting it stabilized.
> > 
> > With the introduction of alloc::Flags (gfp_t abstraction) allocator_api might
> > not be a viable choice anymore.
> 
> Bringing in some more context from the meeting: Gary suggested we create
> a custom trait for allocators that can also handle allocation flags:
> 
>      pub trait AllocatorWithFlags: Allocator {
>          type Flags;
>          
>          fn allocate_with_flags(&self, layout: Layout, flags: Self::Flags) -> Result<NonNull<[u8]>, AllocError>;
> 
>          /* ... */
>      }
>      
>      impl AllocatorWithFlags for Global { /* ... */ }
>      
>      impl<T, A> VecExt<T> for Vec<T, A> where A: AllocatorWithFlags {
>          /* ... */
>      }
> 
> I think that this would work, but we would have to ensure that users are
> only allowed to call allocating functions if they are functions that we
> control. For example `Vec::try_reserve` [1] would still use the normal
> `Allocator` trait that doesn't support our flags.
> Gary noted that this could be solved by `klint` [2].

I agree, extending the Allocator trait should work.

Regarding allocating functions we don't control, isn't that the case already?
AFAICS, we're currently always falling back to GFP_KERNEL when calling
Vec::try_reserve().

But yes, I also think it would be better to enforce being explicit.

Given that, is there any value extending the existing Allocator trait at all?

> 
> 
> But we only need to extend the allocator API, if you want to use the std
> library types that allocate. If you would also be happy with a custom
> newtype wrapper, then we could also do that.

What do you mean with "custom newtype wrapper"?

> I think that we probably want a more general solution (ie `Allocator`
> enriched with flags), but we would have to design that before you can
> use it.
> 
> 
> [1]: https://doc.rust-lang.org/alloc/vec/struct.Vec.html#method.try_reserve
> [2]: https://github.com/Rust-for-Linux/klint
> 
> > 
> > I think it would work for (k)vmalloc, where we could pass the page flags through
> > const generics for instance.
> > 
> > But I don't see how it could work with kmem_cache, where we can't just create a
> > new allocator instance when we want to change the page flags, but need to
> > support allocations with different page flags on the same allocator (same
> > kmem_cache) instance.
> 
> I think that you can write the `kmem_cache` abstraction without using
> the allocator api. You just give the function that allocates a `flags`
> argument like in C.

Guess you mean letting the kmem_cache implementation construct the corresponding
container? Something like:

KmemCache<T>::alloc_box(flags: alloc::Flags) -> Box<T>

I think that'd make a lot of sense, since the size of an allocation is fixed
anyways.

> 
> The `Allocator` API might make it more *convenient* to use it, because
> you don't have to explicitly pass the flags every time (since the flags
> are determined by the allocator). But I have also heard that it might be
> desirable to always be explicit.
> 
> -- 
> Cheers,
> Benno
> 
> > 
> > So, I think we have to create our own allocator trait / API.
> > 
> > Any other thoughts on that?
> > 
> > - Danilo
> > 
> > [1] https://lore.kernel.org/rust-for-linux/20240408094738.00005e59.zhiw@xxxxxxxxxx/
> > 
>