Message-ID: <20240425210338.00007748.zhiw@nvidia.com>
Date: Thu, 25 Apr 2024 21:03:38 +0300
From: Zhi Wang <zhiw@...dia.com>
To: Benno Lossin <benno.lossin@...ton.me>
CC: Danilo Krummrich <dakr@...hat.com>, Wedson Almeida Filho
	<wedsonaf@...il.com>, <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>, <ajanulgu@...hat.com>, Andy Currid
	<acurrid@...dia.com>, Neo Jia <cjia@...dia.com>, John Hubbard
	<jhubbard@...dia.com>
Subject: Re: [PATCH v3 00/10] Allocation APIs

On Thu, 25 Apr 2024 16:09:46 +0000
Benno Lossin <benno.lossin@...ton.me> 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@...rosoft.com>
> >>>
> >>> 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].
> 
> 
> 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.
> 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.
> 

I agree that we should have a trait for allocator API. I think the
purpose of the trait is serving different upper-layer memory allocation
APIs that wants to have Vec/Box kind-like methods.

Look at the rust DMA memory allocation nowadays, there is already a
similar kind of of allocator with similar requirements. They might sit
on the same allocator API and implement traits with their kernel memory
allocation APIs if the trait is properly defined.

I think it is essential for a kernel driver to know the essence of the
heap that a Vec/Box is using. Hiding it away from the driver doesn't
look promising to me.

For wrapping the kernel memory allocation APIs, it can be done by
either having one unique Vec/Box API with different flags or having
different kind of xxVec/xxBox (Maybe it doesn't even need to be named
as xxVec or xxBox). Personally, I prefer the later approach that is
identical to the current kernel memory allocation APIs and more
straight-forward.

Thanks,
Zhi.

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

Powered by blists - more mailing lists