Received-SPF: pass (google.com: domain of linux-kernel+bounces-159874-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 26 Apr 2024 12:31:55 +0200
From: Danilo Krummrich <dakr@redhat.com>
To: Benno Lossin <benno.lossin@proton.me>
Cc: Wedson Almeida Filho <wedsonaf@gmail.com>, Zhi Wang <zhiw@nvidia.com>,
	rust-for-linux@vger.kernel.org, Miguel Ojeda <ojeda@kernel.org>,
	Alex Gaynor <alex.gaynor@gmail.com>,
	Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>,
	=?iso-8859-1?Q?Bj=F6rn?= Roy Baron <bjorn3_gh@protonmail.com>,
	Andreas Hindborg <a.hindborg@samsung.com>,
	Alice Ryhl <aliceryhl@google.com>, linux-kernel@vger.kernel.org,
	Wedson Almeida Filho <walmeida@microsoft.com>, ajanulgu@redhat.com,
	Andy Currid <acurrid@nvidia.com>, Neo Jia <cjia@nvidia.com>,
	John Hubbard <jhubbard@nvidia.com>
Subject: Re: [PATCH v3 00/10] Allocation APIs
Message-ID: <ZiuCm4VtFiCfzTPI@pollux>
References: <Zip4dFpt8xhk7UBL@cassiopeiae>
 <74cbdaf7-360e-47e3-bda4-4661422a11ae@proton.me>
 <ZiqkIQTHe9apd-LW@pollux>
 <f71c4e07-9f6f-4709-a3bb-012e644c4845@proton.me>
 <Zirfyp_NiYCRQYvk@cassiopeiae>
 <71dd99fe-0d64-47cc-b367-8fdd4fcdbdca@proton.me>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <71dd99fe-0d64-47cc-b367-8fdd4fcdbdca@proton.me>

On Fri, Apr 26, 2024 at 06:32:26AM +0000, Benno Lossin wrote:
> On 26.04.24 00:57, Danilo Krummrich wrote:
> > On Thu, Apr 25, 2024 at 08:52:16PM +0000, Benno Lossin wrote:
> >> On 25.04.24 20:42, Danilo Krummrich wrote:
> >>> 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@microsoft.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].
> >>>
> >>> 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().
> >>
> >> Yes we're falling back to that, but
> >> 1. there are currently no calls to `try_reserve` in tree,
> >> 2. if you use eg a `vmalloc` allocator, then I don't know if it would be
> >>      fine to reallocate that pointer using `krealloc`. I assumed that that
> >>      would not be OK (hence my extra care with functions outside of our
> >>      control).
> > 
> > Well, this would indeed not be valid. However, a vmalloc allocater wouldn't
> > implement realloc() this way.
> 
> Oh yeah that is correct.
> 
> > Or are you saying that Vec always uses the global allocator in that case? Why
> > would it do that?
> 
> No, it would use the vmalloc allocator.
> 
> So I guess the issue isn't as bad as I at first thought. I still think
> we should lint for this though (but maybe a warning instead of an error).
> 
> >>> 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?
> >>
> >> This is what I meant in the meeting by "do you really need the allocator
> >> trait?". What you lose is the ability to use `Vec` and `Box`, instead
> > 
> > Oh, indeed. I forgot about that when I wrote that. In that case I feel like it's
> > worth extending the existing allocator_api.
> > 
> >> you have to use your own wrapper types (see below). But what you gain is
> >> freedom to experiment. In the end we should still try to upstream our
> >> findings to Rust or at least share our knowledge, but doing that from
> >> the get-go is not ideal for productivity.
> >>
> >>>> 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"?
> >>
> >> You create a newtype struct ("newtype" means that it wraps an inner type
> >> and adds/removes/changes features from the inner type):
> >>
> >>       pub struct BigVec<T>(Vec<T>);
> >>
> >> And then you implement the common operations on it:
> >>
> >>       impl<T> BigVec<T> {
> >>           pub fn push(&mut self, item: T) -> Result {
> >>               self.reserve(1)?;
> >>
> >>               self.0.spare_capacity_mut()[0].write(item);
> >>
> >>               // SAFETY: <omitted for brevity>
> >>               unsafe { self.0.set_len(self.0.len() + 1) };
> >>               Ok(())
> >>           }
> >>
> >>           pub fn reserve(&mut self, additional: usize) -> Result {
> >>               /*
> >>                * implemented like `VecExt::reserve` from this patchset,
> >>                * except that it uses `vmalloc` instead of `krealloc`.
> >>                */
> >>           }
> >>       }
> >>
> >> If we need several of these, we can also create a general API that
> >> makes it easier to create them and avoids the duplication.
> > 
> > Thanks for for explaining.
> > 
> > I'd probably tend to extending allocator_api then. Do you see any major
> > advantages / disadvantages doing one or the other?
> 
> So aside from being able to use `Vec` and `Box` etc, I don't think there
> are any advantages to using `allocator_api`. The disadvantages are that
> it's another unstable feature that we need to get stabilized in some
> form. So it increases the amount of time it takes for us to be able to
> support multiple versions of Rust.
> 
> I think it's fine for you to experiment with the `allocator_api` and see
> where that leads you. But when we discuss merging patches that enable
> unstable features, we should be sure that the feature is truly needed.
> And that it cannot be replaced by custom code (it also depends on how
> complicated it is, but I think `allocator_api` would be simple enough).

I agree, though I'm not asking to re-enable allocator_api necessarily.

My original question regarding this series was what's the plan on how to
implement alternative allocators given the changes of this series.

This series clearly is a huge improvement, however, before it was quite clear
how to implement alternative allocators. Now it's rather unclear.

It's good that we discuss the options now and I'm also happy to contribute to
the implementation, but I also think that within the context of this series we
should give an answer to this question.

- Danilo

> 
> -- 
> Cheers,
> Benno
>