Received-SPF: pass (google.com: domain of linux-kernel+bounces-158815-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 25 Apr 2024 17:36:20 +0200
From: Danilo Krummrich <dakr@redhat.com>
To: Wedson Almeida Filho <wedsonaf@gmail.com>, Zhi Wang <zhiw@nvidia.com>,
	rust-for-linux@vger.kernel.org
Cc: 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>,
	Benno Lossin <benno.lossin@proton.me>,
	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: <Zip4dFpt8xhk7UBL@cassiopeiae>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <cbeae504-2752-4ef0-b103-e62fcf45ab93@redhat.com>

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

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.

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@nvidia.com/

> 
> - Danilo
> 
> [1] https://gitlab.freedesktop.org/drm/nova/-/blob/topic/firmware/rust/kernel/firmware.rs?ref_type=heads#L63
> 
> > 
> > ---
> > 
> > Changes in v3:
> > - Rebased on top of the latest `rust-next` branch.
> > - Updated `krealloc_aligned` to use `Flags` instead of `bindings::gfp_t`.
> > - Added __GFP_ZERO to flags, as part of the previous change.
> > - Avoiding temporary stack value in `Box::new_uninit`.
> > - Implement `Box::new` using `Box::new_uninit` (so only one of them actually
> >    allocates).
> > - Added examples/tests to `VecExt` methods.
> > - Fixed bug in length in `extend_from_slice`
> > - Link to v2: https://lore.kernel.org/rust-for-linux/20240327023531.187880-1-wedsonaf@gmail.com/T/#t
> > 
> > Changes in v2:
> > - Updated description of `alloc` crate.
> > - Renamed vecext and boxext modules to vec_ext and box_ext.
> > - Added derive directive to `AllocError`.
> > - Updated safety comment in `BoxExt::new`.
> > - Updated `VecExt::push` and `VecExt::extend_from_slice` to use
> >    `spare_capacity_mut`
> > - Added directive to not compile `destructure` and `rebuild` when `test` or
> >    `testlib` are configured. Otherwise we have a warning because `push` and
> >    `extend_from_slice` don't use them anymore.
> > - Updated indentation in `Arc::new_uninit`
> > - Moved the removal of `TryReserveError` convesion to `Error` to patch 7, where
> >    usage of `TryReserveError` is actually removed.
> > - Link to v1: https://lore.kernel.org/rust-for-linux/20240325195418.166013-1-wedsonaf@gmail.com/T/#t
> > 
> > Wedson Almeida Filho (10):
> >    rust: kernel: move `allocator` module under `alloc`
> >    rust: alloc: introduce the `VecExt` trait
> >    kbuild: use the upstream `alloc` crate
> >    rust: alloc: remove our fork of the `alloc` crate
> >    rust: alloc: introduce allocation flags
> >    rust: alloc: introduce the `BoxExt` trait
> >    rust: alloc: update `VecExt` to take allocation flags
> >    rust: sync: update `Arc` and `UniqueArc` to take allocation flags
> >    rust: init: update `init` module to take allocation flags
> >    rust: kernel: remove usage of `allocator_api` unstable feature
> > 
> >   rust/Makefile                        |   16 +-
> >   rust/alloc/README.md                 |   36 -
> >   rust/alloc/alloc.rs                  |  452 ----
> >   rust/alloc/boxed.rs                  | 2463 -----------------
> >   rust/alloc/collections/mod.rs        |  160 --
> >   rust/alloc/lib.rs                    |  288 --
> >   rust/alloc/raw_vec.rs                |  611 -----
> >   rust/alloc/slice.rs                  |  890 -------
> >   rust/alloc/vec/drain.rs              |  255 --
> >   rust/alloc/vec/extract_if.rs         |  115 -
> >   rust/alloc/vec/into_iter.rs          |  454 ----
> >   rust/alloc/vec/is_zero.rs            |  204 --
> >   rust/alloc/vec/mod.rs                | 3683 --------------------------
> >   rust/alloc/vec/partial_eq.rs         |   49 -
> >   rust/alloc/vec/set_len_on_drop.rs    |   35 -
> >   rust/alloc/vec/spec_extend.rs        |  119 -
> >   rust/bindings/bindings_helper.h      |    3 +
> >   rust/kernel/alloc.rs                 |   74 +
> >   rust/kernel/{ => alloc}/allocator.rs |   17 +-
> >   rust/kernel/alloc/box_ext.rs         |   59 +
> >   rust/kernel/alloc/vec_ext.rs         |  176 ++
> >   rust/kernel/error.rs                 |   13 +-
> >   rust/kernel/init.rs                  |   57 +-
> >   rust/kernel/lib.rs                   |    5 +-
> >   rust/kernel/prelude.rs               |    2 +
> >   rust/kernel/str.rs                   |    6 +-
> >   rust/kernel/sync/arc.rs              |   50 +-
> >   rust/kernel/sync/condvar.rs          |    2 +-
> >   rust/kernel/sync/lock/mutex.rs       |    2 +-
> >   rust/kernel/sync/lock/spinlock.rs    |    2 +-
> >   rust/kernel/types.rs                 |    4 +-
> >   rust/kernel/workqueue.rs             |   14 +-
> >   samples/rust/rust_minimal.rs         |    6 +-
> >   samples/rust/rust_print.rs           |    4 +-
> >   scripts/generate_rust_analyzer.py    |    2 +-
> >   35 files changed, 405 insertions(+), 9923 deletions(-)
> >   delete mode 100644 rust/alloc/README.md
> >   delete mode 100644 rust/alloc/alloc.rs
> >   delete mode 100644 rust/alloc/boxed.rs
> >   delete mode 100644 rust/alloc/collections/mod.rs
> >   delete mode 100644 rust/alloc/lib.rs
> >   delete mode 100644 rust/alloc/raw_vec.rs
> >   delete mode 100644 rust/alloc/slice.rs
> >   delete mode 100644 rust/alloc/vec/drain.rs
> >   delete mode 100644 rust/alloc/vec/extract_if.rs
> >   delete mode 100644 rust/alloc/vec/into_iter.rs
> >   delete mode 100644 rust/alloc/vec/is_zero.rs
> >   delete mode 100644 rust/alloc/vec/mod.rs
> >   delete mode 100644 rust/alloc/vec/partial_eq.rs
> >   delete mode 100644 rust/alloc/vec/set_len_on_drop.rs
> >   delete mode 100644 rust/alloc/vec/spec_extend.rs
> >   create mode 100644 rust/kernel/alloc.rs
> >   rename rust/kernel/{ => alloc}/allocator.rs (86%)
> >   create mode 100644 rust/kernel/alloc/box_ext.rs
> >   create mode 100644 rust/kernel/alloc/vec_ext.rs
> > 
> > 
> > base-commit: 768409cff6cc89fe1194da880537a09857b6e4db