Implement `Allocator` for `Kmalloc`, the kernel's default allocator,
typically used for objects smaller than page size.

All memory allocations made with `Kmalloc` end up in `krealloc()`.

It serves as allocator for the subsequently introduced types `KBox` and
`KVec`.

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

diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
index 9932f21b0539..477dbe3c5a2f 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 e32182f91167..78e7d5488843 100644
--- a/rust/kernel/alloc/allocator.rs
+++ b/rust/kernel/alloc/allocator.rs
@@ -1,12 +1,28 @@
 // SPDX-License-Identifier: GPL-2.0
 
 //! Allocator support.
+//!
+//! Documentation for the kernel's memory allocators can found in the "Memory Allocation Guide"
+//! linked below. For instance, this includes the concept of "get free page" (GFP) flags and the
+//! typical application of the different kernel allocators.
+//!
+//! Reference: <https://docs.kernel.org/core-api/memory-allocation.html>
 
 use super::{flags::*, Flags};
 use core::alloc::{GlobalAlloc, Layout};
 use core::ptr;
+use core::ptr::NonNull;
 
-struct Kmalloc;
+use crate::alloc::{AllocError, Allocator};
+use crate::bindings;
+
+/// The contiguous kernel allocator.
+///
+/// `Kmalloc` is typically used for physically contiguous allocations up to page size, but also
+/// supports larger allocations up to `bindings::KMALLOC_MAX_SIZE`, which is hardware specific.
+///
+/// For more details see [self].
+pub struct Kmalloc;
 
 /// Returns a proper size to alloc a new object aligned to `new_layout`'s alignment.
 fn aligned_size(new_layout: Layout) -> usize {
@@ -36,6 +52,60 @@ 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 }
 }
 
+/// # Invariants
+///
+/// One of the following `krealloc`, `vrealloc`, `kvrealloc`.
+struct ReallocFunc(
+    unsafe extern "C" fn(*const core::ffi::c_void, usize, u32) -> *mut core::ffi::c_void,
+);
+
+impl ReallocFunc {
+    // INVARIANT: `krealloc` satisfies the type invariants.
+    const KREALLOC: Self = Self(bindings::krealloc);
+
+    /// # Safety
+    ///
+    /// This method has the same safety requirements as [`Allocator::realloc`].
+    unsafe fn call(
+        &self,
+        ptr: Option<NonNull<u8>>,
+        layout: Layout,
+        flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError> {
+        let size = aligned_size(layout);
+        let ptr = match ptr {
+            Some(ptr) => ptr.as_ptr(),
+            None => ptr::null(),
+        };
+
+        // SAFETY: `ptr` is either NULL or valid by the safety requirements of this function.
+        let raw_ptr = unsafe {
+            // If `size == 0` and `ptr != NULL` the memory behind the pointer is freed.
+            self.0(ptr.cast(), size, flags.0).cast()
+        };
+
+        let ptr = if size == 0 {
+            NonNull::dangling()
+        } else {
+            NonNull::new(raw_ptr).ok_or(AllocError)?
+        };
+
+        Ok(NonNull::slice_from_raw_parts(ptr, size))
+    }
+}
+
+unsafe impl Allocator for Kmalloc {
+    #[inline]
+    unsafe fn realloc(
+        ptr: Option<NonNull<u8>>,
+        layout: Layout,
+        flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError> {
+        // SAFETY: `ReallocFunc::call` has the same safety requirements as `Allocator::realloc`.
+        unsafe { ReallocFunc::KREALLOC.call(ptr, layout, flags) }
+    }
+}
+
 unsafe impl GlobalAlloc for Kmalloc {
     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.46.0

On 16.08.24 02:10, Danilo Krummrich wrote:
> Implement `Allocator` for `Kmalloc`, the kernel's default allocator,
> typically used for objects smaller than page size.
> 
> All memory allocations made with `Kmalloc` end up in `krealloc()`.
> 
> It serves as allocator for the subsequently introduced types `KBox` and
> `KVec`.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/alloc.rs           |  2 +-
>  rust/kernel/alloc/allocator.rs | 72 +++++++++++++++++++++++++++++++++-
>  2 files changed, 72 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> index 9932f21b0539..477dbe3c5a2f 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 e32182f91167..78e7d5488843 100644
> --- a/rust/kernel/alloc/allocator.rs
> +++ b/rust/kernel/alloc/allocator.rs
> @@ -1,12 +1,28 @@
>  // SPDX-License-Identifier: GPL-2.0
> 
>  //! Allocator support.
> +//!
> +//! Documentation for the kernel's memory allocators can found in the "Memory Allocation Guide"
> +//! linked below. For instance, this includes the concept of "get free page" (GFP) flags and the
> +//! typical application of the different kernel allocators.
> +//!
> +//! Reference: <https://docs.kernel.org/core-api/memory-allocation.html>

Thanks, this nice.

> 
>  use super::{flags::*, Flags};
>  use core::alloc::{GlobalAlloc, Layout};
>  use core::ptr;
> +use core::ptr::NonNull;
> 
> -struct Kmalloc;
> +use crate::alloc::{AllocError, Allocator};
> +use crate::bindings;
> +
> +/// The contiguous kernel allocator.
> +///
> +/// `Kmalloc` is typically used for physically contiguous allocations up to page size, but also
> +/// supports larger allocations up to `bindings::KMALLOC_MAX_SIZE`, which is hardware specific.

Does putting a link here work? (I guess we don't yet export the bindings
documentation, so it will probably fail... When we decide to enable it,
we should create an issue to add missing links)

> +///
> +/// For more details see [self].
> +pub struct Kmalloc;
> 
>  /// Returns a proper size to alloc a new object aligned to `new_layout`'s alignment.
>  fn aligned_size(new_layout: Layout) -> usize {
> @@ -36,6 +52,60 @@ 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 }
>  }
> 
> +/// # Invariants
> +///
> +/// One of the following `krealloc`, `vrealloc`, `kvrealloc`.
> +struct ReallocFunc(
> +    unsafe extern "C" fn(*const core::ffi::c_void, usize, u32) -> *mut core::ffi::c_void,
> +);
> +
> +impl ReallocFunc {
> +    // INVARIANT: `krealloc` satisfies the type invariants.
> +    const KREALLOC: Self = Self(bindings::krealloc);
> +
> +    /// # Safety
> +    ///
> +    /// This method has the same safety requirements as [`Allocator::realloc`].
> +    unsafe fn call(
> +        &self,
> +        ptr: Option<NonNull<u8>>,
> +        layout: Layout,
> +        flags: Flags,
> +    ) -> Result<NonNull<[u8]>, AllocError> {
> +        let size = aligned_size(layout);
> +        let ptr = match ptr {
> +            Some(ptr) => ptr.as_ptr(),
> +            None => ptr::null(),
> +        };
> +
> +        // SAFETY: `ptr` is either NULL or valid by the safety requirements of this function.

You need some justification as to why calling the three allowed
functions here.

> +        let raw_ptr = unsafe {
> +            // If `size == 0` and `ptr != NULL` the memory behind the pointer is freed.
> +            self.0(ptr.cast(), size, flags.0).cast()
> +        };
> +
> +        let ptr = if size == 0 {
> +            NonNull::dangling()
> +        } else {
> +            NonNull::new(raw_ptr).ok_or(AllocError)?
> +        };
> +
> +        Ok(NonNull::slice_from_raw_parts(ptr, size))
> +    }
> +}
> +
> +unsafe impl Allocator for Kmalloc {

Missing SAFETY comment.

---
Cheers,
Benno

> +    #[inline]
> +    unsafe fn realloc(
> +        ptr: Option<NonNull<u8>>,
> +        layout: Layout,
> +        flags: Flags,
> +    ) -> Result<NonNull<[u8]>, AllocError> {
> +        // SAFETY: `ReallocFunc::call` has the same safety requirements as `Allocator::realloc`.
> +        unsafe { ReallocFunc::KREALLOC.call(ptr, layout, flags) }
> +    }
> +}
> +
>  unsafe impl GlobalAlloc for Kmalloc {
>      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.46.0
> 

On Thu, Aug 29, 2024 at 06:32:42PM +0000, Benno Lossin wrote:
> On 16.08.24 02:10, Danilo Krummrich wrote:
> > Implement `Allocator` for `Kmalloc`, the kernel's default allocator,
> > typically used for objects smaller than page size.
> > 
> > All memory allocations made with `Kmalloc` end up in `krealloc()`.
> > 
> > It serves as allocator for the subsequently introduced types `KBox` and
> > `KVec`.
> > 
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> >  rust/kernel/alloc.rs           |  2 +-
> >  rust/kernel/alloc/allocator.rs | 72 +++++++++++++++++++++++++++++++++-
> >  2 files changed, 72 insertions(+), 2 deletions(-)
> > 
> > diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> > index 9932f21b0539..477dbe3c5a2f 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 e32182f91167..78e7d5488843 100644
> > --- a/rust/kernel/alloc/allocator.rs
> > +++ b/rust/kernel/alloc/allocator.rs
> > @@ -1,12 +1,28 @@
> >  // SPDX-License-Identifier: GPL-2.0
> > 
> >  //! Allocator support.
> > +//!
> > +//! Documentation for the kernel's memory allocators can found in the "Memory Allocation Guide"
> > +//! linked below. For instance, this includes the concept of "get free page" (GFP) flags and the
> > +//! typical application of the different kernel allocators.
> > +//!
> > +//! Reference: <https://docs.kernel.org/core-api/memory-allocation.html>
> 
> Thanks, this nice.
> 
> > 
> >  use super::{flags::*, Flags};
> >  use core::alloc::{GlobalAlloc, Layout};
> >  use core::ptr;
> > +use core::ptr::NonNull;
> > 
> > -struct Kmalloc;
> > +use crate::alloc::{AllocError, Allocator};
> > +use crate::bindings;
> > +
> > +/// The contiguous kernel allocator.
> > +///
> > +/// `Kmalloc` is typically used for physically contiguous allocations up to page size, but also
> > +/// supports larger allocations up to `bindings::KMALLOC_MAX_SIZE`, which is hardware specific.
> 
> Does putting a link here work? (I guess we don't yet export the bindings
> documentation, so it will probably fail... When we decide to enable it,
> we should create an issue to add missing links)
> 
> > +///
> > +/// For more details see [self].
> > +pub struct Kmalloc;
> > 
> >  /// Returns a proper size to alloc a new object aligned to `new_layout`'s alignment.
> >  fn aligned_size(new_layout: Layout) -> usize {
> > @@ -36,6 +52,60 @@ 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 }
> >  }
> > 
> > +/// # Invariants
> > +///
> > +/// One of the following `krealloc`, `vrealloc`, `kvrealloc`.
> > +struct ReallocFunc(
> > +    unsafe extern "C" fn(*const core::ffi::c_void, usize, u32) -> *mut core::ffi::c_void,
> > +);
> > +
> > +impl ReallocFunc {
> > +    // INVARIANT: `krealloc` satisfies the type invariants.
> > +    const KREALLOC: Self = Self(bindings::krealloc);
> > +
> > +    /// # Safety
> > +    ///
> > +    /// This method has the same safety requirements as [`Allocator::realloc`].
> > +    unsafe fn call(
> > +        &self,
> > +        ptr: Option<NonNull<u8>>,
> > +        layout: Layout,
> > +        flags: Flags,
> > +    ) -> Result<NonNull<[u8]>, AllocError> {
> > +        let size = aligned_size(layout);
> > +        let ptr = match ptr {
> > +            Some(ptr) => ptr.as_ptr(),
> > +            None => ptr::null(),
> > +        };
> > +
> > +        // SAFETY: `ptr` is either NULL or valid by the safety requirements of this function.
> 
> You need some justification as to why calling the three allowed
> functions here.

What kind of justification do I need? Can you please share some more details on
what you think is missing here?

> 
> > +        let raw_ptr = unsafe {
> > +            // If `size == 0` and `ptr != NULL` the memory behind the pointer is freed.
> > +            self.0(ptr.cast(), size, flags.0).cast()
> > +        };
> > +
> > +        let ptr = if size == 0 {
> > +            NonNull::dangling()
> > +        } else {
> > +            NonNull::new(raw_ptr).ok_or(AllocError)?
> > +        };
> > +
> > +        Ok(NonNull::slice_from_raw_parts(ptr, size))
> > +    }
> > +}
> > +
> > +unsafe impl Allocator for Kmalloc {
> 
> Missing SAFETY comment.

Yeah, I think we came across this in an earlier version of the series. I asked
you about the content and usefulness of a comment here, since I'd just end up
re-iterating what the `Allocator` trait documentation says.

IIRC, you replied that you want to think of something that'd make sense to add
here.

What do you think should be written here?

> 
> ---
> Cheers,
> Benno
> 
> > +    #[inline]
> > +    unsafe fn realloc(
> > +        ptr: Option<NonNull<u8>>,
> > +        layout: Layout,
> > +        flags: Flags,
> > +    ) -> Result<NonNull<[u8]>, AllocError> {
> > +        // SAFETY: `ReallocFunc::call` has the same safety requirements as `Allocator::realloc`.
> > +        unsafe { ReallocFunc::KREALLOC.call(ptr, layout, flags) }
> > +    }
> > +}
> > +
> >  unsafe impl GlobalAlloc for Kmalloc {
> >      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.46.0
> > 
>

On 30.08.24 00:04, Danilo Krummrich wrote:
> On Thu, Aug 29, 2024 at 06:32:42PM +0000, Benno Lossin wrote:
>> On 16.08.24 02:10, Danilo Krummrich wrote:
>>> Implement `Allocator` for `Kmalloc`, the kernel's default allocator,
>>> typically used for objects smaller than page size.
>>>
>>> All memory allocations made with `Kmalloc` end up in `krealloc()`.
>>>
>>> It serves as allocator for the subsequently introduced types `KBox` and
>>> `KVec`.
>>>
>>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>>> ---
>>>  rust/kernel/alloc.rs           |  2 +-
>>>  rust/kernel/alloc/allocator.rs | 72 +++++++++++++++++++++++++++++++++-
>>>  2 files changed, 72 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
>>> index 9932f21b0539..477dbe3c5a2f 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 e32182f91167..78e7d5488843 100644
>>> --- a/rust/kernel/alloc/allocator.rs
>>> +++ b/rust/kernel/alloc/allocator.rs
>>> @@ -1,12 +1,28 @@
>>>  // SPDX-License-Identifier: GPL-2.0
>>>
>>>  //! Allocator support.
>>> +//!
>>> +//! Documentation for the kernel's memory allocators can found in the "Memory Allocation Guide"
>>> +//! linked below. For instance, this includes the concept of "get free page" (GFP) flags and the
>>> +//! typical application of the different kernel allocators.
>>> +//!
>>> +//! Reference: <https://docs.kernel.org/core-api/memory-allocation.html>
>>
>> Thanks, this nice.
>>
>>>
>>>  use super::{flags::*, Flags};
>>>  use core::alloc::{GlobalAlloc, Layout};
>>>  use core::ptr;
>>> +use core::ptr::NonNull;
>>>
>>> -struct Kmalloc;
>>> +use crate::alloc::{AllocError, Allocator};
>>> +use crate::bindings;
>>> +
>>> +/// The contiguous kernel allocator.
>>> +///
>>> +/// `Kmalloc` is typically used for physically contiguous allocations up to page size, but also
>>> +/// supports larger allocations up to `bindings::KMALLOC_MAX_SIZE`, which is hardware specific.
>>
>> Does putting a link here work? (I guess we don't yet export the bindings
>> documentation, so it will probably fail... When we decide to enable it,
>> we should create an issue to add missing links)
>>
>>> +///
>>> +/// For more details see [self].
>>> +pub struct Kmalloc;
>>>
>>>  /// Returns a proper size to alloc a new object aligned to `new_layout`'s alignment.
>>>  fn aligned_size(new_layout: Layout) -> usize {
>>> @@ -36,6 +52,60 @@ 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 }
>>>  }
>>>
>>> +/// # Invariants
>>> +///
>>> +/// One of the following `krealloc`, `vrealloc`, `kvrealloc`.
>>> +struct ReallocFunc(
>>> +    unsafe extern "C" fn(*const core::ffi::c_void, usize, u32) -> *mut core::ffi::c_void,
>>> +);
>>> +
>>> +impl ReallocFunc {
>>> +    // INVARIANT: `krealloc` satisfies the type invariants.
>>> +    const KREALLOC: Self = Self(bindings::krealloc);
>>> +
>>> +    /// # Safety
>>> +    ///
>>> +    /// This method has the same safety requirements as [`Allocator::realloc`].
>>> +    unsafe fn call(
>>> +        &self,
>>> +        ptr: Option<NonNull<u8>>,
>>> +        layout: Layout,
>>> +        flags: Flags,
>>> +    ) -> Result<NonNull<[u8]>, AllocError> {
>>> +        let size = aligned_size(layout);
>>> +        let ptr = match ptr {
>>> +            Some(ptr) => ptr.as_ptr(),
>>> +            None => ptr::null(),
>>> +        };
>>> +
>>> +        // SAFETY: `ptr` is either NULL or valid by the safety requirements of this function.
>>
>> You need some justification as to why calling the three allowed
>> functions here.
> 
> What kind of justification do I need? Can you please share some more details on
> what you think is missing here?

So, you are calling a function pointer to an `unsafe` function. This
means that through some invariant you have to know what the safety
requirements are (otherwise how can you guarantee that this is OK?). You
have the invariant that the pointer points at one of the three functions
mentioned above. What are the safety requirements of those functions? I
would assume that the only one is that `ptr` is valid. So you can use:

    // SAFETY:
    // - `self.0` is one of `krealloc`, `vrealloc`, `kvrealloc` and thus only requires that `ptr` is
    //   NULL or valid.
    // - `ptr` is either NULL or valid by the safety requirements of this function.

>>> +        let raw_ptr = unsafe {
>>> +            // If `size == 0` and `ptr != NULL` the memory behind the pointer is freed.
>>> +            self.0(ptr.cast(), size, flags.0).cast()
>>> +        };
>>> +
>>> +        let ptr = if size == 0 {
>>> +            NonNull::dangling()
>>> +        } else {
>>> +            NonNull::new(raw_ptr).ok_or(AllocError)?
>>> +        };
>>> +
>>> +        Ok(NonNull::slice_from_raw_parts(ptr, size))
>>> +    }
>>> +}
>>> +
>>> +unsafe impl Allocator for Kmalloc {
>>
>> Missing SAFETY comment.
> 
> Yeah, I think we came across this in an earlier version of the series. I asked
> you about the content and usefulness of a comment here, since I'd just end up
> re-iterating what the `Allocator` trait documentation says.
> 
> IIRC, you replied that you want to think of something that'd make sense to add
> here.

Oh yeah, sorry I forgot about that.
 
> What do you think should be written here?

I think the best way to do it, would be to push this question down into
`ReallocFunc::call`. So we would put this on the trait:

    // SAFETY: `realloc` delegates to `ReallocFunc::call`, which guarantees that
    // - memory remains valid until it is explicitly freed,
    // - passing a pointer to a vaild memory allocation is OK,
    // - `realloc` satisfies the guarantees, since `ReallocFunc::call` has the same.

We then need to put this on `ReallocFunc::call`:

    /// # Guarantees
    ///
    /// This method has the same guarantees as `Allocator::realloc`. Additionally
    /// - it accepts any pointer to a valid memory allocation allocated by this function.
    /// - memory allocated by this function remains valid until it is passed to this function.

Finally, we need a `GUARANTEE` comment (just above the return [^1]
value) that establishes these guarantees:

    // GUARANTEE: Since we called `self.0` with `size` above and by the type invariants of `Self`,
    // `self.0` is one of `krealloc`, `vrealloc`, `kvrealloc`. Those functions provide the guarantees of
    // this function.

I am not really happy with the last sentence, but I also don't think
that there is value in listing out all the guarantees, only to then say
"all of this is guaranteed by us calling one of these three functions.


[^1]: I am not sure that there is the right place. If you have any
      suggestions, feel free to share them.


>>> +    #[inline]
>>> +    unsafe fn realloc(
>>> +        ptr: Option<NonNull<u8>>,
>>> +        layout: Layout,
>>> +        flags: Flags,
>>> +    ) -> Result<NonNull<[u8]>, AllocError> {
>>> +        // SAFETY: `ReallocFunc::call` has the same safety requirements as `Allocator::realloc`.
>>> +        unsafe { ReallocFunc::KREALLOC.call(ptr, layout, flags) }
>>> +    }
>>> +}

Oh one more thing, I know that you already have a lot of patches in this
series, but could you split this one into two? So the first one should
introduce `ReallocFunc` and the second one add the impl for `Kmalloc`?
I managed to confuse me twice because of that :) 

---
Cheers,
Benno

>>> +
>>>  unsafe impl GlobalAlloc for Kmalloc {
>>>      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.46.0
>>>
>>

On Fri, Aug 30, 2024 at 02:45:35PM +0000, Benno Lossin wrote:
> On 30.08.24 00:04, Danilo Krummrich wrote:
> > On Thu, Aug 29, 2024 at 06:32:42PM +0000, Benno Lossin wrote:
> >> On 16.08.24 02:10, Danilo Krummrich wrote:
> >>> Implement `Allocator` for `Kmalloc`, the kernel's default allocator,
> >>> typically used for objects smaller than page size.
> >>>
> >>> All memory allocations made with `Kmalloc` end up in `krealloc()`.
> >>>
> >>> It serves as allocator for the subsequently introduced types `KBox` and
> >>> `KVec`.
> >>>
> >>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> >>> ---
> >>>  rust/kernel/alloc.rs           |  2 +-
> >>>  rust/kernel/alloc/allocator.rs | 72 +++++++++++++++++++++++++++++++++-
> >>>  2 files changed, 72 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> >>> index 9932f21b0539..477dbe3c5a2f 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 e32182f91167..78e7d5488843 100644
> >>> --- a/rust/kernel/alloc/allocator.rs
> >>> +++ b/rust/kernel/alloc/allocator.rs
> >>> @@ -1,12 +1,28 @@
> >>>  // SPDX-License-Identifier: GPL-2.0
> >>>
> >>>  //! Allocator support.
> >>> +//!
> >>> +//! Documentation for the kernel's memory allocators can found in the "Memory Allocation Guide"
> >>> +//! linked below. For instance, this includes the concept of "get free page" (GFP) flags and the
> >>> +//! typical application of the different kernel allocators.
> >>> +//!
> >>> +//! Reference: <https://docs.kernel.org/core-api/memory-allocation.html>
> >>
> >> Thanks, this nice.
> >>
> >>>
> >>>  use super::{flags::*, Flags};
> >>>  use core::alloc::{GlobalAlloc, Layout};
> >>>  use core::ptr;
> >>> +use core::ptr::NonNull;
> >>>
> >>> -struct Kmalloc;
> >>> +use crate::alloc::{AllocError, Allocator};
> >>> +use crate::bindings;
> >>> +
> >>> +/// The contiguous kernel allocator.
> >>> +///
> >>> +/// `Kmalloc` is typically used for physically contiguous allocations up to page size, but also
> >>> +/// supports larger allocations up to `bindings::KMALLOC_MAX_SIZE`, which is hardware specific.
> >>
> >> Does putting a link here work? (I guess we don't yet export the bindings
> >> documentation, so it will probably fail... When we decide to enable it,
> >> we should create an issue to add missing links)
> >>
> >>> +///
> >>> +/// For more details see [self].
> >>> +pub struct Kmalloc;
> >>>
> >>>  /// Returns a proper size to alloc a new object aligned to `new_layout`'s alignment.
> >>>  fn aligned_size(new_layout: Layout) -> usize {
> >>> @@ -36,6 +52,60 @@ 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 }
> >>>  }
> >>>
> >>> +/// # Invariants
> >>> +///
> >>> +/// One of the following `krealloc`, `vrealloc`, `kvrealloc`.
> >>> +struct ReallocFunc(
> >>> +    unsafe extern "C" fn(*const core::ffi::c_void, usize, u32) -> *mut core::ffi::c_void,
> >>> +);
> >>> +
> >>> +impl ReallocFunc {
> >>> +    // INVARIANT: `krealloc` satisfies the type invariants.
> >>> +    const KREALLOC: Self = Self(bindings::krealloc);
> >>> +
> >>> +    /// # Safety
> >>> +    ///
> >>> +    /// This method has the same safety requirements as [`Allocator::realloc`].
> >>> +    unsafe fn call(
> >>> +        &self,
> >>> +        ptr: Option<NonNull<u8>>,
> >>> +        layout: Layout,
> >>> +        flags: Flags,
> >>> +    ) -> Result<NonNull<[u8]>, AllocError> {
> >>> +        let size = aligned_size(layout);
> >>> +        let ptr = match ptr {
> >>> +            Some(ptr) => ptr.as_ptr(),
> >>> +            None => ptr::null(),
> >>> +        };
> >>> +
> >>> +        // SAFETY: `ptr` is either NULL or valid by the safety requirements of this function.
> >>
> >> You need some justification as to why calling the three allowed
> >> functions here.
> > 
> > What kind of justification do I need? Can you please share some more details on
> > what you think is missing here?
> 
> So, you are calling a function pointer to an `unsafe` function. This
> means that through some invariant you have to know what the safety
> requirements are (otherwise how can you guarantee that this is OK?). You
> have the invariant that the pointer points at one of the three functions
> mentioned above. What are the safety requirements of those functions? I
> would assume that the only one is that `ptr` is valid. So you can use:
> 
>     // SAFETY:
>     // - `self.0` is one of `krealloc`, `vrealloc`, `kvrealloc` and thus only requires that `ptr` is
>     //   NULL or valid.

I'm fine adding it, but I'd like to understand why you think it's required in
the safety comment here? Isn't this implicit by being the type invariant?

>     // - `ptr` is either NULL or valid by the safety requirements of this function.

This is the part I already have.

> 
> >>> +        let raw_ptr = unsafe {
> >>> +            // If `size == 0` and `ptr != NULL` the memory behind the pointer is freed.
> >>> +            self.0(ptr.cast(), size, flags.0).cast()
> >>> +        };
> >>> +
> >>> +        let ptr = if size == 0 {
> >>> +            NonNull::dangling()
> >>> +        } else {
> >>> +            NonNull::new(raw_ptr).ok_or(AllocError)?
> >>> +        };
> >>> +
> >>> +        Ok(NonNull::slice_from_raw_parts(ptr, size))
> >>> +    }
> >>> +}
> >>> +
> >>> +unsafe impl Allocator for Kmalloc {
> >>
> >> Missing SAFETY comment.
> > 
> > Yeah, I think we came across this in an earlier version of the series. I asked
> > you about the content and usefulness of a comment here, since I'd just end up
> > re-iterating what the `Allocator` trait documentation says.
> > 
> > IIRC, you replied that you want to think of something that'd make sense to add
> > here.
> 
> Oh yeah, sorry I forgot about that.
>  
> > What do you think should be written here?
> 
> I think the best way to do it, would be to push this question down into
> `ReallocFunc::call`. So we would put this on the trait:
> 
>     // SAFETY: `realloc` delegates to `ReallocFunc::call`, which guarantees that
>     // - memory remains valid until it is explicitly freed,
>     // - passing a pointer to a vaild memory allocation is OK,
>     // - `realloc` satisfies the guarantees, since `ReallocFunc::call` has the same.

So, we'd also need the same for:
  - `unsafe impl Allocator for Vmalloc`
  - `unsafe impl Allocator for KVmalloc`

> 
> We then need to put this on `ReallocFunc::call`:
> 
>     /// # Guarantees
>     ///
>     /// This method has the same guarantees as `Allocator::realloc`. Additionally
>     /// - it accepts any pointer to a valid memory allocation allocated by this function.

You propose this, since for `Allocator::realloc` memory allocated with
`Allocator::alloc` would be fine too I guess.

But if e.g. `Kmalloc` wouldn't use the default `Allocator::alloc`, this would be
valid too.

We could instead write something like:

"it accepts any pointer to a valid memory allocation allocated with the same
kernel allocator."

>     /// - memory allocated by this function remains valid until it is passed to this function.

Same here, `Kmalloc` could implement its own `Allocator::free`.

Maybe just "...until it is explicitly freed.".

Anyway, I'm fine with both, since non of the kernel allocators uses anything
else than `ReallocFunc::call` to allocate and free memory.

> 
> Finally, we need a `GUARANTEE` comment (just above the return [^1]
> value) that establishes these guarantees:
> 
>     // GUARANTEE: Since we called `self.0` with `size` above and by the type invariants of `Self`,
>     // `self.0` is one of `krealloc`, `vrealloc`, `kvrealloc`. Those functions provide the guarantees of
>     // this function.
> 
> I am not really happy with the last sentence, but I also don't think
> that there is value in listing out all the guarantees, only to then say
> "all of this is guaranteed by us calling one of these three functions.
> 
> 
> [^1]: I am not sure that there is the right place. If you have any
>       suggestions, feel free to share them.

Either way, I'm fine with this proposal.

> 
> 
> >>> +    #[inline]
> >>> +    unsafe fn realloc(
> >>> +        ptr: Option<NonNull<u8>>,
> >>> +        layout: Layout,
> >>> +        flags: Flags,
> >>> +    ) -> Result<NonNull<[u8]>, AllocError> {
> >>> +        // SAFETY: `ReallocFunc::call` has the same safety requirements as `Allocator::realloc`.
> >>> +        unsafe { ReallocFunc::KREALLOC.call(ptr, layout, flags) }
> >>> +    }
> >>> +}
> 
> Oh one more thing, I know that you already have a lot of patches in this
> series, but could you split this one into two? So the first one should
> introduce `ReallocFunc` and the second one add the impl for `Kmalloc`?
> I managed to confuse me twice because of that :) 

Generally, I'm fine with that, but I'm not sure if I can avoid an intermediate
compiler warning about unused code doing that.

> 
> ---
> Cheers,
> Benno
> 
> >>> +
> >>>  unsafe impl GlobalAlloc for Kmalloc {
> >>>      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.46.0
> >>>
> >>
>

On 03.09.24 13:48, Danilo Krummrich wrote:
> On Fri, Aug 30, 2024 at 02:45:35PM +0000, Benno Lossin wrote:
>> On 30.08.24 00:04, Danilo Krummrich wrote:
>>> On Thu, Aug 29, 2024 at 06:32:42PM +0000, Benno Lossin wrote:
>>>> On 16.08.24 02:10, Danilo Krummrich wrote:
>>>>> +///
>>>>> +/// For more details see [self].
>>>>> +pub struct Kmalloc;
>>>>>
>>>>>  /// Returns a proper size to alloc a new object aligned to `new_layout`'s alignment.
>>>>>  fn aligned_size(new_layout: Layout) -> usize {
>>>>> @@ -36,6 +52,60 @@ 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 }
>>>>>  }
>>>>>
>>>>> +/// # Invariants
>>>>> +///
>>>>> +/// One of the following `krealloc`, `vrealloc`, `kvrealloc`.
>>>>> +struct ReallocFunc(
>>>>> +    unsafe extern "C" fn(*const core::ffi::c_void, usize, u32) -> *mut core::ffi::c_void,
>>>>> +);
>>>>> +
>>>>> +impl ReallocFunc {
>>>>> +    // INVARIANT: `krealloc` satisfies the type invariants.
>>>>> +    const KREALLOC: Self = Self(bindings::krealloc);
>>>>> +
>>>>> +    /// # Safety
>>>>> +    ///
>>>>> +    /// This method has the same safety requirements as [`Allocator::realloc`].
>>>>> +    unsafe fn call(
>>>>> +        &self,
>>>>> +        ptr: Option<NonNull<u8>>,
>>>>> +        layout: Layout,
>>>>> +        flags: Flags,
>>>>> +    ) -> Result<NonNull<[u8]>, AllocError> {
>>>>> +        let size = aligned_size(layout);
>>>>> +        let ptr = match ptr {
>>>>> +            Some(ptr) => ptr.as_ptr(),
>>>>> +            None => ptr::null(),
>>>>> +        };
>>>>> +
>>>>> +        // SAFETY: `ptr` is either NULL or valid by the safety requirements of this function.
>>>>
>>>> You need some justification as to why calling the three allowed
>>>> functions here.
>>>
>>> What kind of justification do I need? Can you please share some more details on
>>> what you think is missing here?
>>
>> So, you are calling a function pointer to an `unsafe` function. This
>> means that through some invariant you have to know what the safety
>> requirements are (otherwise how can you guarantee that this is OK?). You
>> have the invariant that the pointer points at one of the three functions
>> mentioned above. What are the safety requirements of those functions? I
>> would assume that the only one is that `ptr` is valid. So you can use:
>>
>>     // SAFETY:
>>     // - `self.0` is one of `krealloc`, `vrealloc`, `kvrealloc` and thus only requires that `ptr` is
>>     //   NULL or valid.
> 
> I'm fine adding it, but I'd like to understand why you think it's required in
> the safety comment here? Isn't this implicit by being the type invariant?

You are calling a function pointer to an `unsafe` function that takes a
raw pointer. Without this comment it is not clear what the function
pointer's safety requirements are for the raw pointer parameter.

>>     // - `ptr` is either NULL or valid by the safety requirements of this function.
> 
> This is the part I already have.

I kept it to ensure that you also keep it.

>>>>> +        let raw_ptr = unsafe {
>>>>> +            // If `size == 0` and `ptr != NULL` the memory behind the pointer is freed.
>>>>> +            self.0(ptr.cast(), size, flags.0).cast()
>>>>> +        };
>>>>> +
>>>>> +        let ptr = if size == 0 {
>>>>> +            NonNull::dangling()
>>>>> +        } else {
>>>>> +            NonNull::new(raw_ptr).ok_or(AllocError)?
>>>>> +        };
>>>>> +
>>>>> +        Ok(NonNull::slice_from_raw_parts(ptr, size))
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +unsafe impl Allocator for Kmalloc {
>>>>
>>>> Missing SAFETY comment.
>>>
>>> Yeah, I think we came across this in an earlier version of the series. I asked
>>> you about the content and usefulness of a comment here, since I'd just end up
>>> re-iterating what the `Allocator` trait documentation says.
>>>
>>> IIRC, you replied that you want to think of something that'd make sense to add
>>> here.
>>
>> Oh yeah, sorry I forgot about that.
>>
>>> What do you think should be written here?
>>
>> I think the best way to do it, would be to push this question down into
>> `ReallocFunc::call`. So we would put this on the trait:
>>
>>     // SAFETY: `realloc` delegates to `ReallocFunc::call`, which guarantees that
>>     // - memory remains valid until it is explicitly freed,
>>     // - passing a pointer to a vaild memory allocation is OK,
>>     // - `realloc` satisfies the guarantees, since `ReallocFunc::call` has the same.
> 
> So, we'd also need the same for:
>   - `unsafe impl Allocator for Vmalloc`
>   - `unsafe impl Allocator for KVmalloc`

Yes.

>> We then need to put this on `ReallocFunc::call`:
>>
>>     /// # Guarantees
>>     ///
>>     /// This method has the same guarantees as `Allocator::realloc`. Additionally
>>     /// - it accepts any pointer to a valid memory allocation allocated by this function.
> 
> You propose this, since for `Allocator::realloc` memory allocated with
> `Allocator::alloc` would be fine too I guess.
> 
> But if e.g. `Kmalloc` wouldn't use the default `Allocator::alloc`, this would be
> valid too.

So if `Kmalloc` were to implement `alloc` by not calling
`ReallocFun::call`, then we couldn't use this comment. Do you think that
such a change might be required at some point?

> We could instead write something like:
> 
> "it accepts any pointer to a valid memory allocation allocated with the same
> kernel allocator."

It would be better, if we can keep it simpler (ie only `realloc` is
implemented).

>>     /// - memory allocated by this function remains valid until it is passed to this function.
> 
> Same here, `Kmalloc` could implement its own `Allocator::free`.
> 
> Maybe just "...until it is explicitly freed.".

I don't really like that, since by that any other function could be
meant. Do you need to override the `free` function? If not then it would
be better.

> Anyway, I'm fine with both, since non of the kernel allocators uses anything
> else than `ReallocFunc::call` to allocate and free memory.
> 
>>
>> Finally, we need a `GUARANTEE` comment (just above the return [^1]
>> value) that establishes these guarantees:
>>
>>     // GUARANTEE: Since we called `self.0` with `size` above and by the type invariants of `Self`,
>>     // `self.0` is one of `krealloc`, `vrealloc`, `kvrealloc`. Those functions provide the guarantees of
>>     // this function.
>>
>> I am not really happy with the last sentence, but I also don't think
>> that there is value in listing out all the guarantees, only to then say
>> "all of this is guaranteed by us calling one of these three functions.
>>
>>
>> [^1]: I am not sure that there is the right place. If you have any
>>       suggestions, feel free to share them.
> 
> Either way, I'm fine with this proposal.
> 
>>
>>
>>>>> +    #[inline]
>>>>> +    unsafe fn realloc(
>>>>> +        ptr: Option<NonNull<u8>>,
>>>>> +        layout: Layout,
>>>>> +        flags: Flags,
>>>>> +    ) -> Result<NonNull<[u8]>, AllocError> {
>>>>> +        // SAFETY: `ReallocFunc::call` has the same safety requirements as `Allocator::realloc`.
>>>>> +        unsafe { ReallocFunc::KREALLOC.call(ptr, layout, flags) }
>>>>> +    }
>>>>> +}
>>
>> Oh one more thing, I know that you already have a lot of patches in this
>> series, but could you split this one into two? So the first one should
>> introduce `ReallocFunc` and the second one add the impl for `Kmalloc`?
>> I managed to confuse me twice because of that :)
> 
> Generally, I'm fine with that, but I'm not sure if I can avoid an intermediate
> compiler warning about unused code doing that.

You can just use `#[expect(dead_code)]` for that in the intermediate
patches.

---
Cheers,
Benno

On Tue, Sep 10, 2024 at 01:11:35PM +0000, Benno Lossin wrote:
> On 03.09.24 13:48, Danilo Krummrich wrote:
> > On Fri, Aug 30, 2024 at 02:45:35PM +0000, Benno Lossin wrote:
> >> On 30.08.24 00:04, Danilo Krummrich wrote:
> >>> On Thu, Aug 29, 2024 at 06:32:42PM +0000, Benno Lossin wrote:
> >>>> On 16.08.24 02:10, Danilo Krummrich wrote:
> >>>>> +///
> >>>>> +/// For more details see [self].
> >>>>> +pub struct Kmalloc;
> >>>>>
> >>>>>  /// Returns a proper size to alloc a new object aligned to `new_layout`'s alignment.
> >>>>>  fn aligned_size(new_layout: Layout) -> usize {
> >>>>> @@ -36,6 +52,60 @@ 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 }
> >>>>>  }
> >>>>>
> >>>>> +/// # Invariants
> >>>>> +///
> >>>>> +/// One of the following `krealloc`, `vrealloc`, `kvrealloc`.
> >>>>> +struct ReallocFunc(
> >>>>> +    unsafe extern "C" fn(*const core::ffi::c_void, usize, u32) -> *mut core::ffi::c_void,
> >>>>> +);
> >>>>> +
> >>>>> +impl ReallocFunc {
> >>>>> +    // INVARIANT: `krealloc` satisfies the type invariants.
> >>>>> +    const KREALLOC: Self = Self(bindings::krealloc);
> >>>>> +
> >>>>> +    /// # Safety
> >>>>> +    ///
> >>>>> +    /// This method has the same safety requirements as [`Allocator::realloc`].
> >>>>> +    unsafe fn call(
> >>>>> +        &self,
> >>>>> +        ptr: Option<NonNull<u8>>,
> >>>>> +        layout: Layout,
> >>>>> +        flags: Flags,
> >>>>> +    ) -> Result<NonNull<[u8]>, AllocError> {
> >>>>> +        let size = aligned_size(layout);
> >>>>> +        let ptr = match ptr {
> >>>>> +            Some(ptr) => ptr.as_ptr(),
> >>>>> +            None => ptr::null(),
> >>>>> +        };
> >>>>> +
> >>>>> +        // SAFETY: `ptr` is either NULL or valid by the safety requirements of this function.
> >>>>
> >>>> You need some justification as to why calling the three allowed
> >>>> functions here.
> >>>
> >>> What kind of justification do I need? Can you please share some more details on
> >>> what you think is missing here?
> >>
> >> So, you are calling a function pointer to an `unsafe` function. This
> >> means that through some invariant you have to know what the safety
> >> requirements are (otherwise how can you guarantee that this is OK?). You
> >> have the invariant that the pointer points at one of the three functions
> >> mentioned above. What are the safety requirements of those functions? I
> >> would assume that the only one is that `ptr` is valid. So you can use:
> >>
> >>     // SAFETY:
> >>     // - `self.0` is one of `krealloc`, `vrealloc`, `kvrealloc` and thus only requires that `ptr` is
> >>     //   NULL or valid.
> > 
> > I'm fine adding it, but I'd like to understand why you think it's required in
> > the safety comment here? Isn't this implicit by being the type invariant?
> 
> You are calling a function pointer to an `unsafe` function that takes a
> raw pointer. Without this comment it is not clear what the function
> pointer's safety requirements are for the raw pointer parameter.

That's my point, isn't this implicitly clear by the type invariant? If needed,
shouldn't it be:

// INVARIANT:
// - `self.0` is one of [...]
//
// SAFETY:
// - `ptr` is either NULL or [...]

> 
> >>     // - `ptr` is either NULL or valid by the safety requirements of this function.
> > 
> > This is the part I already have.
> 
> I kept it to ensure that you also keep it.
> 
> >>>>> +        let raw_ptr = unsafe {
> >>>>> +            // If `size == 0` and `ptr != NULL` the memory behind the pointer is freed.
> >>>>> +            self.0(ptr.cast(), size, flags.0).cast()
> >>>>> +        };
> >>>>> +
> >>>>> +        let ptr = if size == 0 {
> >>>>> +            NonNull::dangling()
> >>>>> +        } else {
> >>>>> +            NonNull::new(raw_ptr).ok_or(AllocError)?
> >>>>> +        };
> >>>>> +
> >>>>> +        Ok(NonNull::slice_from_raw_parts(ptr, size))
> >>>>> +    }
> >>>>> +}
> >>>>> +
> >>>>> +unsafe impl Allocator for Kmalloc {
> >>>>
> >>>> Missing SAFETY comment.
> >>>
> >>> Yeah, I think we came across this in an earlier version of the series. I asked
> >>> you about the content and usefulness of a comment here, since I'd just end up
> >>> re-iterating what the `Allocator` trait documentation says.
> >>>
> >>> IIRC, you replied that you want to think of something that'd make sense to add
> >>> here.
> >>
> >> Oh yeah, sorry I forgot about that.
> >>
> >>> What do you think should be written here?
> >>
> >> I think the best way to do it, would be to push this question down into
> >> `ReallocFunc::call`. So we would put this on the trait:
> >>
> >>     // SAFETY: `realloc` delegates to `ReallocFunc::call`, which guarantees that
> >>     // - memory remains valid until it is explicitly freed,
> >>     // - passing a pointer to a vaild memory allocation is OK,
> >>     // - `realloc` satisfies the guarantees, since `ReallocFunc::call` has the same.
> > 
> > So, we'd also need the same for:
> >   - `unsafe impl Allocator for Vmalloc`
> >   - `unsafe impl Allocator for KVmalloc`
> 
> Yes.
> 
> >> We then need to put this on `ReallocFunc::call`:
> >>
> >>     /// # Guarantees
> >>     ///
> >>     /// This method has the same guarantees as `Allocator::realloc`. Additionally
> >>     /// - it accepts any pointer to a valid memory allocation allocated by this function.
> > 
> > You propose this, since for `Allocator::realloc` memory allocated with
> > `Allocator::alloc` would be fine too I guess.
> > 
> > But if e.g. `Kmalloc` wouldn't use the default `Allocator::alloc`, this would be
> > valid too.
> 
> So if `Kmalloc` were to implement `alloc` by not calling
> `ReallocFun::call`, then we couldn't use this comment. Do you think that
> such a change might be required at some point?

I don't think so, this was purely hypothetical. Let's stick to your proposal.

> 
> > We could instead write something like:
> > 
> > "it accepts any pointer to a valid memory allocation allocated with the same
> > kernel allocator."
> 
> It would be better, if we can keep it simpler (ie only `realloc` is
> implemented).
> 
> >>     /// - memory allocated by this function remains valid until it is passed to this function.
> > 
> > Same here, `Kmalloc` could implement its own `Allocator::free`.
> > 
> > Maybe just "...until it is explicitly freed.".
> 
> I don't really like that, since by that any other function could be
> meant. Do you need to override the `free` function? If not then it would
> be better.
> 
> > Anyway, I'm fine with both, since non of the kernel allocators uses anything
> > else than `ReallocFunc::call` to allocate and free memory.
> > 
> >>
> >> Finally, we need a `GUARANTEE` comment (just above the return [^1]
> >> value) that establishes these guarantees:
> >>
> >>     // GUARANTEE: Since we called `self.0` with `size` above and by the type invariants of `Self`,
> >>     // `self.0` is one of `krealloc`, `vrealloc`, `kvrealloc`. Those functions provide the guarantees of
> >>     // this function.
> >>
> >> I am not really happy with the last sentence, but I also don't think
> >> that there is value in listing out all the guarantees, only to then say
> >> "all of this is guaranteed by us calling one of these three functions.
> >>
> >>
> >> [^1]: I am not sure that there is the right place. If you have any
> >>       suggestions, feel free to share them.
> > 
> > Either way, I'm fine with this proposal.
> > 
> >>
> >>
> >>>>> +    #[inline]
> >>>>> +    unsafe fn realloc(
> >>>>> +        ptr: Option<NonNull<u8>>,
> >>>>> +        layout: Layout,
> >>>>> +        flags: Flags,
> >>>>> +    ) -> Result<NonNull<[u8]>, AllocError> {
> >>>>> +        // SAFETY: `ReallocFunc::call` has the same safety requirements as `Allocator::realloc`.
> >>>>> +        unsafe { ReallocFunc::KREALLOC.call(ptr, layout, flags) }
> >>>>> +    }
> >>>>> +}
> >>
> >> Oh one more thing, I know that you already have a lot of patches in this
> >> series, but could you split this one into two? So the first one should
> >> introduce `ReallocFunc` and the second one add the impl for `Kmalloc`?
> >> I managed to confuse me twice because of that :)
> > 
> > Generally, I'm fine with that, but I'm not sure if I can avoid an intermediate
> > compiler warning about unused code doing that.
> 
> You can just use `#[expect(dead_code)]` for that in the intermediate
> patches.

I usually try to avoid that, because it can be misleading when bisecting things.

If the temporarily unused code contains a bug, your bisection doesn't end up at
this patch, but some other patch that starts using it.

> 
> ---
> Cheers,
> Benno
>

On 10.09.24 15:37, Danilo Krummrich wrote:
> On Tue, Sep 10, 2024 at 01:11:35PM +0000, Benno Lossin wrote:
>> On 03.09.24 13:48, Danilo Krummrich wrote:
>>> On Fri, Aug 30, 2024 at 02:45:35PM +0000, Benno Lossin wrote:
>>>> On 30.08.24 00:04, Danilo Krummrich wrote:
>>>>> On Thu, Aug 29, 2024 at 06:32:42PM +0000, Benno Lossin wrote:
>>>>>> On 16.08.24 02:10, Danilo Krummrich wrote:
>>>>>>> +///
>>>>>>> +/// For more details see [self].
>>>>>>> +pub struct Kmalloc;
>>>>>>>
>>>>>>>  /// Returns a proper size to alloc a new object aligned to `new_layout`'s alignment.
>>>>>>>  fn aligned_size(new_layout: Layout) -> usize {
>>>>>>> @@ -36,6 +52,60 @@ 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 }
>>>>>>>  }
>>>>>>>
>>>>>>> +/// # Invariants
>>>>>>> +///
>>>>>>> +/// One of the following `krealloc`, `vrealloc`, `kvrealloc`.
>>>>>>> +struct ReallocFunc(
>>>>>>> +    unsafe extern "C" fn(*const core::ffi::c_void, usize, u32) -> *mut core::ffi::c_void,
>>>>>>> +);
>>>>>>> +
>>>>>>> +impl ReallocFunc {
>>>>>>> +    // INVARIANT: `krealloc` satisfies the type invariants.
>>>>>>> +    const KREALLOC: Self = Self(bindings::krealloc);
>>>>>>> +
>>>>>>> +    /// # Safety
>>>>>>> +    ///
>>>>>>> +    /// This method has the same safety requirements as [`Allocator::realloc`].
>>>>>>> +    unsafe fn call(
>>>>>>> +        &self,
>>>>>>> +        ptr: Option<NonNull<u8>>,
>>>>>>> +        layout: Layout,
>>>>>>> +        flags: Flags,
>>>>>>> +    ) -> Result<NonNull<[u8]>, AllocError> {
>>>>>>> +        let size = aligned_size(layout);
>>>>>>> +        let ptr = match ptr {
>>>>>>> +            Some(ptr) => ptr.as_ptr(),
>>>>>>> +            None => ptr::null(),
>>>>>>> +        };
>>>>>>> +
>>>>>>> +        // SAFETY: `ptr` is either NULL or valid by the safety requirements of this function.
>>>>>>
>>>>>> You need some justification as to why calling the three allowed
>>>>>> functions here.
>>>>>
>>>>> What kind of justification do I need? Can you please share some more details on
>>>>> what you think is missing here?
>>>>
>>>> So, you are calling a function pointer to an `unsafe` function. This
>>>> means that through some invariant you have to know what the safety
>>>> requirements are (otherwise how can you guarantee that this is OK?). You
>>>> have the invariant that the pointer points at one of the three functions
>>>> mentioned above. What are the safety requirements of those functions? I
>>>> would assume that the only one is that `ptr` is valid. So you can use:
>>>>
>>>>     // SAFETY:
>>>>     // - `self.0` is one of `krealloc`, `vrealloc`, `kvrealloc` and thus only requires that `ptr` is
>>>>     //   NULL or valid.
>>>
>>> I'm fine adding it, but I'd like to understand why you think it's required in
>>> the safety comment here? Isn't this implicit by being the type invariant?
>>
>> You are calling a function pointer to an `unsafe` function that takes a
>> raw pointer. Without this comment it is not clear what the function
>> pointer's safety requirements are for the raw pointer parameter.
> 
> That's my point, isn't this implicitly clear by the type invariant? If needed,
> shouldn't it be:

I would argue that it is not implicitly clear, since to the reader of
just that unsafe block it's totally unclear that `self.0` has such an
invariant. They would have to read the type definition.

> // INVARIANT:
> // - `self.0` is one of [...]
> //
> // SAFETY:
> // - `ptr` is either NULL or [...]
> 
>>
>>>>     // - `ptr` is either NULL or valid by the safety requirements of this function.
>>>
>>> This is the part I already have.
>>
>> I kept it to ensure that you also keep it.

[...]

>>>>>>> +    #[inline]
>>>>>>> +    unsafe fn realloc(
>>>>>>> +        ptr: Option<NonNull<u8>>,
>>>>>>> +        layout: Layout,
>>>>>>> +        flags: Flags,
>>>>>>> +    ) -> Result<NonNull<[u8]>, AllocError> {
>>>>>>> +        // SAFETY: `ReallocFunc::call` has the same safety requirements as `Allocator::realloc`.
>>>>>>> +        unsafe { ReallocFunc::KREALLOC.call(ptr, layout, flags) }
>>>>>>> +    }
>>>>>>> +}
>>>>
>>>> Oh one more thing, I know that you already have a lot of patches in this
>>>> series, but could you split this one into two? So the first one should
>>>> introduce `ReallocFunc` and the second one add the impl for `Kmalloc`?
>>>> I managed to confuse me twice because of that :)
>>>
>>> Generally, I'm fine with that, but I'm not sure if I can avoid an intermediate
>>> compiler warning about unused code doing that.
>>
>> You can just use `#[expect(dead_code)]` for that in the intermediate
>> patches.
> 
> I usually try to avoid that, because it can be misleading when bisecting things.
> 
> If the temporarily unused code contains a bug, your bisection doesn't end up at
> this patch, but some other patch that starts using it.

I don't think it's a problem in this case, since the two patches are
directly next to each other and you're not changing existing code, just
splitting up the addition of new code.

---
Cheers,
Benno