Message-ID: <2dd02834-b2b6-4ff6-9e29-43c9d77b69e2@proton.me>
Date: Thu, 29 Aug 2024 18:32:42 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Danilo Krummrich <dakr@...nel.org>, ojeda@...nel.org, alex.gaynor@...il.com, wedsonaf@...il.com, boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com, a.hindborg@...sung.com, aliceryhl@...gle.com, akpm@...ux-foundation.org
Cc: daniel.almeida@...labora.com, faith.ekstrand@...labora.com, boris.brezillon@...labora.com, lina@...hilina.net, mcanal@...lia.com, zhiw@...dia.com, cjia@...dia.com, jhubbard@...dia.com, airlied@...hat.com, ajanulgu@...hat.com, lyude@...hat.com, linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v6 04/26] rust: alloc: implement `Allocator` for `Kmalloc`

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@...nel.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
> 

Powered by blists - more mailing lists