[PATCH WIP 0/8] Draft: Alternative allocator support

[PATCH WIP 0/8] Draft: Alternative allocator support

[Danilo Krummrich]

DISCLAIMER: This patch series does not raise a claim for inclusion in the kernel
(yet) and, instead, serves as a baseline for further discussion.

Hi,

This patch series re-enables the allocator_api and implements an extension trait
AllocatorWithFlags expanding the Allocator trait to allow for arbitrary
allocator implementations.

It expands Box<T, A> and Vec<T, A> by adding the BoxExtAlloc<T, A> and
VecExtAlloc<T, A> traits analogous to the existing BoxExt<T> and VecExt<T>
traits and, as an example, implements the VmAllocator (vmalloc).

This patch series is tested (I tested the VecExtAlloc<T, A> parts using my
firmware abstractions) and it seems to work quite well. However, since it's just
a draft there are no proper commit messages, no documentation and no safety
comments.

Also, I'm pretty sure there are some parts of BoxExtAlloc<T, A> and
VecExtAlloc<T, A> that, somehow, can be generalized with the existing BoxExt<T>
and VecExt<T> implementations.

Please let me know what you think.

- Danilo

Danilo Krummrich (8):
  rust: alloc: re-enable allocator_api
  rust: alloc: use AllocError from core::alloc
  rust: alloc: implement AllocatorWithFlags trait
  rust: alloc: separate krealloc_aligned()
  rust: alloc: implement AllocatorWithFlags for KernelAllocator
  rust: alloc: implement BoxExtAlloc
  rust: alloc: implement VecExtAlloc
  rust: alloc: implement vmalloc allocator

 rust/kernel/alloc.rs           |  25 ++++++--
 rust/kernel/alloc/allocator.rs | 103 ++++++++++++++++++++++++++++++---
 rust/kernel/alloc/box_ext.rs   |  41 ++++++++++++-
 rust/kernel/alloc/vec_ext.rs   |  86 ++++++++++++++++++++++++++-
 rust/kernel/error.rs           |   3 +-
 rust/kernel/init.rs            |   3 +-
 rust/kernel/lib.rs             |   1 +
 rust/kernel/str.rs             |   3 +-
 rust/kernel/sync/arc.rs        |   4 +-
 rust/kernel/workqueue.rs       |   3 +-
 10 files changed, 251 insertions(+), 21 deletions(-)


base-commit: 2c1092853f163762ef0aabc551a630ef233e1be3
-- 
2.44.0

[PATCH WIP 1/8] rust: alloc: re-enable allocator_api

[Danilo Krummrich]

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/kernel/lib.rs | 1 +
 1 file changed, 1 insertion(+)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 9a943d99c71a..1e910fe7c2c7 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -12,6 +12,7 @@
 //! do so first instead of bypassing this crate.
 
 #![no_std]
+#![feature(allocator_api)]
 #![feature(coerce_unsized)]
 #![feature(dispatch_from_dyn)]
 #![feature(new_uninit)]
-- 
2.44.0

[PATCH WIP 2/8] rust: alloc: use AllocError from core::alloc

[Danilo Krummrich]

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/kernel/alloc.rs         | 4 ----
 rust/kernel/alloc/box_ext.rs | 3 ++-
 rust/kernel/alloc/vec_ext.rs | 3 ++-
 rust/kernel/error.rs         | 3 ++-
 rust/kernel/init.rs          | 3 ++-
 rust/kernel/str.rs           | 3 ++-
 rust/kernel/sync/arc.rs      | 4 ++--
 rust/kernel/workqueue.rs     | 3 ++-
 8 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
index f1c2c4aa22d2..9bc1b48b5641 100644
--- a/rust/kernel/alloc.rs
+++ b/rust/kernel/alloc.rs
@@ -8,10 +8,6 @@
 pub mod box_ext;
 pub mod vec_ext;
 
-/// Indicates an allocation error.
-#[derive(Copy, Clone, PartialEq, Eq, Debug)]
-pub struct AllocError;
-
 /// Flags to be used when allocating memory.
 ///
 /// They can be combined with the operators `|`, `&`, and `!`.
diff --git a/rust/kernel/alloc/box_ext.rs b/rust/kernel/alloc/box_ext.rs
index cdbb5ad166d9..76653d6f4257 100644
--- a/rust/kernel/alloc/box_ext.rs
+++ b/rust/kernel/alloc/box_ext.rs
@@ -2,8 +2,9 @@
 
 //! Extensions to [`Box`] for fallible allocations.
 
-use super::{AllocError, Flags};
+use super::Flags;
 use alloc::boxed::Box;
+use core::alloc::AllocError;
 use core::mem::MaybeUninit;
 use core::result::Result;
 
diff --git a/rust/kernel/alloc/vec_ext.rs b/rust/kernel/alloc/vec_ext.rs
index 6a916fcf8bf1..1dc991b44be7 100644
--- a/rust/kernel/alloc/vec_ext.rs
+++ b/rust/kernel/alloc/vec_ext.rs
@@ -2,8 +2,9 @@
 
 //! Extensions to [`Vec`] for fallible allocations.
 
-use super::{AllocError, Flags};
+use super::Flags;
 use alloc::vec::Vec;
+use core::alloc::AllocError;
 use core::result::Result;
 
 /// Extensions to [`Vec`].
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index fc986bc24c6d..7151a5bd682a 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -4,10 +4,11 @@
 //!
 //! C header: [`include/uapi/asm-generic/errno-base.h`](srctree/include/uapi/asm-generic/errno-base.h)
 
-use crate::{alloc::AllocError, str::CStr};
+use crate::str::CStr;
 
 use alloc::alloc::LayoutError;
 
+use core::alloc::AllocError;
 use core::convert::From;
 use core::fmt;
 use core::num::TryFromIntError;
diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
index 9608f2bd2211..fec47b274ec3 100644
--- a/rust/kernel/init.rs
+++ b/rust/kernel/init.rs
@@ -211,13 +211,14 @@
 //! [`pin_init!`]: crate::pin_init!
 
 use crate::{
-    alloc::{box_ext::BoxExt, AllocError, Flags},
+    alloc::{box_ext::BoxExt, Flags},
     error::{self, Error},
     sync::UniqueArc,
     types::{Opaque, ScopeGuard},
 };
 use alloc::boxed::Box;
 use core::{
+    alloc::AllocError,
     cell::UnsafeCell,
     convert::Infallible,
     marker::PhantomData,
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index 27641c3e4df8..2e7ab4a620b6 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -2,8 +2,9 @@
 
 //! String representations.
 
-use crate::alloc::{flags::*, vec_ext::VecExt, AllocError};
+use crate::alloc::{flags::*, vec_ext::VecExt};
 use alloc::vec::Vec;
+use core::alloc::AllocError;
 use core::fmt::{self, Write};
 use core::ops::{self, Deref, DerefMut, Index};
 
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index c2a3a2c7cbc5..0866378f1360 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -16,7 +16,7 @@
 //! [`Arc`]: https://doc.rust-lang.org/std/sync/struct.Arc.html
 
 use crate::{
-    alloc::{box_ext::BoxExt, AllocError, Flags},
+    alloc::{box_ext::BoxExt, Flags},
     bindings,
     error::{self, Error},
     init::{self, InPlaceInit, Init, PinInit},
@@ -25,7 +25,7 @@
 };
 use alloc::boxed::Box;
 use core::{
-    alloc::Layout,
+    alloc::{AllocError, Layout},
     fmt,
     marker::{PhantomData, Unsize},
     mem::{ManuallyDrop, MaybeUninit},
diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index 9f47bad0b003..0027cc730d6b 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -132,9 +132,10 @@
 //!
 //! C header: [`include/linux/workqueue.h`](srctree/include/linux/workqueue.h)
 
-use crate::alloc::{AllocError, Flags};
+use crate::alloc::Flags;
 use crate::{bindings, prelude::*, sync::Arc, sync::LockClassKey, types::Opaque};
 use alloc::boxed::Box;
+use core::alloc::AllocError;
 use core::marker::PhantomData;
 use core::pin::Pin;
 
-- 
2.44.0

[PATCH WIP 3/8] rust: alloc: implement AllocatorWithFlags trait

[Danilo Krummrich]

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/kernel/alloc.rs | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
index 9bc1b48b5641..ec45ba8f77ce 100644
--- a/rust/kernel/alloc.rs
+++ b/rust/kernel/alloc.rs
@@ -8,6 +8,14 @@
 pub mod box_ext;
 pub mod vec_ext;
 
+use core::{
+    alloc::{Allocator, AllocError, Layout},
+    ptr,
+    ptr::NonNull,
+};
+
+use flags::*;
+
 /// Flags to be used when allocating memory.
 ///
 /// They can be combined with the operators `|`, `&`, and `!`.
@@ -68,3 +76,16 @@ pub mod flags {
     /// small allocations.
     pub const GFP_NOWAIT: Flags = Flags(bindings::GFP_NOWAIT);
 }
+
+pub unsafe trait AllocatorWithFlags: Allocator {
+    unsafe fn alloc_flags(&self, layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError>;
+    unsafe fn realloc_flags(&self, ptr: *mut u8, old_size: usize, layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError>;
+
+    fn default_allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
+        unsafe { self.realloc_flags(ptr::null_mut(), 0, layout, GFP_KERNEL) }
+    }
+
+    fn default_allocate_zeroed(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
+        unsafe { self.realloc_flags(ptr::null_mut(), 0, layout, GFP_KERNEL | __GFP_ZERO) }
+    }
+}
-- 
2.44.0

Re: [PATCH WIP 3/8] rust: alloc: implement AllocatorWithFlags trait

[Benno Lossin]

On 29.04.24 22:11, Danilo Krummrich wrote:
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
>  rust/kernel/alloc.rs | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> index 9bc1b48b5641..ec45ba8f77ce 100644
> --- a/rust/kernel/alloc.rs
> +++ b/rust/kernel/alloc.rs
> @@ -8,6 +8,14 @@
>  pub mod box_ext;
>  pub mod vec_ext;
> 
> +use core::{
> +    alloc::{Allocator, AllocError, Layout},
> +    ptr,
> +    ptr::NonNull,
> +};
> +
> +use flags::*;
> +
>  /// Flags to be used when allocating memory.
>  ///
>  /// They can be combined with the operators `|`, `&`, and `!`.
> @@ -68,3 +76,16 @@ pub mod flags {
>      /// small allocations.
>      pub const GFP_NOWAIT: Flags = Flags(bindings::GFP_NOWAIT);
>  }
> +
> +pub unsafe trait AllocatorWithFlags: Allocator {
> +    unsafe fn alloc_flags(&self, layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError>;
> +    unsafe fn realloc_flags(&self, ptr: *mut u8, old_size: usize, layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError>;

Please use `rustfmt`, there exists a make target that does it for you.

Also why are these functions `unsafe`? You haven't specified a `Safety`
section, which means nobody knows what the requirements are.

> +
> +    fn default_allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
> +        unsafe { self.realloc_flags(ptr::null_mut(), 0, layout, GFP_KERNEL) }
> +    }

Why do we want to have these functions? I think that it would make sense
to forbid all other ways of allocating memory. So only allow allocation
where the flags are explicitly passed.

-- 
Cheers,
Benno

> +
> +    fn default_allocate_zeroed(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
> +        unsafe { self.realloc_flags(ptr::null_mut(), 0, layout, GFP_KERNEL | __GFP_ZERO) }
> +    }
> +}
> --
> 2.44.0
> 

Re: [PATCH WIP 3/8] rust: alloc: implement AllocatorWithFlags trait

[Danilo Krummrich]

On Wed, May 01, 2024 at 08:32:01AM +0000, Benno Lossin wrote:
> On 29.04.24 22:11, Danilo Krummrich wrote:
> > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > ---
> >  rust/kernel/alloc.rs | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> > index 9bc1b48b5641..ec45ba8f77ce 100644
> > --- a/rust/kernel/alloc.rs
> > +++ b/rust/kernel/alloc.rs
> > @@ -8,6 +8,14 @@
> >  pub mod box_ext;
> >  pub mod vec_ext;
> > 
> > +use core::{
> > +    alloc::{Allocator, AllocError, Layout},
> > +    ptr,
> > +    ptr::NonNull,
> > +};
> > +
> > +use flags::*;
> > +
> >  /// Flags to be used when allocating memory.
> >  ///
> >  /// They can be combined with the operators `|`, `&`, and `!`.
> > @@ -68,3 +76,16 @@ pub mod flags {
> >      /// small allocations.
> >      pub const GFP_NOWAIT: Flags = Flags(bindings::GFP_NOWAIT);
> >  }
> > +
> > +pub unsafe trait AllocatorWithFlags: Allocator {
> > +    unsafe fn alloc_flags(&self, layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError>;
> > +    unsafe fn realloc_flags(&self, ptr: *mut u8, old_size: usize, layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError>;
> 
> Please use `rustfmt`, there exists a make target that does it for you.

Sure, gonna fix.

Out of curiosity, given that we have this make target, the expectation obviously
seems to be that all Rust code in the kernel must be formatted with this. I
think that's indeed desirable. Two questions regarding this:

How do we scale (as in ensure) this once Rust code becomes more common in
subsystems?

Is it possible for rustfmt to change rules unexpectedly? If so, how would we
deal with that?

> 
> Also why are these functions `unsafe`? You haven't specified a `Safety`
> section, which means nobody knows what the requirements are.

I think I did this because they did inherit the safety requirements from
krealloc() (as you also point out in another patch).

I think they shouldn't be unsafe, just as the corresponding Allocator ones
aren't.

Gonna change that.

> 
> > +
> > +    fn default_allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
> > +        unsafe { self.realloc_flags(ptr::null_mut(), 0, layout, GFP_KERNEL) }
> > +    }
> 
> Why do we want to have these functions? I think that it would make sense
> to forbid all other ways of allocating memory. So only allow allocation
> where the flags are explicitly passed.

I agree that we should forbid allocating memory with implicit default page
flags.

I added them, since I think we'll always need some implementation of the
Allocator trait. And this just was the obvious generic implementation.

If we agree to forbid allocating without page flags, we can just always return
an error?

> 
> -- 
> Cheers,
> Benno
> 
> > +
> > +    fn default_allocate_zeroed(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
> > +        unsafe { self.realloc_flags(ptr::null_mut(), 0, layout, GFP_KERNEL | __GFP_ZERO) }
> > +    }
> > +}
> > --
> > 2.44.0
> > 
> 

Re: [PATCH WIP 3/8] rust: alloc: implement AllocatorWithFlags trait

[Benno Lossin]

On 01.05.24 14:50, Danilo Krummrich wrote:
> On Wed, May 01, 2024 at 08:32:01AM +0000, Benno Lossin wrote:
>> On 29.04.24 22:11, Danilo Krummrich wrote:
>>> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
>>> ---
>>>   rust/kernel/alloc.rs | 21 +++++++++++++++++++++
>>>   1 file changed, 21 insertions(+)
>>>
>>> diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
>>> index 9bc1b48b5641..ec45ba8f77ce 100644
>>> --- a/rust/kernel/alloc.rs
>>> +++ b/rust/kernel/alloc.rs
>>> @@ -8,6 +8,14 @@
>>>   pub mod box_ext;
>>>   pub mod vec_ext;
>>>
>>> +use core::{
>>> +    alloc::{Allocator, AllocError, Layout},
>>> +    ptr,
>>> +    ptr::NonNull,
>>> +};
>>> +
>>> +use flags::*;
>>> +
>>>   /// Flags to be used when allocating memory.
>>>   ///
>>>   /// They can be combined with the operators `|`, `&`, and `!`.
>>> @@ -68,3 +76,16 @@ pub mod flags {
>>>       /// small allocations.
>>>       pub const GFP_NOWAIT: Flags = Flags(bindings::GFP_NOWAIT);
>>>   }
>>> +
>>> +pub unsafe trait AllocatorWithFlags: Allocator {
>>> +    unsafe fn alloc_flags(&self, layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError>;
>>> +    unsafe fn realloc_flags(&self, ptr: *mut u8, old_size: usize, layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError>;
>>
>> Please use `rustfmt`, there exists a make target that does it for you.
> 
> Sure, gonna fix.
> 
> Out of curiosity, given that we have this make target, the expectation obviously
> seems to be that all Rust code in the kernel must be formatted with this. I
> think that's indeed desirable. Two questions regarding this:
> 
> How do we scale (as in ensure) this once Rust code becomes more common in
> subsystems?

There also is the make target `rustfmtcheck` which can be used to check
if the code has been formatted correctly (ie it will error on wrongly
formatted code).
IIRC there were some plans to add such a check to CI, but I don't know
the status of that.

> Is it possible for rustfmt to change rules unexpectedly? If so, how would we
> deal with that?

AFAIK, `rustfmt` has a stable format [1] and if we really find
something that breaks our code, we can also add our own defaults to the
`.rustfmt.toml`.

[1]: https://rust-lang.github.io/rfcs/2437-rustfmt-stability.html

>> Also why are these functions `unsafe`? You haven't specified a `Safety`
>> section, which means nobody knows what the requirements are.
> 
> I think I did this because they did inherit the safety requirements from
> krealloc() (as you also point out in another patch).
> 
> I think they shouldn't be unsafe, just as the corresponding Allocator ones
> aren't.
> 
> Gonna change that.

Sounds good.

>>> +
>>> +    fn default_allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
>>> +        unsafe { self.realloc_flags(ptr::null_mut(), 0, layout, GFP_KERNEL) }
>>> +    }
>>
>> Why do we want to have these functions? I think that it would make sense
>> to forbid all other ways of allocating memory. So only allow allocation
>> where the flags are explicitly passed.
> 
> I agree that we should forbid allocating memory with implicit default page
> flags.
> 
> I added them, since I think we'll always need some implementation of the
> Allocator trait. And this just was the obvious generic implementation.
> 
> If we agree to forbid allocating without page flags, we can just always return
> an error?

There are multiple options that we could do here:
1. Return an error.
2. Print a warning at runtime and allocate regardless. Additionally use
    `klint` to lint against usage of that function.
3. Use `build_error!`.

I think that 1 could be very unexpected and hard to debug. 2 is better,
but at the moment `klint` is not required to run, so we would have to
change that. 3 is my preferred option, but I am not 100% confident that
it will work.

So maybe give option 3 a try and if that doesn't work, we can try to
come up with a different solution.

-- 
Cheers,
Benno

Re: [PATCH WIP 3/8] rust: alloc: implement AllocatorWithFlags trait

[Danilo Krummrich]

On 5/1/24 17:39, Benno Lossin wrote:
> On 01.05.24 14:50, Danilo Krummrich wrote:
>> On Wed, May 01, 2024 at 08:32:01AM +0000, Benno Lossin wrote:
>>> On 29.04.24 22:11, Danilo Krummrich wrote:
>>>> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
>>>> ---
>>>>    rust/kernel/alloc.rs | 21 +++++++++++++++++++++
>>>>    1 file changed, 21 insertions(+)
>>>>
>>>> diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
>>>> index 9bc1b48b5641..ec45ba8f77ce 100644
>>>> --- a/rust/kernel/alloc.rs
>>>> +++ b/rust/kernel/alloc.rs
>>>> @@ -8,6 +8,14 @@
>>>>    pub mod box_ext;
>>>>    pub mod vec_ext;
>>>>
>>>> +use core::{
>>>> +    alloc::{Allocator, AllocError, Layout},
>>>> +    ptr,
>>>> +    ptr::NonNull,
>>>> +};
>>>> +
>>>> +use flags::*;
>>>> +
>>>>    /// Flags to be used when allocating memory.
>>>>    ///
>>>>    /// They can be combined with the operators `|`, `&`, and `!`.
>>>> @@ -68,3 +76,16 @@ pub mod flags {
>>>>        /// small allocations.
>>>>        pub const GFP_NOWAIT: Flags = Flags(bindings::GFP_NOWAIT);
>>>>    }
>>>> +
>>>> +pub unsafe trait AllocatorWithFlags: Allocator {
>>>> +    unsafe fn alloc_flags(&self, layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError>;
>>>> +    unsafe fn realloc_flags(&self, ptr: *mut u8, old_size: usize, layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError>;
>>>
>>> Please use `rustfmt`, there exists a make target that does it for you.
>>
>> Sure, gonna fix.
>>
>> Out of curiosity, given that we have this make target, the expectation obviously
>> seems to be that all Rust code in the kernel must be formatted with this. I
>> think that's indeed desirable. Two questions regarding this:
>>
>> How do we scale (as in ensure) this once Rust code becomes more common in
>> subsystems?
> 
> There also is the make target `rustfmtcheck` which can be used to check
> if the code has been formatted correctly (ie it will error on wrongly
> formatted code).
> IIRC there were some plans to add such a check to CI, but I don't know
> the status of that.
> 
>> Is it possible for rustfmt to change rules unexpectedly? If so, how would we
>> deal with that?
> 
> AFAIK, `rustfmt` has a stable format [1] and if we really find
> something that breaks our code, we can also add our own defaults to the
> `.rustfmt.toml`.
> 
> [1]: https://rust-lang.github.io/rfcs/2437-rustfmt-stability.html
> 
>>> Also why are these functions `unsafe`? You haven't specified a `Safety`
>>> section, which means nobody knows what the requirements are.
>>
>> I think I did this because they did inherit the safety requirements from
>> krealloc() (as you also point out in another patch).
>>
>> I think they shouldn't be unsafe, just as the corresponding Allocator ones
>> aren't.
>>
>> Gonna change that.
> 
> Sounds good.
> 
>>>> +
>>>> +    fn default_allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
>>>> +        unsafe { self.realloc_flags(ptr::null_mut(), 0, layout, GFP_KERNEL) }
>>>> +    }
>>>
>>> Why do we want to have these functions? I think that it would make sense
>>> to forbid all other ways of allocating memory. So only allow allocation
>>> where the flags are explicitly passed.
>>
>> I agree that we should forbid allocating memory with implicit default page
>> flags.
>>
>> I added them, since I think we'll always need some implementation of the
>> Allocator trait. And this just was the obvious generic implementation.
>>
>> If we agree to forbid allocating without page flags, we can just always return
>> an error?
> 
> There are multiple options that we could do here:
> 1. Return an error.

I'd WARN() additionally here.

> 2. Print a warning at runtime and allocate regardless. Additionally use
>      `klint` to lint against usage of that function.
> 3. Use `build_error!`.

Agreed, that'd be even better - Will give that a shot.

> 
> I think that 1 could be very unexpected and hard to debug. 2 is better,
> but at the moment `klint` is not required to run, so we would have to
> change that. 3 is my preferred option, but I am not 100% confident that
> it will work.
> 
> So maybe give option 3 a try and if that doesn't work, we can try to
> come up with a different solution.
> 

Re: [PATCH WIP 3/8] rust: alloc: implement AllocatorWithFlags trait

[Miguel Ojeda]

On Wed, May 1, 2024 at 2:50 PM Danilo Krummrich <dakr@redhat.com> wrote:
>
> How do we scale (as in ensure) this once Rust code becomes more common in
> subsystems?

It is easy to detect, so it is a matter of having the check in enough
bots/CIs/subsystems so that people complain early. I do so for our
main branches, next and so on.

> Is it possible for rustfmt to change rules unexpectedly? If so, how would we
> deal with that?

Like Benno said, not intentionally, as far as I understand. The tool
could have bugs, though, and there are some limitations:

    https://github.com/rust-lang/rustfmt?tab=readme-ov-file#limitations

So we may need to deal with a bug or similar at some point.

In any case, my goal is to have the kernel compiled in upstream Rust's
CI if possible eventually, which ideally would prevent this from
happening to begin with.

Cheers,
Miguel

Re: [PATCH WIP 3/8] rust: alloc: implement AllocatorWithFlags trait

[Gary Guo]

On Wed, 1 May 2024 14:50:27 +0200
Danilo Krummrich <dakr@redhat.com> wrote:

> >   
> > > +
> > > +    fn default_allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
> > > +        unsafe { self.realloc_flags(ptr::null_mut(), 0, layout, GFP_KERNEL) }
> > > +    }  
> > 
> > Why do we want to have these functions? I think that it would make sense
> > to forbid all other ways of allocating memory. So only allow allocation
> > where the flags are explicitly passed.  
> 
> I agree that we should forbid allocating memory with implicit default page
> flags.
> 
> I added them, since I think we'll always need some implementation of the
> Allocator trait. And this just was the obvious generic implementation.

Ignoring Benno's point about enforcing explicit allocation, these
functions still shouldn't exist. Given `Allocator` is a super trait,
all implementors should just implement their default allocation
behaviour in their `Allocator` implementation rather than calling into
the `default_allocate` function.

- Gary

Re: [PATCH WIP 3/8] rust: alloc: implement AllocatorWithFlags trait

[Danilo Krummrich]

On 5/3/24 17:27, Gary Guo wrote:
> On Wed, 1 May 2024 14:50:27 +0200
> Danilo Krummrich <dakr@redhat.com> wrote:
> 
>>>    
>>>> +
>>>> +    fn default_allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
>>>> +        unsafe { self.realloc_flags(ptr::null_mut(), 0, layout, GFP_KERNEL) }
>>>> +    }
>>>
>>> Why do we want to have these functions? I think that it would make sense
>>> to forbid all other ways of allocating memory. So only allow allocation
>>> where the flags are explicitly passed.
>>
>> I agree that we should forbid allocating memory with implicit default page
>> flags.
>>
>> I added them, since I think we'll always need some implementation of the
>> Allocator trait. And this just was the obvious generic implementation.
> 
> Ignoring Benno's point about enforcing explicit allocation, these
> functions still shouldn't exist. Given `Allocator` is a super trait,
> all implementors should just implement their default allocation
> behaviour in their `Allocator` implementation rather than calling into
> the `default_allocate` function.

Well, most Allocator's default allocation behavior is likely to be the same,
which is calling *their* realloc_flags() implementation with a NULL pointer
and GFP_KERNEL for allocate() and with an additional __GFP_ZERO for
allocate_zeroed(), hence the corresponding default helpers.

However, it doesn't really save a lot of boilerplate, hence I agree we could
just omit those.

> 
> - Gary
> 

[PATCH WIP 4/8] rust: alloc: separate krealloc_aligned()

[Danilo Krummrich]

Separate krealloc_aligned() into krealloc() and aligned_size().

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/kernel/alloc/allocator.rs | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/rust/kernel/alloc/allocator.rs b/rust/kernel/alloc/allocator.rs
index ff88bce04fd4..236100649ae1 100644
--- a/rust/kernel/alloc/allocator.rs
+++ b/rust/kernel/alloc/allocator.rs
@@ -10,13 +10,7 @@
 
 struct KernelAllocator;
 
-/// Calls `krealloc` with a proper size to alloc a new object aligned to `new_layout`'s alignment.
-///
-/// # Safety
-///
-/// - `ptr` can be either null or a pointer which has been allocated by this allocator.
-/// - `new_layout` must have a non-zero size.
-pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: Flags) -> *mut u8 {
+pub(crate) fn aligned_size(new_layout: Layout) -> usize {
     // Customized layouts from `Layout::from_size_align()` can have size < align, so pad first.
     let layout = new_layout.pad_to_align();
 
@@ -32,6 +26,16 @@ pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F
         size = size.next_power_of_two();
     }
 
+    size
+}
+
+/// Calls `krealloc` with a proper size to alloc a new object aligned to `new_layout`'s alignment.
+///
+/// # Safety
+///
+/// - `ptr` can be either null or a pointer which has been allocated by this allocator.
+/// - `size` must be non-zero.
+pub(crate) unsafe fn krealloc(ptr: *mut u8, size: usize, flags: Flags) -> *mut u8 {
     // SAFETY:
     // - `ptr` is either null or a pointer returned from a previous `k{re}alloc()` by the
     //   function safety requirement.
@@ -40,6 +44,16 @@ pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F
     unsafe { bindings::krealloc(ptr as *const core::ffi::c_void, size, flags.0) as *mut u8 }
 }
 
+/// Calls `krealloc` with a proper size to alloc a new object aligned to `new_layout`'s alignment.
+///
+/// # Safety
+///
+/// - `ptr` can be either null or a pointer which has been allocated by this allocator.
+/// - `new_layout` must have a non-zero size.
+pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: Flags) -> *mut u8 {
+    unsafe { krealloc(ptr, aligned_size(new_layout), flags) }
+}
+
 unsafe impl GlobalAlloc for KernelAllocator {
     unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
         // SAFETY: `ptr::null_mut()` is null and `layout` has a non-zero size by the function safety
-- 
2.44.0

[PATCH WIP 5/8] rust: alloc: implement AllocatorWithFlags for KernelAllocator

[Danilo Krummrich]

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/kernel/alloc.rs           |  2 +-
 rust/kernel/alloc/allocator.rs | 38 ++++++++++++++++++++++++++++++++--
 2 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
index ec45ba8f77ce..12c6c4e423f5 100644
--- a/rust/kernel/alloc.rs
+++ b/rust/kernel/alloc.rs
@@ -4,7 +4,7 @@
 
 #[cfg(not(test))]
 #[cfg(not(testlib))]
-mod allocator;
+pub mod allocator;
 pub mod box_ext;
 pub mod vec_ext;
 
diff --git a/rust/kernel/alloc/allocator.rs b/rust/kernel/alloc/allocator.rs
index 236100649ae1..173347ff018a 100644
--- a/rust/kernel/alloc/allocator.rs
+++ b/rust/kernel/alloc/allocator.rs
@@ -3,12 +3,14 @@
 //! Allocator support.
 
 use super::{flags::*, Flags};
-use core::alloc::{GlobalAlloc, Layout};
+use core::alloc::{GlobalAlloc, Layout, Allocator, AllocError};
 use core::ptr;
+use core::ptr::NonNull;
 
 use crate::bindings;
+use crate::alloc::AllocatorWithFlags;
 
-struct KernelAllocator;
+pub struct KernelAllocator;
 
 pub(crate) fn aligned_size(new_layout: Layout) -> usize {
     // Customized layouts from `Layout::from_size_align()` can have size < align, so pad first.
@@ -89,6 +91,38 @@ unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 {
     }
 }
 
+unsafe impl Allocator for KernelAllocator {
+    fn allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
+        self.default_allocate(layout)
+    }
+
+    fn allocate_zeroed(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
+        self.default_allocate_zeroed(layout)
+    }
+
+    unsafe fn deallocate(&self, ptr: NonNull<u8>, layout: Layout) {
+        unsafe { self.dealloc(ptr.as_ptr(), layout) }
+    }
+}
+
+unsafe impl AllocatorWithFlags for KernelAllocator {
+    unsafe fn alloc_flags(&self, layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError>
+    {
+        unsafe { self.realloc_flags(ptr::null_mut(), 0, layout, flags) }
+    }
+
+    unsafe fn realloc_flags(&self, ptr: *mut u8, _old_size: usize, layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError>
+    {
+        let size = aligned_size(layout);
+
+        unsafe {
+            let raw_ptr = krealloc(ptr, size, flags);
+            let ptr = NonNull::new(raw_ptr).ok_or(AllocError)?;
+            Ok(NonNull::slice_from_raw_parts(ptr, size))
+        }
+    }
+}
+
 #[global_allocator]
 static ALLOCATOR: KernelAllocator = KernelAllocator;
 
-- 
2.44.0

Re: [PATCH WIP 5/8] rust: alloc: implement AllocatorWithFlags for KernelAllocator

[Benno Lossin]

On 29.04.24 22:11, Danilo Krummrich wrote:
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
>  rust/kernel/alloc.rs           |  2 +-
>  rust/kernel/alloc/allocator.rs | 38 ++++++++++++++++++++++++++++++++--
>  2 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> index ec45ba8f77ce..12c6c4e423f5 100644
> --- a/rust/kernel/alloc.rs
> +++ b/rust/kernel/alloc.rs
> @@ -4,7 +4,7 @@
> 
>  #[cfg(not(test))]
>  #[cfg(not(testlib))]
> -mod allocator;
> +pub mod allocator;
>  pub mod box_ext;
>  pub mod vec_ext;
> 
> diff --git a/rust/kernel/alloc/allocator.rs b/rust/kernel/alloc/allocator.rs
> index 236100649ae1..173347ff018a 100644
> --- a/rust/kernel/alloc/allocator.rs
> +++ b/rust/kernel/alloc/allocator.rs
> @@ -3,12 +3,14 @@
>  //! Allocator support.
> 
>  use super::{flags::*, Flags};
> -use core::alloc::{GlobalAlloc, Layout};
> +use core::alloc::{GlobalAlloc, Layout, Allocator, AllocError};
>  use core::ptr;
> +use core::ptr::NonNull;
> 
>  use crate::bindings;
> +use crate::alloc::AllocatorWithFlags;
> 
> -struct KernelAllocator;
> +pub struct KernelAllocator;
> 
>  pub(crate) fn aligned_size(new_layout: Layout) -> usize {
>      // Customized layouts from `Layout::from_size_align()` can have size < align, so pad first.
> @@ -89,6 +91,38 @@ unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 {
>      }
>  }
> 
> +unsafe impl Allocator for KernelAllocator {
> +    fn allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
> +        self.default_allocate(layout)
> +    }
> +
> +    fn allocate_zeroed(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
> +        self.default_allocate_zeroed(layout)
> +    }
> +
> +    unsafe fn deallocate(&self, ptr: NonNull<u8>, layout: Layout) {
> +        unsafe { self.dealloc(ptr.as_ptr(), layout) }
> +    }
> +}
> +
> +unsafe impl AllocatorWithFlags for KernelAllocator {
> +    unsafe fn alloc_flags(&self, layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError>
> +    {
> +        unsafe { self.realloc_flags(ptr::null_mut(), 0, layout, flags) }
> +    }
> +
> +    unsafe fn realloc_flags(&self, ptr: *mut u8, _old_size: usize, layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError>
> +    {
> +        let size = aligned_size(layout);
> +
> +        unsafe {
> +            let raw_ptr = krealloc(ptr, size, flags);

I looked at the safety requirements of `krealloc`, it needs that
`size` is non-zero. But who guarantees that? I could call
`KernelAllocator::allocate()` with a zero-sized layout, since it is a
safe function.

-- 
Cheers,
Benno

> +            let ptr = NonNull::new(raw_ptr).ok_or(AllocError)?;
> +            Ok(NonNull::slice_from_raw_parts(ptr, size))
> +        }
> +    }
> +}
> +
>  #[global_allocator]
>  static ALLOCATOR: KernelAllocator = KernelAllocator;
> 
> --
> 2.44.0
> 

Re: [PATCH WIP 5/8] rust: alloc: implement AllocatorWithFlags for KernelAllocator

[Danilo Krummrich]

On Wed, May 01, 2024 at 08:44:46AM +0000, Benno Lossin wrote:
> On 29.04.24 22:11, Danilo Krummrich wrote:
> > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > ---
> >  rust/kernel/alloc.rs           |  2 +-
> >  rust/kernel/alloc/allocator.rs | 38 ++++++++++++++++++++++++++++++++--
> >  2 files changed, 37 insertions(+), 3 deletions(-)
> > 
> > diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> > index ec45ba8f77ce..12c6c4e423f5 100644
> > --- a/rust/kernel/alloc.rs
> > +++ b/rust/kernel/alloc.rs
> > @@ -4,7 +4,7 @@
> > 
> >  #[cfg(not(test))]
> >  #[cfg(not(testlib))]
> > -mod allocator;
> > +pub mod allocator;
> >  pub mod box_ext;
> >  pub mod vec_ext;
> > 
> > diff --git a/rust/kernel/alloc/allocator.rs b/rust/kernel/alloc/allocator.rs
> > index 236100649ae1..173347ff018a 100644
> > --- a/rust/kernel/alloc/allocator.rs
> > +++ b/rust/kernel/alloc/allocator.rs
> > @@ -3,12 +3,14 @@
> >  //! Allocator support.
> > 
> >  use super::{flags::*, Flags};
> > -use core::alloc::{GlobalAlloc, Layout};
> > +use core::alloc::{GlobalAlloc, Layout, Allocator, AllocError};
> >  use core::ptr;
> > +use core::ptr::NonNull;
> > 
> >  use crate::bindings;
> > +use crate::alloc::AllocatorWithFlags;
> > 
> > -struct KernelAllocator;
> > +pub struct KernelAllocator;
> > 
> >  pub(crate) fn aligned_size(new_layout: Layout) -> usize {
> >      // Customized layouts from `Layout::from_size_align()` can have size < align, so pad first.
> > @@ -89,6 +91,38 @@ unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 {
> >      }
> >  }
> > 
> > +unsafe impl Allocator for KernelAllocator {
> > +    fn allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
> > +        self.default_allocate(layout)
> > +    }
> > +
> > +    fn allocate_zeroed(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
> > +        self.default_allocate_zeroed(layout)
> > +    }
> > +
> > +    unsafe fn deallocate(&self, ptr: NonNull<u8>, layout: Layout) {
> > +        unsafe { self.dealloc(ptr.as_ptr(), layout) }
> > +    }
> > +}
> > +
> > +unsafe impl AllocatorWithFlags for KernelAllocator {
> > +    unsafe fn alloc_flags(&self, layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError>
> > +    {
> > +        unsafe { self.realloc_flags(ptr::null_mut(), 0, layout, flags) }
> > +    }
> > +
> > +    unsafe fn realloc_flags(&self, ptr: *mut u8, _old_size: usize, layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError>
> > +    {
> > +        let size = aligned_size(layout);
> > +
> > +        unsafe {
> > +            let raw_ptr = krealloc(ptr, size, flags);
> 
> I looked at the safety requirements of `krealloc`, it needs that
> `size` is non-zero. But who guarantees that? I could call
> `KernelAllocator::allocate()` with a zero-sized layout, since it is a
> safe function.

Indeed, as mentioned in the other thread, realloc_flags() shouldn't be unsafe
IMHO and instead should fulfill the safety requirement of krealloc().

Gonna fix that.

> 
> -- 
> Cheers,
> Benno
> 
> > +            let ptr = NonNull::new(raw_ptr).ok_or(AllocError)?;
> > +            Ok(NonNull::slice_from_raw_parts(ptr, size))
> > +        }
> > +    }
> > +}
> > +
> >  #[global_allocator]
> >  static ALLOCATOR: KernelAllocator = KernelAllocator;
> > 
> > --
> > 2.44.0
> > 
> 

[PATCH WIP 6/8] rust: alloc: implement BoxExtAlloc

[Danilo Krummrich]

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/kernel/alloc/box_ext.rs | 40 +++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/alloc/box_ext.rs b/rust/kernel/alloc/box_ext.rs
index 76653d6f4257..74d6bb24e965 100644
--- a/rust/kernel/alloc/box_ext.rs
+++ b/rust/kernel/alloc/box_ext.rs
@@ -2,7 +2,7 @@
 
 //! Extensions to [`Box`] for fallible allocations.
 
-use super::Flags;
+use super::{AllocatorWithFlags, Flags};
 use alloc::boxed::Box;
 use core::alloc::AllocError;
 use core::mem::MaybeUninit;
@@ -21,6 +21,19 @@ pub trait BoxExt<T>: Sized {
     fn new_uninit(flags: Flags) -> Result<Box<MaybeUninit<T>>, AllocError>;
 }
 
+/// Extensions to [`Box`].
+pub trait BoxExtAlloc<T, A: AllocatorWithFlags>: Sized {
+    /// Allocates a new box using a custom allocator.
+    ///
+    /// The allocation may fail, in which case an error is returned.
+    fn new_in(x: T, alloc: A, flags: Flags) -> Result<Self, AllocError>;
+
+    /// Allocates a new uninitialised box using a custom allocator..
+    ///
+    /// The allocation may fail, in which case an error is returned.
+    fn new_uninit_in(alloc: A, flags: Flags) -> Result<Box<MaybeUninit<T>>, AllocError>;
+}
+
 impl<T> BoxExt<T> for Box<T> {
     fn new(x: T, flags: Flags) -> Result<Self, AllocError> {
         let b = <Self as BoxExt<_>>::new_uninit(flags)?;
@@ -51,6 +64,31 @@ fn new_uninit(flags: Flags) -> Result<Box<MaybeUninit<T>>, AllocError> {
             ptr.cast::<MaybeUninit<T>>()
         };
 
+        // SAFETY: For non-zero-sized types, we allocate above using the global allocator. For
+        // zero-sized types, we use `NonNull::dangling`.
+        Ok(unsafe { Box::from_raw(ptr) })
+    }
+
+}
+
+impl<T, A> BoxExtAlloc<T, A> for Box<T> where A: AllocatorWithFlags {
+    fn new_in(x: T, alloc: A, flags: Flags) -> Result<Self, AllocError> {
+        let b = <Self as BoxExtAlloc<_, A>>::new_uninit_in(alloc, flags)?;
+        Ok(Box::write(b, x))
+    }
+
+    fn new_uninit_in(alloc: A, flags: Flags) -> Result<Box<MaybeUninit<T>>, AllocError> {
+        let ptr = if core::mem::size_of::<MaybeUninit<T>>() == 0 {
+            core::ptr::NonNull::<_>::dangling().as_ptr()
+        } else {
+            let layout = core::alloc::Layout::new::<MaybeUninit<T>>();
+
+            let data = unsafe { alloc.alloc_flags(layout, flags)? };
+            let ptr = data.as_ptr();
+
+            ptr.cast::<MaybeUninit<T>>()
+        };
+
         // SAFETY: For non-zero-sized types, we allocate above using the global allocator. For
         // zero-sized types, we use `NonNull::dangling`.
         Ok(unsafe { Box::from_raw(ptr) })
-- 
2.44.0

Re: [PATCH WIP 6/8] rust: alloc: implement BoxExtAlloc

[Benno Lossin]

On 29.04.24 22:11, Danilo Krummrich wrote:
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
>  rust/kernel/alloc/box_ext.rs | 40 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/alloc/box_ext.rs b/rust/kernel/alloc/box_ext.rs
> index 76653d6f4257..74d6bb24e965 100644
> --- a/rust/kernel/alloc/box_ext.rs
> +++ b/rust/kernel/alloc/box_ext.rs
> @@ -2,7 +2,7 @@
> 
>  //! Extensions to [`Box`] for fallible allocations.
> 
> -use super::Flags;
> +use super::{AllocatorWithFlags, Flags};
>  use alloc::boxed::Box;
>  use core::alloc::AllocError;
>  use core::mem::MaybeUninit;
> @@ -21,6 +21,19 @@ pub trait BoxExt<T>: Sized {
>      fn new_uninit(flags: Flags) -> Result<Box<MaybeUninit<T>>, AllocError>;
>  }
> 
> +/// Extensions to [`Box`].
> +pub trait BoxExtAlloc<T, A: AllocatorWithFlags>: Sized {

I think it would be better to put all of this stuff into the `BoxExt`
trait. Otherwise we will end up with even more extension traits...
Ditto for `VecExtAlloc`.

-- 
Cheers,
Benno

> +    /// Allocates a new box using a custom allocator.
> +    ///
> +    /// The allocation may fail, in which case an error is returned.
> +    fn new_in(x: T, alloc: A, flags: Flags) -> Result<Self, AllocError>;
> +
> +    /// Allocates a new uninitialised box using a custom allocator..
> +    ///
> +    /// The allocation may fail, in which case an error is returned.
> +    fn new_uninit_in(alloc: A, flags: Flags) -> Result<Box<MaybeUninit<T>>, AllocError>;
> +}
> +

Re: [PATCH WIP 6/8] rust: alloc: implement BoxExtAlloc

[Danilo Krummrich]

On Wed, May 01, 2024 at 08:53:51AM +0000, Benno Lossin wrote:
> On 29.04.24 22:11, Danilo Krummrich wrote:
> > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > ---
> >  rust/kernel/alloc/box_ext.rs | 40 +++++++++++++++++++++++++++++++++++-
> >  1 file changed, 39 insertions(+), 1 deletion(-)
> > 
> > diff --git a/rust/kernel/alloc/box_ext.rs b/rust/kernel/alloc/box_ext.rs
> > index 76653d6f4257..74d6bb24e965 100644
> > --- a/rust/kernel/alloc/box_ext.rs
> > +++ b/rust/kernel/alloc/box_ext.rs
> > @@ -2,7 +2,7 @@
> > 
> >  //! Extensions to [`Box`] for fallible allocations.
> > 
> > -use super::Flags;
> > +use super::{AllocatorWithFlags, Flags};
> >  use alloc::boxed::Box;
> >  use core::alloc::AllocError;
> >  use core::mem::MaybeUninit;
> > @@ -21,6 +21,19 @@ pub trait BoxExt<T>: Sized {
> >      fn new_uninit(flags: Flags) -> Result<Box<MaybeUninit<T>>, AllocError>;
> >  }
> > 
> > +/// Extensions to [`Box`].
> > +pub trait BoxExtAlloc<T, A: AllocatorWithFlags>: Sized {
> 
> I think it would be better to put all of this stuff into the `BoxExt`
> trait. Otherwise we will end up with even more extension traits...

That was my inital idea as well, but it wasn't quite obvious for me how to make
this happen.

I think for BoxExt<T>, it's rather easy, since we don't need an implementation
for Box<T, A>. What do you think about this [1]?

For VecExt<T>, I'm not quite sure how this can work though, since we'd need to
implement VecExt<T> for both Vec<T> and Vec<T, A>.

For instance, VecExt<T>::reserve() needs to be implemented for both Vec<T> and
Vec<T, A>, but VecExt<T>::with_capacity() can only be implemented for Vec<T> and
VecExt<T>::with_capacity_in() can only be implemented for Vec<T, A>. But we have
to implement all trait functions for both types.

Do I miss something here?

[1] https://gitlab.freedesktop.org/drm/nova/-/snippets/7784

> Ditto for `VecExtAlloc`.
> 
> -- 
> Cheers,
> Benno
> 
> > +    /// Allocates a new box using a custom allocator.
> > +    ///
> > +    /// The allocation may fail, in which case an error is returned.
> > +    fn new_in(x: T, alloc: A, flags: Flags) -> Result<Self, AllocError>;
> > +
> > +    /// Allocates a new uninitialised box using a custom allocator..
> > +    ///
> > +    /// The allocation may fail, in which case an error is returned.
> > +    fn new_uninit_in(alloc: A, flags: Flags) -> Result<Box<MaybeUninit<T>>, AllocError>;
> > +}
> > +
> 
> 

Re: [PATCH WIP 6/8] rust: alloc: implement BoxExtAlloc

[Benno Lossin]

On 01.05.24 15:06, Danilo Krummrich wrote:
> On Wed, May 01, 2024 at 08:53:51AM +0000, Benno Lossin wrote:
>> On 29.04.24 22:11, Danilo Krummrich wrote:
>>> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
>>> ---
>>>   rust/kernel/alloc/box_ext.rs | 40 +++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 39 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/rust/kernel/alloc/box_ext.rs b/rust/kernel/alloc/box_ext.rs
>>> index 76653d6f4257..74d6bb24e965 100644
>>> --- a/rust/kernel/alloc/box_ext.rs
>>> +++ b/rust/kernel/alloc/box_ext.rs
>>> @@ -2,7 +2,7 @@
>>>
>>>   //! Extensions to [`Box`] for fallible allocations.
>>>
>>> -use super::Flags;
>>> +use super::{AllocatorWithFlags, Flags};
>>>   use alloc::boxed::Box;
>>>   use core::alloc::AllocError;
>>>   use core::mem::MaybeUninit;
>>> @@ -21,6 +21,19 @@ pub trait BoxExt<T>: Sized {
>>>       fn new_uninit(flags: Flags) -> Result<Box<MaybeUninit<T>>, AllocError>;
>>>   }
>>>
>>> +/// Extensions to [`Box`].
>>> +pub trait BoxExtAlloc<T, A: AllocatorWithFlags>: Sized {
>>
>> I think it would be better to put all of this stuff into the `BoxExt`
>> trait. Otherwise we will end up with even more extension traits...
> 
> That was my inital idea as well, but it wasn't quite obvious for me how to make
> this happen.
> 
> I think for BoxExt<T>, it's rather easy, since we don't need an implementation
> for Box<T, A>. What do you think about this [1]?

In the `.._in` functions you're not passing the allocator to the `Box`.
This will lead to problems, since it will use the global allocator to
deallocate the value when you drop the `Box`.

> For VecExt<T>, I'm not quite sure how this can work though, since we'd need to
> implement VecExt<T> for both Vec<T> and Vec<T, A>.
> 
> For instance, VecExt<T>::reserve() needs to be implemented for both Vec<T> and
> Vec<T, A>, but VecExt<T>::with_capacity() can only be implemented for Vec<T> and
> VecExt<T>::with_capacity_in() can only be implemented for Vec<T, A>. But we have
> to implement all trait functions for both types.
> 
> Do I miss something here?

You're right, we need two traits for that...

-- 
Cheers,
Benno

> [1] https://gitlab.freedesktop.org/drm/nova/-/snippets/7784

Re: [PATCH WIP 6/8] rust: alloc: implement BoxExtAlloc

[Danilo Krummrich]

On Wed, May 01, 2024 at 03:46:31PM +0000, Benno Lossin wrote:
> On 01.05.24 15:06, Danilo Krummrich wrote:
> > On Wed, May 01, 2024 at 08:53:51AM +0000, Benno Lossin wrote:
> >> On 29.04.24 22:11, Danilo Krummrich wrote:
> >>> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> >>> ---
> >>>   rust/kernel/alloc/box_ext.rs | 40 +++++++++++++++++++++++++++++++++++-
> >>>   1 file changed, 39 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/rust/kernel/alloc/box_ext.rs b/rust/kernel/alloc/box_ext.rs
> >>> index 76653d6f4257..74d6bb24e965 100644
> >>> --- a/rust/kernel/alloc/box_ext.rs
> >>> +++ b/rust/kernel/alloc/box_ext.rs
> >>> @@ -2,7 +2,7 @@
> >>>
> >>>   //! Extensions to [`Box`] for fallible allocations.
> >>>
> >>> -use super::Flags;
> >>> +use super::{AllocatorWithFlags, Flags};
> >>>   use alloc::boxed::Box;
> >>>   use core::alloc::AllocError;
> >>>   use core::mem::MaybeUninit;
> >>> @@ -21,6 +21,19 @@ pub trait BoxExt<T>: Sized {
> >>>       fn new_uninit(flags: Flags) -> Result<Box<MaybeUninit<T>>, AllocError>;
> >>>   }
> >>>
> >>> +/// Extensions to [`Box`].
> >>> +pub trait BoxExtAlloc<T, A: AllocatorWithFlags>: Sized {
> >>
> >> I think it would be better to put all of this stuff into the `BoxExt`
> >> trait. Otherwise we will end up with even more extension traits...
> > 
> > That was my inital idea as well, but it wasn't quite obvious for me how to make
> > this happen.
> > 
> > I think for BoxExt<T>, it's rather easy, since we don't need an implementation
> > for Box<T, A>. What do you think about this [1]?
> 
> In the `.._in` functions you're not passing the allocator to the `Box`.
> This will lead to problems, since it will use the global allocator to
> deallocate the value when you drop the `Box`.

Indeed, good catch.

> 
> > For VecExt<T>, I'm not quite sure how this can work though, since we'd need to
> > implement VecExt<T> for both Vec<T> and Vec<T, A>.
> > 
> > For instance, VecExt<T>::reserve() needs to be implemented for both Vec<T> and
> > Vec<T, A>, but VecExt<T>::with_capacity() can only be implemented for Vec<T> and
> > VecExt<T>::with_capacity_in() can only be implemented for Vec<T, A>. But we have
> > to implement all trait functions for both types.
> > 
> > Do I miss something here?
> 
> You're right, we need two traits for that...

Ok, so I guess we keep BoxExt<T> for everthing and just separate VecExt<T> and
VecExtAlloc<T, A>.

- Danilo

> 
> -- 
> Cheers,
> Benno
> 
> > [1] https://gitlab.freedesktop.org/drm/nova/-/snippets/7784
> 

[PATCH WIP 7/8] rust: alloc: implement VecExtAlloc

[Danilo Krummrich]

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/kernel/alloc/vec_ext.rs | 85 +++++++++++++++++++++++++++++++++++-
 1 file changed, 84 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/alloc/vec_ext.rs b/rust/kernel/alloc/vec_ext.rs
index 1dc991b44be7..3ed33fdffe70 100644
--- a/rust/kernel/alloc/vec_ext.rs
+++ b/rust/kernel/alloc/vec_ext.rs
@@ -2,9 +2,10 @@
 
 //! Extensions to [`Vec`] for fallible allocations.
 
-use super::Flags;
+use super::{AllocatorWithFlags,Flags};
 use alloc::vec::Vec;
 use core::alloc::AllocError;
+use core::ptr;
 use core::result::Result;
 
 /// Extensions to [`Vec`].
@@ -76,6 +77,76 @@ fn extend_from_slice(&mut self, other: &[T], flags: Flags) -> Result<(), AllocEr
     fn reserve(&mut self, additional: usize, flags: Flags) -> Result<(), AllocError>;
 }
 
+pub trait VecExtAlloc<T, A: AllocatorWithFlags>: Sized {
+    fn with_capacity_in(capacity: usize, alloc: A, flags: Flags) -> Result<Self, AllocError>;
+    fn extend_from_slice(&mut self, other: &[T], flags: Flags) -> Result<(), AllocError>
+    where
+        T: Clone;
+    fn reserve(&mut self, additional: usize, flags: Flags) -> Result<(), AllocError>;
+}
+
+impl<T, A> VecExtAlloc<T, A> for Vec<T, A> where A: AllocatorWithFlags {
+    fn with_capacity_in(capacity: usize, alloc: A, flags: Flags) -> Result<Self, AllocError> {
+        let mut v = Vec::new_in(alloc);
+        <Self as VecExtAlloc<_, A>>::reserve(&mut v, capacity, flags)?;
+        Ok(v)
+    }
+
+    fn extend_from_slice(&mut self, other: &[T], flags: Flags) -> Result<(), AllocError>
+    where
+        T: Clone,
+    {
+        <Self as VecExtAlloc<_, A>>::reserve(self, other.len(), flags)?;
+        for (slot, item) in core::iter::zip(self.spare_capacity_mut(), other) {
+            slot.write(item.clone());
+        }
+
+        // SAFETY: We just initialised the `other.len()` spare entries, so it is safe to increase
+        // the length by the same amount. We also know that the new length is <= capacity because
+        // of the previous call to `reserve` above.
+        unsafe { self.set_len(self.len() + other.len()) };
+        Ok(())
+    }
+
+    fn reserve(&mut self, additional: usize, flags: Flags) -> Result<(), AllocError> {
+        let len = self.len();
+        let cap = self.capacity();
+
+        if cap - len >= additional {
+            return Ok(());
+        }
+
+        if core::mem::size_of::<T>() == 0 {
+            // The capacity is already `usize::MAX` for SZTs, we can't go higher.
+            return Err(AllocError);
+        }
+
+        // We know cap is <= `isize::MAX` because `Layout::array` fails if the resulting byte size
+        // is greater than `isize::MAX`. So the multiplication by two won't overflow.
+        let new_cap = core::cmp::max(cap * 2, len.checked_add(additional).ok_or(AllocError)?);
+        let layout = core::alloc::Layout::array::<T>(new_cap).map_err(|_| AllocError)?;
+
+        let (ptr, len, cap, alloc) = destructure_alloc(self);
+
+        // We can't trust ptr to be a NULL pointer if cap is zero, hence explicitly assing a NULL
+        // pointer.
+        let ptr = match cap {
+            0 => ptr::null_mut(),
+            _ => ptr,
+        };
+
+        // SAFETY: `ptr` is valid because it's either NULL or comes from a previous call to
+        // `krealloc_aligned`. We also verified that the type is not a ZST.
+        let data = unsafe { alloc.realloc_flags(ptr.cast(), cap, layout, flags)? };
+        let new_ptr = data.as_ptr();
+
+        // SAFETY: `ptr` has been reallocated with the layout for `new_cap` elements. New cap
+        // is greater than `cap`, so it continues to be >= `len`.
+        unsafe { rebuild_alloc(self, alloc, new_ptr.cast::<T>(), len, new_cap) };
+        Ok(())
+    }
+}
+
 impl<T> VecExt<T> for Vec<T> {
     fn with_capacity(capacity: usize, flags: Flags) -> Result<Self, AllocError> {
         let mut v = Vec::new();
@@ -175,3 +246,15 @@ unsafe fn rebuild<T>(v: &mut Vec<T>, ptr: *mut T, len: usize, cap: usize) {
     let mut tmp = unsafe { Vec::from_raw_parts(ptr, len, cap) };
     core::mem::swap(&mut tmp, v);
 }
+
+fn destructure_alloc<T, A: AllocatorWithFlags>(v: &mut Vec<T, A>) -> (*mut T, usize, usize, A) {
+    let mut tmp: Vec<T, A> = unsafe { core::mem::transmute_copy(v) };
+    core::mem::swap(&mut tmp, v);
+    tmp.into_raw_parts_with_alloc()
+}
+
+unsafe fn rebuild_alloc<T, A: AllocatorWithFlags>(v: &mut Vec<T, A>, alloc: A, ptr: *mut T, len: usize, cap: usize) {
+    // SAFETY: The safety requirements from this function satisfy those of `from_raw_parts`.
+    let mut tmp = unsafe { Vec::from_raw_parts_in(ptr, len, cap, alloc) };
+    core::mem::swap(&mut tmp, v);
+}
-- 
2.44.0

[PATCH WIP 8/8] rust: alloc: implement vmalloc allocator

[Danilo Krummrich]

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 rust/kernel/alloc/allocator.rs | 37 ++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/rust/kernel/alloc/allocator.rs b/rust/kernel/alloc/allocator.rs
index 173347ff018a..d3d0bbc41ab2 100644
--- a/rust/kernel/alloc/allocator.rs
+++ b/rust/kernel/alloc/allocator.rs
@@ -11,6 +11,7 @@
 use crate::alloc::AllocatorWithFlags;
 
 pub struct KernelAllocator;
+pub struct VmAllocator;
 
 pub(crate) fn aligned_size(new_layout: Layout) -> usize {
     // Customized layouts from `Layout::from_size_align()` can have size < align, so pad first.
@@ -123,6 +124,42 @@ unsafe fn realloc_flags(&self, ptr: *mut u8, _old_size: usize, layout: Layout, f
     }
 }
 
+unsafe impl Allocator for VmAllocator {
+    fn allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
+        self.default_allocate(layout)
+    }
+
+    fn allocate_zeroed(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
+        self.default_allocate_zeroed(layout)
+    }
+
+    unsafe fn deallocate(&self, ptr: NonNull<u8>, _layout: Layout) {
+        unsafe { bindings::vfree(ptr.as_ptr() as *const core::ffi::c_void) }
+    }
+}
+
+unsafe impl AllocatorWithFlags for VmAllocator {
+    unsafe fn alloc_flags(&self, layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError>
+    {
+        unsafe { self.realloc_flags(ptr::null_mut(), 0, layout, flags) }
+    }
+
+    unsafe fn realloc_flags(&self, src: *mut u8, old_size: usize, layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError>
+    {
+        let size = aligned_size(layout);
+
+        unsafe {
+            let dst = bindings::__vmalloc(size as u64, flags.0);
+            if !src.is_null() {
+                core::ptr::copy_nonoverlapping(src, dst as *mut u8, old_size);
+                bindings::vfree(src as *const core::ffi::c_void);
+            }
+            let ptr = NonNull::new(dst as *mut u8).ok_or(AllocError)?;
+            Ok(NonNull::slice_from_raw_parts(ptr, size))
+        }
+    }
+}
+
 #[global_allocator]
 static ALLOCATOR: KernelAllocator = KernelAllocator;
 
-- 
2.44.0

Re: [PATCH WIP 0/8] Draft: Alternative allocator support

[Miguel Ojeda]

On Mon, Apr 29, 2024 at 10:12 PM Danilo Krummrich <dakr@redhat.com> wrote:
>
> Please let me know what you think.

Thanks Danilo for implementing this.

I think we should explore options that avoid going back to
`allocator_api` if possible (i.e. like for any other unstable
feature).

In particular, `allocator_api` has been unstable for a while and it is
not completely clear to me what the path forward there will be
(especially with all the discussions around that particular unstable
feature and previous efforts to make progress there, e.g. whether it
would be fine for them to add more `try_*` methods and how to name
them and the documentation concerns, and whether it may need to wait
for other features like keyword generics / how exactly to avoid
duplication of methods, and so on). It is also unclear if they could
give us support on their side for flags per call site and so on.

So if we can find a way that does not use it and that does not have a
performance penalty (or a big ergonomics penalty), then I think it is
worth avoiding it.

In fact, I would say what we really need is the flexibility to
implement our own allocation APIs from scratch, including types like
`Box`, `Vec`, `Arc`, etc. suited for particular kernel use cases, and
giving those the methods that make sense in each case (whether or not
wrapping/newtyping existing `alloc` types), rather than the ability to
"just" customize the allocator for existing collections. To be fair,
Rust is already willing to consider solutions for us on these matters,
like they are doing with e.g. `derive(SmartPointer)`, which we really
appreciate.

Cheers,
Miguel

Re: [PATCH WIP 0/8] Draft: Alternative allocator support

[Danilo Krummrich]

On Wed, May 01, 2024 at 11:32:02PM +0200, Miguel Ojeda wrote:
> On Mon, Apr 29, 2024 at 10:12 PM Danilo Krummrich <dakr@redhat.com> wrote:
> >
> > Please let me know what you think.
> 
> Thanks Danilo for implementing this.
> 
> I think we should explore options that avoid going back to
> `allocator_api` if possible (i.e. like for any other unstable
> feature).
> 
> In particular, `allocator_api` has been unstable for a while and it is
> not completely clear to me what the path forward there will be
> (especially with all the discussions around that particular unstable
> feature and previous efforts to make progress there, e.g. whether it
> would be fine for them to add more `try_*` methods and how to name
> them and the documentation concerns, and whether it may need to wait
> for other features like keyword generics / how exactly to avoid
> duplication of methods, and so on). It is also unclear if they could
> give us support on their side for flags per call site and so on.
> 
> So if we can find a way that does not use it and that does not have a
> performance penalty (or a big ergonomics penalty), then I think it is
> worth avoiding it.

Yes, I fully agree. As discussed, I will explore this option, get something
hacked up, post it and then we can see how it compares to the approach of this
series.

- Danilo

> 
> In fact, I would say what we really need is the flexibility to
> implement our own allocation APIs from scratch, including types like
> `Box`, `Vec`, `Arc`, etc. suited for particular kernel use cases, and
> giving those the methods that make sense in each case (whether or not
> wrapping/newtyping existing `alloc` types), rather than the ability to
> "just" customize the allocator for existing collections. To be fair,
> Rust is already willing to consider solutions for us on these matters,
> like they are doing with e.g. `derive(SmartPointer)`, which we really
> appreciate.
> 
> Cheers,
> Miguel
>