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
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
---
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
--
2.34.1
On Wed, Mar 27, 2024 at 10:35:53PM -0300, 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
>
> ---
>
FWIW, I've put this into rust-dev:
https://github.com/Rust-for-Linux/linux rust-dev
a few adjustments are needed to work with other commits that have been
queued in rust-dev, so I add an commit on the top for everyone's
reference. (Besides this commit, I also removed all updates to our own
alloc in Miguel's 1.77.0 compiler version bump patch)
Regards,
Boqun
---------------------------------------------------->8
diff --git a/rust/kernel/file.rs b/rust/kernel/file.rs
index 8902f490ccc8..a5930f0f2bc5 100644
--- a/rust/kernel/file.rs
+++ b/rust/kernel/file.rs
@@ -6,13 +6,15 @@
//! [`include/linux/file.h`](srctree/include/linux/file.h)
use crate::{
+ alloc::AllocError,
bindings,
cred::Credential,
error::{code::*, Error, Result},
+ prelude::*,
types::{ARef, AlwaysRefCounted, NotThreadSafe, Opaque},
};
use alloc::boxed::Box;
-use core::{alloc::AllocError, mem, ptr};
+use core::{mem, ptr};
/// Flags associated with a [`File`].
pub mod flags {
@@ -348,10 +350,13 @@ impl DeferredFdCloser {
pub fn new() -> Result<Self, AllocError> {
Ok(Self {
// INVARIANT: The `file` pointer is null, so the type invariant does not apply.
- inner: Box::try_new(DeferredFdCloserInner {
- twork: mem::MaybeUninit::uninit(),
- file: core::ptr::null_mut(),
- })?,
+ inner: Box::new(
+ DeferredFdCloserInner {
+ twork: mem::MaybeUninit::uninit(),
+ file: core::ptr::null_mut(),
+ },
+ GFP_KERNEL,
+ )?,
})
}
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index c4a5e175b574..13a2166c4f41 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -302,7 +302,7 @@ pub fn ptr_eq(this: &Self, other: &Self) -> bool {
/// ```
/// use kernel::sync::{Arc, UniqueArc};
///
- /// let arc = Arc::try_new(42)?;
+ /// let arc = Arc::new(42, GFP_KERNEL)?;
/// let unique_arc = arc.into_unique_or_drop();
///
/// // The above conversion should succeed since refcount of `arc` is 1.
@@ -316,7 +316,7 @@ pub fn ptr_eq(this: &Self, other: &Self) -> bool {
/// ```
/// use kernel::sync::{Arc, UniqueArc};
///
- /// let arc = Arc::try_new(42)?;
+ /// let arc = Arc::new(42, GFP_KERNEL)?;
/// let another = arc.clone();
///
/// let unique_arc = arc.into_unique_or_drop();
On Fri, 29 Mar 2024 at 15:25, Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Wed, Mar 27, 2024 at 10:35:53PM -0300, 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
> >
> > ---
> >
>
> FWIW, I've put this into rust-dev:
>
> https://github.com/Rust-for-Linux/linux rust-dev
>
> a few adjustments are needed to work with other commits that have been
> queued in rust-dev, so I add an commit on the top for everyone's
> reference. (Besides this commit, I also removed all updates to our own
> alloc in Miguel's 1.77.0 compiler version bump patch)
>
> Regards,
> Boqun
>
> ---------------------------------------------------->8
> diff --git a/rust/kernel/file.rs b/rust/kernel/file.rs
> index 8902f490ccc8..a5930f0f2bc5 100644
> --- a/rust/kernel/file.rs
> +++ b/rust/kernel/file.rs
> @@ -6,13 +6,15 @@
> //! [`include/linux/file.h`](srctree/include/linux/file.h)
>
> use crate::{
> + alloc::AllocError,
I would do `alloc::{flags::*, AlocError},` here and add not add
`prelude::*` below.
> bindings,
> cred::Credential,
> error::{code::*, Error, Result},
> + prelude::*,
> types::{ARef, AlwaysRefCounted, NotThreadSafe, Opaque},
> };
> use alloc::boxed::Box;
> -use core::{alloc::AllocError, mem, ptr};
> +use core::{mem, ptr};
>
> /// Flags associated with a [`File`].
> pub mod flags {
> @@ -348,10 +350,13 @@ impl DeferredFdCloser {
> pub fn new() -> Result<Self, AllocError> {
> Ok(Self {
> // INVARIANT: The `file` pointer is null, so the type invariant does not apply.
> - inner: Box::try_new(DeferredFdCloserInner {
> - twork: mem::MaybeUninit::uninit(),
> - file: core::ptr::null_mut(),
> - })?,
> + inner: Box::new(
> + DeferredFdCloserInner {
> + twork: mem::MaybeUninit::uninit(),
> + file: core::ptr::null_mut(),
> + },
> + GFP_KERNEL,
> + )?,
> })
> }
>
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index c4a5e175b574..13a2166c4f41 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -302,7 +302,7 @@ pub fn ptr_eq(this: &Self, other: &Self) -> bool {
> /// ```
> /// use kernel::sync::{Arc, UniqueArc};
> ///
> - /// let arc = Arc::try_new(42)?;
> + /// let arc = Arc::new(42, GFP_KERNEL)?;
> /// let unique_arc = arc.into_unique_or_drop();
> ///
> /// // The above conversion should succeed since refcount of `arc` is 1.
> @@ -316,7 +316,7 @@ pub fn ptr_eq(this: &Self, other: &Self) -> bool {
> /// ```
> /// use kernel::sync::{Arc, UniqueArc};
> ///
> - /// let arc = Arc::try_new(42)?;
> + /// let arc = Arc::new(42, GFP_KERNEL)?;
> /// let another = arc.clone();
> ///
> /// let unique_arc = arc.into_unique_or_drop();
On Fri, Mar 29, 2024 at 11:25:00AM -0700, Boqun Feng wrote:
> On Wed, Mar 27, 2024 at 10:35:53PM -0300, 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
> >
> > ---
> >
>
> FWIW, I've put this into rust-dev:
>
> https://github.com/Rust-for-Linux/linux rust-dev
>
> a few adjustments are needed to work with other commits that have been
> queued in rust-dev, so I add an commit on the top for everyone's
> reference. (Besides this commit, I also removed all updates to our own
> alloc in Miguel's 1.77.0 compiler version bump patch)
>
> Regards,
> Boqun
As Miguel pointed out offline, the following is also needed:
diff --git a/rust/kernel/file.rs b/rust/kernel/file.rs
index a5930f0f2bc5..bf68a0ce9f14 100644
--- a/rust/kernel/file.rs
+++ b/rust/kernel/file.rs
@@ -350,7 +350,7 @@ impl DeferredFdCloser {
pub fn new() -> Result<Self, AllocError> {
Ok(Self {
// INVARIANT: The `file` pointer is null, so the type invariant does not apply.
- inner: Box::new(
+ inner: <Box<_> as BoxExt<_>>::new(
DeferredFdCloserInner {
twork: mem::MaybeUninit::uninit(),
file: core::ptr::null_mut(),
On Thu, Mar 28, 2024 at 2:36 AM Wedson Almeida Filho <wedsonaf@gmail.com> 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
Applied to `rust-next` -- thanks everyone!
[ Used `Box::write()` to avoid one `unsafe` block as suggested by Boqun. ]
Cheers,
Miguel
On Wed, 27 Mar 2024 22:35:53 -0300
Wedson Almeida Filho <wedsonaf@gmail.com> 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`.
>
IMO, It seems the allocation flag here means GFP flags according to
Patch 5 and I understand the benefit of introducing the flags.
I am interested in the future plan. With this change, will Vec and Box
stick to kernel memory application APIs with GFP flags in the future?
e.g. GUP, kmalloc, those mostly allocates continuous memory for small
objects. Is that the future for Vec and Box in kernel?
Is there any plan for having vmalloc() in rust kernel? Currently, if I
push a large object into a Vec, kernel MM will complain for sure. And
literally Vec/Box themselves don't imply to the user that they allocate
memory with limitations.
Kernel uses different MM alloc APIs for different usages. For rust,
should we have a different kind of "Vec/Box" or having a Vec/Box with
different set of allocation flags that covers both GFP MM APIs and
vmalloc()? I am curious about the plan.
> 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
>
> ---
>
> 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
On Mon, Apr 8, 2024 at 8:48 AM Zhi Wang <zhiw@nvidia.com> wrote: > > IMO, It seems the allocation flag here means GFP flags according to > Patch 5 and I understand the benefit of introducing the flags. > > I am interested in the future plan. With this change, will Vec and Box > stick to kernel memory application APIs with GFP flags in the future? > e.g. GUP, kmalloc, those mostly allocates continuous memory for small > objects. Is that the future for Vec and Box in kernel? > > Is there any plan for having vmalloc() in rust kernel? Currently, if I > push a large object into a Vec, kernel MM will complain for sure. And > literally Vec/Box themselves don't imply to the user that they allocate > memory with limitations. > > Kernel uses different MM alloc APIs for different usages. For rust, > should we have a different kind of "Vec/Box" or having a Vec/Box with > different set of allocation flags that covers both GFP MM APIs and > vmalloc()? I am curious about the plan. Sorry Zhi, Danilo brought this message to my attention today (thanks!). We have been trying to find the best way forward to support fallible allocations, per-call-site flags, other kernel allocators and so on for a long time, including discussions with upstream Rust. At this point, after these years, I think the ideal path is to try our best to avoid re-enabling `allocator_api` and to have perhaps our own custom APIs/types as needed (possibly several depending on the use case) -- please also see my reply at https://lore.kernel.org/rust-for-linux/CANiq72=0BNw-KiURBjosLqfuUEPpjZPbRg1XMFZyobOzBt7aMA@mail.gmail.com/ Regarding this patch series, we mainly wanted to make progress on (finally) allowing at least to use the GFP flags and also, importantly, dropping `alloc` since we want to start supporting several compiler versions soon. Thank you for your feedback on this. It would be nice to hear what you think about Danilo's approach in the other series and other possible approaches that may not rely on `allocator_api`. Cheers, Miguel
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?
- 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
(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
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].
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.
[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.
--
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@nvidia.com/
>
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().
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@nvidia.com/
> >
>
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).
> 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
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.
>> 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.
Yes, but you would probably need a newtype return type, since you need
to control how it is being freed. Also in the example above there is no
`kmem_cache` object that stores the state.
I think that the API would look more like this:
impl<T> KMemCache<T> {
pub fn alloc(&self, value: T, flags: Flags) -> Result<KMemObj<'_, T>>;
}
Here are a couple of interesting elements of this API:
- I don't know if `kmem_cache` uses the same flags as the alloc module,
if not you will need a custom `Flags` type.
- I assume that an object allocated by a `kmem_cache` is only valid
while the cache still exists (ie if you drop the cache, all allocated
objects are also invalidated). That is why the return type contains a
lifetime (`'_` refers to the elided lifetime on the `&self` parameter.
It means that the `KMemObj` is only valid while the `KMemCache` is
also valid).
- The return type is its own kind of smart pointer that allows you to
modify the inner value like `Box`, but it takes care of all the
`kmem_cache` specifics (eg ensuring that the associated cache is still
valid, freeing the object etc).r
Since I assumed several things, in the end the API might look different,
but I think that this could be a more fruitful starting point.
--
Cheers,
Benno
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.
Or are you saying that Vec always uses the global allocator in that case? Why
would it do that?
>
> > 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?
>
> >> 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.
>
> Yes, but you would probably need a newtype return type, since you need
> to control how it is being freed. Also in the example above there is no
> `kmem_cache` object that stores the state.
Sure, the above was just meant to see whether I understood you correctly.
>
> I think that the API would look more like this:
>
> impl<T> KMemCache<T> {
> pub fn alloc(&self, value: T, flags: Flags) -> Result<KMemObj<'_, T>>;
> }
>
> Here are a couple of interesting elements of this API:
> - I don't know if `kmem_cache` uses the same flags as the alloc module,
> if not you will need a custom `Flags` type.
> - I assume that an object allocated by a `kmem_cache` is only valid
> while the cache still exists (ie if you drop the cache, all allocated
> objects are also invalidated). That is why the return type contains a
> lifetime (`'_` refers to the elided lifetime on the `&self` parameter.
> It means that the `KMemObj` is only valid while the `KMemCache` is
> also valid).
> - The return type is its own kind of smart pointer that allows you to
> modify the inner value like `Box`, but it takes care of all the
> `kmem_cache` specifics (eg ensuring that the associated cache is still
> valid, freeing the object etc).r
>
> Since I assumed several things, in the end the API might look different,
> but I think that this could be a more fruitful starting point.
Thanks for the hints - your assumptions are correct. The page flags are the same.
>
> --
> Cheers,
> Benno
>
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).
--
Cheers,
Benno
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
>
On 4/26/24 12:31, Danilo Krummrich wrote: > On Fri, Apr 26, 2024 at 06:32:26AM +0000, Benno Lossin wrote: <snip> >> 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. In order to get a little closer to an answer I sketched up a patch series [1] to support alternative allocators again and added, besides the KernelAllocator itself, VmAllocator as an example. The patches can also be found in this tree [2]. Please let me know what you think. - Danilo [1] https://lore.kernel.org/rust-for-linux/20240429201202.3490-1-dakr@redhat.com/T/#u [2] https://gitlab.freedesktop.org/drm/nova/-/tree/topic/allocator
On Thu, 25 Apr 2024 16:09:46 +0000
Benno Lossin <benno.lossin@proton.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@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].
>
>
> 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.
>
© 2016 - 2026 Red Hat, Inc.