[PATCH v4 0/2] rust: zpool: add abstraction for zpool drivers

[PATCH v4 0/2] rust: zpool: add abstraction for zpool drivers

[Vitaly Wool]

Zpool is a common frontend for memory storage pool implementations.
These pools are typically used to store compressed memory objects,
e. g. for Zswap, the lightweight compressed cache for swap pages.

This patch provides the interface to use Zpool in Rust kernel code,
thus enabling Rust implementations of Zpool allocators for Zswap.

Co-developed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Vitaly Wool <vitaly.wool@konsulko.se>
---
Changelog:
v1 -> v2:
* reworked to stick to the existing Rust driver infrastructure
* removed raw pointers from the Rust API
v2 -> v3:
* detailed safety requirements for unsafe API functions
* removed unwrap()
* some typo corrections
v3 -> v4:
* added a working example of zpool Rust API usage in the
  documentation part
* change to Flags arranged as a separate patch
* improved safety requirements for ZpoolDriver trait
---
 bindings/bindings_helper.h |    1 
 helpers/helpers.c          |    1 
 helpers/zpool.c            |    6 +
 kernel/alloc.rs            |    5 
 kernel/lib.rs              |    2 
 kernel/zpool.rs            |  338 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 353 insertions(+)

[PATCH v4 1/2] rust: alloc: add from_raw method to Flags

[Vitaly Wool]

We need to be able to create Flags from its raw representation as u32
to properly map zpool C API into Rust. This patch adds from_raw method
to Flags and makes it crate private.

Signed-off-by: Vitaly Wool <vitaly.wool@konsulko.se>
---
 rust/kernel/alloc.rs | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
index b39c279236f5..808bd4281164 100644
--- a/rust/kernel/alloc.rs
+++ b/rust/kernel/alloc.rs
@@ -41,6 +41,11 @@
 pub struct Flags(u32);
 
 impl Flags {
+    /// Create from the raw representation
+    pub(crate) fn from_raw(f: u32) -> Self {
+        Self(f)
+    }
+
     /// Get the raw representation of this flag.
     pub(crate) fn as_raw(self) -> u32 {
         self.0
-- 
2.39.2

Re: [PATCH v4 1/2] rust: alloc: add from_raw method to Flags

[Danilo Krummrich]

On Sat Aug 23, 2025 at 3:05 PM CEST, Vitaly Wool wrote:
> diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> index b39c279236f5..808bd4281164 100644
> --- a/rust/kernel/alloc.rs
> +++ b/rust/kernel/alloc.rs
> @@ -41,6 +41,11 @@
>  pub struct Flags(u32);
>  
>  impl Flags {
> +    /// Create from the raw representation
> +    pub(crate) fn from_raw(f: u32) -> Self {

The argument shoould probably be of type bindings::gfp_t instead. However, the
alloc::Flags type itself uses u32. So, for this patch using u32 is fine. But I
think in general we should fix this up.

> +        Self(f)
> +    }
> +

I think you forgot to document that the given value must be a valid combination
of GFP flags.

Please also write full sentences and end them with a period.

[PATCH v4 2/2] rust: zpool: add abstraction for zpool drivers

[Vitaly Wool]

Zpool is a common frontend for memory storage pool implementations.
These pools are typically used to store compressed memory objects,
e. g. for Zswap, the lightweight compressed cache for swap pages.

This patch provides the interface to use Zpool in Rust kernel code,
thus enabling Rust implementations of Zpool allocators for Zswap.

Co-developed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Vitaly Wool <vitaly.wool@konsulko.se>
---
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/zpool.c            |   6 +
 rust/kernel/lib.rs              |   2 +
 rust/kernel/zpool.rs            | 338 ++++++++++++++++++++++++++++++++
 5 files changed, 348 insertions(+)
 create mode 100644 rust/helpers/zpool.c
 create mode 100644 rust/kernel/zpool.rs

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 84d60635e8a9..f0c4c454882b 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -75,6 +75,7 @@
 #include <linux/wait.h>
 #include <linux/workqueue.h>
 #include <linux/xarray.h>
+#include <linux/zpool.h>
 #include <trace/events/rust_sample.h>
 
 #if defined(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 7cf7fe95e41d..e1a7556cc700 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -51,3 +51,4 @@
 #include "wait.c"
 #include "workqueue.c"
 #include "xarray.c"
+#include "zpool.c"
diff --git a/rust/helpers/zpool.c b/rust/helpers/zpool.c
new file mode 100644
index 000000000000..71ba173f917a
--- /dev/null
+++ b/rust/helpers/zpool.c
@@ -0,0 +1,6 @@
+#include <linux/zpool.h>
+
+void rust_helper_zpool_register_driver(struct zpool_driver *driver)
+{
+	zpool_register_driver(driver);
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index ed53169e795c..165d52feeea4 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -129,6 +129,8 @@
 pub mod uaccess;
 pub mod workqueue;
 pub mod xarray;
+#[cfg(CONFIG_ZPOOL)]
+pub mod zpool;
 
 #[doc(hidden)]
 pub use bindings;
diff --git a/rust/kernel/zpool.rs b/rust/kernel/zpool.rs
new file mode 100644
index 000000000000..88b863b71c8d
--- /dev/null
+++ b/rust/kernel/zpool.rs
@@ -0,0 +1,338 @@
+use crate::{
+    bindings,
+    error::{from_result, Result},
+    kernel::alloc::Flags,
+    str::CStr,
+    types::{ForeignOwnable, Opaque},
+};
+use core::ffi::{c_int, c_uchar, c_void};
+use core::ptr::{null_mut, NonNull};
+use kernel::alloc::NumaNode;
+use kernel::driver;
+use kernel::ThisModule;
+
+/// Zpool API.
+///
+/// The [`ZpoolDriver`] trait serves as an interface for Zpool drivers implemented in Rust.
+/// Such drivers implement memory storage pools in accordance with the zpool API.
+///
+/// # Example
+///
+/// A zpool driver implementation which uses KVec of 2**n sizes, n = 6, 7, ..., PAGE_SHIFT.
+/// Every zpool object is packed into a KVec that is sufficiently large, and n (the
+/// denominator) is saved in the least significant bits of the handle, which is guaranteed
+/// to be at least 2**6 aligned by kmalloc.
+///
+/// ```
+/// use core::ptr::{NonNull, copy_nonoverlapping};
+/// use core::sync::atomic::{AtomicU64, Ordering};
+/// use kernel::alloc::{Flags, KBox, KVec, NumaNode};
+/// use kernel::page::PAGE_SHIFT;
+/// use kernel::prelude::EINVAL;
+/// use kernel::zpool::*;
+///
+/// struct MyZpool {
+///     name: &'static CStr,
+///     bytes_used: AtomicU64,
+/// }
+///
+/// struct MyZpoolDriver;
+///
+/// impl ZpoolDriver for MyZpoolDriver {
+///     type Pool = KBox<MyZpool>;
+///
+///     fn create(name: &'static CStr, gfp: Flags) -> Result<KBox<MyZpool>> {
+///         let my_pool = MyZpool { name, bytes_used: AtomicU64::new(0) };
+///         let pool = KBox::new(my_pool, gfp)?;
+///
+///         Ok(pool)
+///     }
+///
+///     fn destroy(p: KBox<MyZpool>) {
+///         drop(p);
+///     }
+///
+///     fn malloc(pool: &mut MyZpool, size: usize, gfp: Flags, _nid: NumaNode) -> Result<usize> {
+///         let mut pow: usize = 0;
+///         for n in 6..=PAGE_SHIFT {
+///             if size <= 1 << n {
+///                 pow = n;
+///                 break;
+///             }
+///         }
+///         match pow {
+///             0 => Err(EINVAL),
+///             _ => {
+///                 let vec = KVec::<u64>::with_capacity(1 << (pow - 3), gfp)?;
+///                 let (ptr, _len, _cap) = vec.into_raw_parts();
+///                 pool.bytes_used.fetch_add(1 << pow, Ordering::Relaxed);
+///                 Ok(ptr as usize | (pow - 6))
+///             }
+///         }
+///     }
+///
+///     unsafe fn free(pool: &MyZpool, handle: usize) {
+///         let n = (handle & 0x3F) + 3;
+///         let uptr = handle & !0x3F;
+///
+///         // SAFETY:
+///         // - uptr comes from handle which points to the KVec allocation from `alloc`.
+///         // - size == capacity and is coming from the first 6 bits of handle.
+///         let vec = unsafe { KVec::<u64>::from_raw_parts(uptr as *mut u64, 1 << n, 1 << n) };
+///         drop(vec);
+///         pool.bytes_used.fetch_sub(1 << (n + 3), Ordering::Relaxed);
+///     }
+///
+///     unsafe fn read_begin(_pool: &MyZpool, handle: usize) -> NonNull<u8> {
+///         let uptr = handle & !0x3F;
+///         // SAFETY: uptr points to a memory area allocated by KVec
+///         unsafe { NonNull::new_unchecked(uptr as *mut u8) }
+///     }
+///
+///     unsafe fn read_end(_pool: &MyZpool, _handle: usize, _handle_mem: NonNull<u8>) {}
+///
+///     unsafe fn write(_p: &MyZpool, handle: usize, handle_mem: NonNull<u8>, mem_len: usize) {
+///         let uptr = handle & !0x3F;
+///         // SAFETY: handle_mem is a valid non-null pointer provided by zpool, uptr points to
+///         // a KVec allocated in `malloc` and is therefore also valid.
+///         unsafe {
+///             copy_nonoverlapping(handle_mem.as_ptr().cast(), uptr as *mut c_void, mem_len)
+///         };
+///     }
+///
+///     fn total_pages(pool: &MyZpool) -> u64 {
+///         pool.bytes_used.load(Ordering::Relaxed) >> PAGE_SHIFT
+///     }
+/// }
+/// ```
+///
+pub trait ZpoolDriver {
+    /// Opaque Rust representation of `struct zpool`.
+    type Pool: ForeignOwnable;
+
+    /// Create a pool.
+    fn create(name: &'static CStr, gfp: Flags) -> Result<Self::Pool>;
+
+    /// Destroy the pool.
+    fn destroy(pool: Self::Pool);
+
+    /// Allocate an object of size `size` bytes from `pool`, with the allocation flags `gfp` and
+    /// preferred NUMA node `nid`. If the allocation is successful, an opaque handle is returned.
+    fn malloc(
+        pool: <Self::Pool as ForeignOwnable>::BorrowedMut<'_>,
+        size: usize,
+        gfp: Flags,
+        nid: NumaNode,
+    ) -> Result<usize>;
+
+    /// Free a previously allocated from the `pool` object, represented by `handle`.
+    ///
+    /// # Safety
+    ///
+    /// - `handle` must be a valid handle previously returned by `malloc`.
+    /// - `handle` must not be used any more after the call to `free`.
+    unsafe fn free(pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>, handle: usize);
+
+    /// Make all the necessary preparations for the caller to be able to read from the object
+    /// represented by `handle` and return a valid pointer to the `handle` memory to be read.
+    ///
+    /// # Safety
+    ///
+    /// - `handle` must be a valid handle previously returned by `malloc`.
+    /// - `read_end` with the same `handle` must be called for each `read_begin`.
+    unsafe fn read_begin(
+        pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>,
+        handle: usize,
+    ) -> NonNull<u8>;
+
+    /// Finish reading from a previously allocated `handle`. `handle_mem` must be the pointer
+    /// previously returned by `read_begin`.
+    ///
+    /// # Safety
+    ///
+    /// - `handle` must be a valid handle previously returned by `malloc`.
+    /// - `handle_mem` must be the pointer previously returned by `read_begin`.
+    unsafe fn read_end(
+        pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>,
+        handle: usize,
+        handle_mem: NonNull<u8>,
+    );
+
+    /// Write to the object represented by a previously allocated `handle`. `handle_mem` points
+    /// to the memory to copy data from, and `mem_len` defines the length of the data block to
+    /// be copied.
+    ///
+    /// # Safety
+    ///
+    /// - `handle` must be a valid handle previously returned by `malloc`.
+    /// - `handle_mem` must be a valid pointer to an allocated memory area.
+    /// - `handle_mem` + `mem_len` must not point outside the allocated memory area.
+    unsafe fn write(
+        pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>,
+        handle: usize,
+        handle_mem: NonNull<u8>,
+        mem_len: usize,
+    );
+
+    /// Get the number of pages used by the `pool`.
+    fn total_pages(pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>) -> u64;
+}
+
+/// An "adapter" for the registration of zpool drivers.
+pub struct Adapter<T: ZpoolDriver>(T);
+
+impl<T: ZpoolDriver> Adapter<T> {
+    extern "C" fn create_(name: *const c_uchar, gfp: u32) -> *mut c_void {
+        // SAFETY: the memory pointed to by name is guaranteed by zpool to be a valid string
+        let pool = unsafe { T::create(CStr::from_char_ptr(name), Flags::from_raw(gfp)) };
+        match pool {
+            Err(_) => null_mut(),
+            Ok(p) => T::Pool::into_foreign(p),
+        }
+    }
+    extern "C" fn destroy_(pool: *mut c_void) {
+        // SAFETY: The pointer originates from an `into_foreign` call.
+        T::destroy(unsafe { T::Pool::from_foreign(pool) })
+    }
+    extern "C" fn malloc_(
+        pool: *mut c_void,
+        size: usize,
+        gfp: u32,
+        handle: *mut usize,
+        nid: c_int,
+    ) -> c_int {
+        // SAFETY: The pointer originates from an `into_foreign` call. If `pool` is passed to
+        // `from_foreign`, then that happens in `_destroy` which will not be called during this
+        // method.
+        let pool = unsafe { T::Pool::borrow_mut(pool) };
+
+        from_result(|| {
+            let real_nid = match nid {
+                bindings::NUMA_NO_NODE => NumaNode::NO_NODE,
+                _ => NumaNode::new(nid)?,
+            };
+            let h = T::malloc(pool, size, Flags::from_raw(gfp), real_nid)?;
+            // SAFETY: handle is guaranteed to be a valid pointer by zpool.
+            unsafe { *handle = h };
+            Ok(0)
+        })
+    }
+    extern "C" fn free_(pool: *mut c_void, handle: usize) {
+        // SAFETY: The pointer originates from an `into_foreign` call. If `pool` is passed to
+        // `from_foreign`, then that happens in `_destroy` which will not be called during this
+        // method.
+        let pool = unsafe { T::Pool::borrow(pool) };
+
+        // SAFETY: the caller (zswap) guarantees that `handle` is a valid handle previously
+        // allocated by `malloc`.
+        unsafe { T::free(pool, handle) }
+    }
+    extern "C" fn obj_read_begin_(
+        pool: *mut c_void,
+        handle: usize,
+        _local_copy: *mut c_void,
+    ) -> *mut c_void {
+        // SAFETY: The pointer originates from an `into_foreign` call. If `pool` is passed to
+        // `from_foreign`, then that happens in `_destroy` which will not be called during this
+        // method.
+        let pool = unsafe { T::Pool::borrow(pool) };
+
+        // SAFETY: the caller (zswap) guarantees that `handle` is a valid handle previously
+        // allocated by `malloc`.
+        let non_null_ptr = unsafe { T::read_begin(pool, handle) };
+        non_null_ptr.as_ptr().cast()
+    }
+    extern "C" fn obj_read_end_(pool: *mut c_void, handle: usize, handle_mem: *mut c_void) {
+        // SAFETY: The pointer originates from an `into_foreign` call. If `pool` is passed to
+        // `from_foreign`, then that happens in `_destroy` which will not be called during this
+        // method.
+        let pool = unsafe { T::Pool::borrow(pool) };
+
+        // SAFETY: handle_mem is guaranteed to be non-null by the caller (zswap)
+        let handle_mem_ptr = unsafe { NonNull::new_unchecked(handle_mem.cast()) };
+
+        // SAFETY: the caller (zswap) guarantees that `handle` is a valid handle previously
+        // allocated by `malloc`.
+        unsafe { T::read_end(pool, handle, handle_mem_ptr) }
+    }
+    extern "C" fn obj_write_(
+        pool: *mut c_void,
+        handle: usize,
+        handle_mem: *mut c_void,
+        mem_len: usize,
+    ) {
+        // SAFETY: The pointer originates from an `into_foreign` call. If `pool` is passed to
+        // `from_foreign`, then that happens in `_destroy` which will not be called during this
+        // method.
+        let pool = unsafe { T::Pool::borrow(pool) };
+
+        // SAFETY: handle_mem is guaranteed to be non-null by the caller (zswap)
+        let handle_mem_ptr = unsafe { NonNull::new_unchecked(handle_mem.cast()) };
+
+        // SAFETY: the caller (zswap) guarantees that `handle` is a valid handle previously
+        // allocated by `malloc`.
+        unsafe {
+            T::write(pool, handle, handle_mem_ptr, mem_len);
+        }
+    }
+    extern "C" fn total_pages_(pool: *mut c_void) -> u64 {
+        // SAFETY: The pointer originates from an `into_foreign` call. If `pool` is passed to
+        // `from_foreign`, then that happens in `_destroy` which will not be called during this
+        // method.
+        let pool = unsafe { T::Pool::borrow(pool) };
+        T::total_pages(pool)
+    }
+}
+
+// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid
+// because preceding call to `register` never fails for zpool.
+unsafe impl<T: ZpoolDriver + 'static> driver::RegistrationOps for Adapter<T> {
+    type RegType = bindings::zpool_driver;
+
+    unsafe fn register(
+        pdrv: &Opaque<Self::RegType>,
+        name: &'static CStr,
+        _module: &'static ThisModule,
+    ) -> Result {
+        // SAFETY: It's safe to set the fields of `struct zpool_driver` on initialization.
+        unsafe {
+            (*(pdrv.get())).type_ = name.as_char_ptr().cast_mut();
+            (*(pdrv.get())).create = Some(Self::create_);
+            (*(pdrv.get())).destroy = Some(Self::destroy_);
+            (*(pdrv.get())).malloc = Some(Self::malloc_);
+            (*(pdrv.get())).free = Some(Self::free_);
+            (*(pdrv.get())).obj_read_begin = Some(Self::obj_read_begin_);
+            (*(pdrv.get())).obj_read_end = Some(Self::obj_read_end_);
+            (*(pdrv.get())).obj_write = Some(Self::obj_write_);
+            (*(pdrv.get())).total_pages = Some(Self::total_pages_);
+
+            bindings::zpool_register_driver(pdrv.get());
+        }
+        Ok(())
+    }
+    unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
+        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+        unsafe { bindings::zpool_unregister_driver(pdrv.get()) };
+    }
+}
+
+/// Declares a kernel module that exposes a zpool driver (i. e. an implementation of the zpool
+/// API).
+///
+/// # Examples
+///
+///```ignore
+/// kernel::module_zpool_driver! {
+///     type: MyDriver,
+///     name: "Module name",
+///     authors: ["Author name"],
+///     description: "Description",
+///     license: "GPL",
+/// }
+///```
+#[macro_export]
+macro_rules! module_zpool_driver {
+($($f:tt)*) => {
+    $crate::module_driver!(<T>, $crate::zpool::Adapter<T>, { $($f)* });
+};
+}
-- 
2.39.2

Re: [PATCH v4 2/2] rust: zpool: add abstraction for zpool drivers

[Danilo Krummrich]

On Sat Aug 23, 2025 at 3:05 PM CEST, Vitaly Wool wrote:
> +/// Zpool API.
> +///
> +/// The [`ZpoolDriver`] trait serves as an interface for Zpool drivers implemented in Rust.
> +/// Such drivers implement memory storage pools in accordance with the zpool API.
> +///
> +/// # Example
> +///
> +/// A zpool driver implementation which uses KVec of 2**n sizes, n = 6, 7, ..., PAGE_SHIFT.
> +/// Every zpool object is packed into a KVec that is sufficiently large, and n (the
> +/// denominator) is saved in the least significant bits of the handle, which is guaranteed
> +/// to be at least 2**6 aligned by kmalloc.
> +///
> +/// ```
> +/// use core::ptr::{NonNull, copy_nonoverlapping};
> +/// use core::sync::atomic::{AtomicU64, Ordering};
> +/// use kernel::alloc::{Flags, KBox, KVec, NumaNode};
> +/// use kernel::page::PAGE_SHIFT;
> +/// use kernel::prelude::EINVAL;
> +/// use kernel::zpool::*;
> +///
> +/// struct MyZpool {
> +///     name: &'static CStr,
> +///     bytes_used: AtomicU64,
> +/// }
> +///
> +/// struct MyZpoolDriver;
> +///
> +/// impl ZpoolDriver for MyZpoolDriver {
> +///     type Pool = KBox<MyZpool>;
> +///
> +///     fn create(name: &'static CStr, gfp: Flags) -> Result<KBox<MyZpool>> {
> +///         let my_pool = MyZpool { name, bytes_used: AtomicU64::new(0) };
> +///         let pool = KBox::new(my_pool, gfp)?;
> +///
> +///         Ok(pool)
> +///     }
> +///
> +///     fn destroy(p: KBox<MyZpool>) {
> +///         drop(p);
> +///     }
> +///
> +///     fn malloc(pool: &mut MyZpool, size: usize, gfp: Flags, _nid: NumaNode) -> Result<usize> {
> +///         let mut pow: usize = 0;
> +///         for n in 6..=PAGE_SHIFT {
> +///             if size <= 1 << n {
> +///                 pow = n;
> +///                 break;
> +///             }
> +///         }

Why not just use next_power_of_two()? I think the same logic could also be
achieved with

	size.next_power_of_two().trailing_zeros().max(6).min(PAGE_SHIFT)

> +///         match pow {
> +///             0 => Err(EINVAL),
> +///             _ => {
> +///                 let vec = KVec::<u64>::with_capacity(1 << (pow - 3), gfp)?;

Why use u64 and 1 << (pow - 3), rather than simply u8 and 1 << pow?

(Btw. you could also just use VBox<u8; PAGE_SIZE>::new_uninit() for all
allocations to keep the example simple.)

> +///                 let (ptr, _len, _cap) = vec.into_raw_parts();
> +///                 pool.bytes_used.fetch_add(1 << pow, Ordering::Relaxed);
> +///                 Ok(ptr as usize | (pow - 6))
> +///             }
> +///         }
> +///     }
> +///
> +///     unsafe fn free(pool: &MyZpool, handle: usize) {
> +///         let n = (handle & 0x3F) + 3;
> +///         let uptr = handle & !0x3F;
> +///
> +///         // SAFETY:
> +///         // - uptr comes from handle which points to the KVec allocation from `alloc`.

That's not true, you modified the pointer you got from KVec. Please explain why
it is always safe to use lower 6 bits for something else.

What does "`alloc`" refer to?

NIT: `uptr`, `KVec`

> +///         // - size == capacity and is coming from the first 6 bits of handle.
> +///         let vec = unsafe { KVec::<u64>::from_raw_parts(uptr as *mut u64, 1 << n, 1 << n) };

Why do you set the length (not the capacity) of the Vector to 1 << n? I think
technically it doesn't matter, but you should explain that in the safety
comment.

> +///         drop(vec);
> +///         pool.bytes_used.fetch_sub(1 << (n + 3), Ordering::Relaxed);
> +///     }
> +///
> +///     unsafe fn read_begin(_pool: &MyZpool, handle: usize) -> NonNull<u8> {
> +///         let uptr = handle & !0x3F;
> +///         // SAFETY: uptr points to a memory area allocated by KVec

Please use markdown and end sentences with a period. (Applies to the entire
file.)

> +///         unsafe { NonNull::new_unchecked(uptr as *mut u8) }
> +///     }
> +///
> +///     unsafe fn read_end(_pool: &MyZpool, _handle: usize, _handle_mem: NonNull<u8>) {}
> +///
> +///     unsafe fn write(_p: &MyZpool, handle: usize, handle_mem: NonNull<u8>, mem_len: usize) {
> +///         let uptr = handle & !0x3F;
> +///         // SAFETY: handle_mem is a valid non-null pointer provided by zpool, uptr points to
> +///         // a KVec allocated in `malloc` and is therefore also valid.
> +///         unsafe {
> +///             copy_nonoverlapping(handle_mem.as_ptr().cast(), uptr as *mut c_void, mem_len)
> +///         };
> +///     }
> +///
> +///     fn total_pages(pool: &MyZpool) -> u64 {
> +///         pool.bytes_used.load(Ordering::Relaxed) >> PAGE_SHIFT

I'm not sure what the semantic of this function is; the documentation says
"Get the number of pages used by the `pool`".

However, given that you give out allocations from a kmalloc() bucket in
malloc(), this pool might be backed by more pages than what you calculate here.

So, what is done here is calculating the number of pages you could fill with
the memory that is kept around, but not the number of backing pages you consume
memory from.

Using VBox<u8; PAGE_SIZE>::new_uninit() for all allocations might simplify this.

> +///     }
> +/// }
> +/// ```
> +///
> +pub trait ZpoolDriver {
> +    /// Opaque Rust representation of `struct zpool`.
> +    type Pool: ForeignOwnable;
> +
> +    /// Create a pool.
> +    fn create(name: &'static CStr, gfp: Flags) -> Result<Self::Pool>;
> +
> +    /// Destroy the pool.
> +    fn destroy(pool: Self::Pool);
> +
> +    /// Allocate an object of size `size` bytes from `pool`, with the allocation flags `gfp` and

"of `size` bytes"

> +    /// preferred NUMA node `nid`. If the allocation is successful, an opaque handle is returned.
> +    fn malloc(
> +        pool: <Self::Pool as ForeignOwnable>::BorrowedMut<'_>,
> +        size: usize,
> +        gfp: Flags,
> +        nid: NumaNode,
> +    ) -> Result<usize>;
> +
> +    /// Free a previously allocated from the `pool` object, represented by `handle`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - `handle` must be a valid handle previously returned by `malloc`.
> +    /// - `handle` must not be used any more after the call to `free`.
> +    unsafe fn free(pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>, handle: usize);
> +
> +    /// Make all the necessary preparations for the caller to be able to read from the object
> +    /// represented by `handle` and return a valid pointer to the `handle` memory to be read.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - `handle` must be a valid handle previously returned by `malloc`.
> +    /// - `read_end` with the same `handle` must be called for each `read_begin`.

What can potentially happen if we don't? I.e. how is this different from
malloc()?

> +    unsafe fn read_begin(
> +        pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>,
> +        handle: usize,
> +    ) -> NonNull<u8>;
> +
> +    /// Finish reading from a previously allocated `handle`. `handle_mem` must be the pointer
> +    /// previously returned by `read_begin`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - `handle` must be a valid handle previously returned by `malloc`.
> +    /// - `handle_mem` must be the pointer previously returned by `read_begin`.
> +    unsafe fn read_end(
> +        pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>,
> +        handle: usize,
> +        handle_mem: NonNull<u8>,
> +    );
> +
> +    /// Write to the object represented by a previously allocated `handle`. `handle_mem` points
> +    /// to the memory to copy data from, and `mem_len` defines the length of the data block to
> +    /// be copied.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - `handle` must be a valid handle previously returned by `malloc`.
> +    /// - `handle_mem` must be a valid pointer to an allocated memory area.

"must be a valid pointer into the allocated memory aread represented by
`handle`"

> +    /// - `handle_mem` + `mem_len` must not point outside the allocated memory area.
> +    unsafe fn write(
> +        pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>,
> +        handle: usize,
> +        handle_mem: NonNull<u8>,
> +        mem_len: usize,
> +    );
> +
> +    /// Get the number of pages used by the `pool`.
> +    fn total_pages(pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>) -> u64;
> +}
> +
> +/// An "adapter" for the registration of zpool drivers.
> +pub struct Adapter<T: ZpoolDriver>(T);
> +
> +impl<T: ZpoolDriver> Adapter<T> {
> +    extern "C" fn create_(name: *const c_uchar, gfp: u32) -> *mut c_void {
> +        // SAFETY: the memory pointed to by name is guaranteed by zpool to be a valid string

What about the lifetime of the string? In the abstraction you assume 'static,
how is this guaranteed?

> +        let pool = unsafe { T::create(CStr::from_char_ptr(name), Flags::from_raw(gfp)) };
> +        match pool {
> +            Err(_) => null_mut(),
> +            Ok(p) => T::Pool::into_foreign(p),
> +        }
> +    }

Please add an empty line in between function definitions.

> +    extern "C" fn destroy_(pool: *mut c_void) {
> +        // SAFETY: The pointer originates from an `into_foreign` call.
> +        T::destroy(unsafe { T::Pool::from_foreign(pool) })
> +    }
> +    extern "C" fn malloc_(
> +        pool: *mut c_void,
> +        size: usize,
> +        gfp: u32,
> +        handle: *mut usize,
> +        nid: c_int,
> +    ) -> c_int {
> +        // SAFETY: The pointer originates from an `into_foreign` call. If `pool` is passed to
> +        // `from_foreign`, then that happens in `_destroy` which will not be called during this
> +        // method.
> +        let pool = unsafe { T::Pool::borrow_mut(pool) };

Wait, can't this happen concurrently to all the other functions that borrow the
pool? This would be undefined behavior, no?

> +        from_result(|| {
> +            let real_nid = match nid {
> +                bindings::NUMA_NO_NODE => NumaNode::NO_NODE,
> +                _ => NumaNode::new(nid)?,
> +            };
> +            let h = T::malloc(pool, size, Flags::from_raw(gfp), real_nid)?;
> +            // SAFETY: handle is guaranteed to be a valid pointer by zpool.
> +            unsafe { *handle = h };
> +            Ok(0)
> +        })
> +    }
> +    extern "C" fn free_(pool: *mut c_void, handle: usize) {
> +        // SAFETY: The pointer originates from an `into_foreign` call. If `pool` is passed to
> +        // `from_foreign`, then that happens in `_destroy` which will not be called during this
> +        // method.
> +        let pool = unsafe { T::Pool::borrow(pool) };
> +
> +        // SAFETY: the caller (zswap) guarantees that `handle` is a valid handle previously

Why does this mention zwap here and in the other functions below?

> +        // allocated by `malloc`.
> +        unsafe { T::free(pool, handle) }
> +    }

Re: [PATCH v4 2/2] rust: zpool: add abstraction for zpool drivers

[Vitaly Wool]

On 8/26/25 14:20, Danilo Krummrich wrote:
> On Sat Aug 23, 2025 at 3:05 PM CEST, Vitaly Wool wrote:
>> +/// Zpool API.
>> +///
>> +/// The [`ZpoolDriver`] trait serves as an interface for Zpool drivers implemented in Rust.
>> +/// Such drivers implement memory storage pools in accordance with the zpool API.
>> +///
>> +/// # Example
>> +///
>> +/// A zpool driver implementation which uses KVec of 2**n sizes, n = 6, 7, ..., PAGE_SHIFT.
>> +/// Every zpool object is packed into a KVec that is sufficiently large, and n (the
>> +/// denominator) is saved in the least significant bits of the handle, which is guaranteed
>> +/// to be at least 2**6 aligned by kmalloc.
>> +///
>> +/// ```
>> +/// use core::ptr::{NonNull, copy_nonoverlapping};
>> +/// use core::sync::atomic::{AtomicU64, Ordering};
>> +/// use kernel::alloc::{Flags, KBox, KVec, NumaNode};
>> +/// use kernel::page::PAGE_SHIFT;
>> +/// use kernel::prelude::EINVAL;
>> +/// use kernel::zpool::*;
>> +///
>> +/// struct MyZpool {
>> +///     name: &'static CStr,
>> +///     bytes_used: AtomicU64,
>> +/// }
>> +///
>> +/// struct MyZpoolDriver;
>> +///
>> +/// impl ZpoolDriver for MyZpoolDriver {
>> +///     type Pool = KBox<MyZpool>;
>> +///
>> +///     fn create(name: &'static CStr, gfp: Flags) -> Result<KBox<MyZpool>> {
>> +///         let my_pool = MyZpool { name, bytes_used: AtomicU64::new(0) };
>> +///         let pool = KBox::new(my_pool, gfp)?;
>> +///
>> +///         Ok(pool)
>> +///     }
>> +///
>> +///     fn destroy(p: KBox<MyZpool>) {
>> +///         drop(p);
>> +///     }
>> +///
>> +///     fn malloc(pool: &mut MyZpool, size: usize, gfp: Flags, _nid: NumaNode) -> Result<usize> {
>> +///         let mut pow: usize = 0;
>> +///         for n in 6..=PAGE_SHIFT {
>> +///             if size <= 1 << n {
>> +///                 pow = n;
>> +///                 break;
>> +///             }
>> +///         }
> 
> Why not just use next_power_of_two()? I think the same logic could also be
> achieved with
> 
> 	size.next_power_of_two().trailing_zeros().max(6).min(PAGE_SHIFT)

It indeed can, thanks :)

>> +///         match pow {
>> +///             0 => Err(EINVAL),
>> +///             _ => {
>> +///                 let vec = KVec::<u64>::with_capacity(1 << (pow - 3), gfp)?;
> 
> Why use u64 and 1 << (pow - 3), rather than simply u8 and 1 << pow?
> 
> (Btw. you could also just use VBox<u8; PAGE_SIZE>::new_uninit() for all
> allocations to keep the example simple.)

Right, that fixation on u64 doesn't help at all here.

>> +///                 let (ptr, _len, _cap) = vec.into_raw_parts();
>> +///                 pool.bytes_used.fetch_add(1 << pow, Ordering::Relaxed);
>> +///                 Ok(ptr as usize | (pow - 6))
>> +///             }
>> +///         }
>> +///     }
>> +///
>> +///     unsafe fn free(pool: &MyZpool, handle: usize) {
>> +///         let n = (handle & 0x3F) + 3;
>> +///         let uptr = handle & !0x3F;
>> +///
>> +///         // SAFETY:
>> +///         // - uptr comes from handle which points to the KVec allocation from `alloc`.
> 
> That's not true, you modified the pointer you got from KVec. Please explain why
> it is always safe to use lower 6 bits for something else.
> 
> What does "`alloc`" refer to?

It refers to the alloc function we implement in this toy backend for 
ZpoolDriver trait.

> NIT: `uptr`, `KVec`
> 
>> +///         // - size == capacity and is coming from the first 6 bits of handle.
>> +///         let vec = unsafe { KVec::<u64>::from_raw_parts(uptr as *mut u64, 1 << n, 1 << n) };
> 
> Why do you set the length (not the capacity) of the Vector to 1 << n? I think
> technically it doesn't matter, but you should explain that in the safety
> comment.

Would the following work: "we know the capacity of this vector and it is 
1 << n, we set the length to the same value to be deterministic, but the 
actual length of vec doesn't matter because we're dropping it right here 
anyway"?

>> +///         drop(vec);
>> +///         pool.bytes_used.fetch_sub(1 << (n + 3), Ordering::Relaxed);
>> +///     }
>> +///
>> +///     unsafe fn read_begin(_pool: &MyZpool, handle: usize) -> NonNull<u8> {
>> +///         let uptr = handle & !0x3F;
>> +///         // SAFETY: uptr points to a memory area allocated by KVec
> 
> Please use markdown and end sentences with a period. (Applies to the entire
> file.)
> 
>> +///         unsafe { NonNull::new_unchecked(uptr as *mut u8) }
>> +///     }
>> +///
>> +///     unsafe fn read_end(_pool: &MyZpool, _handle: usize, _handle_mem: NonNull<u8>) {}
>> +///
>> +///     unsafe fn write(_p: &MyZpool, handle: usize, handle_mem: NonNull<u8>, mem_len: usize) {
>> +///         let uptr = handle & !0x3F;
>> +///         // SAFETY: handle_mem is a valid non-null pointer provided by zpool, uptr points to
>> +///         // a KVec allocated in `malloc` and is therefore also valid.
>> +///         unsafe {
>> +///             copy_nonoverlapping(handle_mem.as_ptr().cast(), uptr as *mut c_void, mem_len)
>> +///         };
>> +///     }
>> +///
>> +///     fn total_pages(pool: &MyZpool) -> u64 {
>> +///         pool.bytes_used.load(Ordering::Relaxed) >> PAGE_SHIFT
> 
> I'm not sure what the semantic of this function is; the documentation says
> "Get the number of pages used by the `pool`".
> 
> However, given that you give out allocations from a kmalloc() bucket in
> malloc(), this pool might be backed by more pages than what you calculate here.

Well, maybe I need to add an explicit comment about it here, but the 
idea is that with the SLUB allocator, you have kmalloc-64, kmalloc-128, 
kmalloc-256 etc. caches which will be used to manage requests for up to 
64, 128, 256, ... bytes respectively, and in that case the calculations 
are correct. FWIW I smoke tested this allocator and the actual numbers 
seem to be consistent with these calculations.

> So, what is done here is calculating the number of pages you could fill with
> the memory that is kept around, but not the number of backing pages you consume
> memory from.
> 
> Using VBox<u8; PAGE_SIZE>::new_uninit() for all allocations might simplify this.
> 
>> +///     }
>> +/// }
>> +/// ```
>> +///
>> +pub trait ZpoolDriver {
>> +    /// Opaque Rust representation of `struct zpool`.
>> +    type Pool: ForeignOwnable;
>> +
>> +    /// Create a pool.
>> +    fn create(name: &'static CStr, gfp: Flags) -> Result<Self::Pool>;
>> +
>> +    /// Destroy the pool.
>> +    fn destroy(pool: Self::Pool);
>> +
>> +    /// Allocate an object of size `size` bytes from `pool`, with the allocation flags `gfp` and
> 
> "of `size` bytes"
> 
>> +    /// preferred NUMA node `nid`. If the allocation is successful, an opaque handle is returned.
>> +    fn malloc(
>> +        pool: <Self::Pool as ForeignOwnable>::BorrowedMut<'_>,
>> +        size: usize,
>> +        gfp: Flags,
>> +        nid: NumaNode,
>> +    ) -> Result<usize>;
>> +
>> +    /// Free a previously allocated from the `pool` object, represented by `handle`.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// - `handle` must be a valid handle previously returned by `malloc`.
>> +    /// - `handle` must not be used any more after the call to `free`.
>> +    unsafe fn free(pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>, handle: usize);
>> +
>> +    /// Make all the necessary preparations for the caller to be able to read from the object
>> +    /// represented by `handle` and return a valid pointer to the `handle` memory to be read.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// - `handle` must be a valid handle previously returned by `malloc`.
>> +    /// - `read_end` with the same `handle` must be called for each `read_begin`.
> 
> What can potentially happen if we don't? I.e. how is this different from
> malloc()?

Here the idea is that if read_begin() has some sort of extra mapping 
involved (like, doing kmap_atomic() on some weird memory address) for 
the caller to be able to read directly from the returned pointer, 
read_end() must clean that mapping up.

>> +    unsafe fn read_begin(
>> +        pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>,
>> +        handle: usize,
>> +    ) -> NonNull<u8>;
>> +
>> +    /// Finish reading from a previously allocated `handle`. `handle_mem` must be the pointer
>> +    /// previously returned by `read_begin`.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// - `handle` must be a valid handle previously returned by `malloc`.
>> +    /// - `handle_mem` must be the pointer previously returned by `read_begin`.
>> +    unsafe fn read_end(
>> +        pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>,
>> +        handle: usize,
>> +        handle_mem: NonNull<u8>,
>> +    );
>> +
>> +    /// Write to the object represented by a previously allocated `handle`. `handle_mem` points
>> +    /// to the memory to copy data from, and `mem_len` defines the length of the data block to
>> +    /// be copied.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// - `handle` must be a valid handle previously returned by `malloc`.
>> +    /// - `handle_mem` must be a valid pointer to an allocated memory area.
> 
> "must be a valid pointer into the allocated memory aread represented by
> `handle`"
> 
>> +    /// - `handle_mem` + `mem_len` must not point outside the allocated memory area.
>> +    unsafe fn write(
>> +        pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>,
>> +        handle: usize,
>> +        handle_mem: NonNull<u8>,
>> +        mem_len: usize,
>> +    );
>> +
>> +    /// Get the number of pages used by the `pool`.
>> +    fn total_pages(pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>) -> u64;
>> +}
>> +
>> +/// An "adapter" for the registration of zpool drivers.
>> +pub struct Adapter<T: ZpoolDriver>(T);
>> +
>> +impl<T: ZpoolDriver> Adapter<T> {
>> +    extern "C" fn create_(name: *const c_uchar, gfp: u32) -> *mut c_void {
>> +        // SAFETY: the memory pointed to by name is guaranteed by zpool to be a valid string
> 
> What about the lifetime of the string? In the abstraction you assume 'static,
> how is this guaranteed?

Actually it isn't, thanks for finding this.

>> +        let pool = unsafe { T::create(CStr::from_char_ptr(name), Flags::from_raw(gfp)) };
>> +        match pool {
>> +            Err(_) => null_mut(),
>> +            Ok(p) => T::Pool::into_foreign(p),
>> +        }
>> +    }
> 
> Please add an empty line in between function definitions.
> 
>> +    extern "C" fn destroy_(pool: *mut c_void) {
>> +        // SAFETY: The pointer originates from an `into_foreign` call.
>> +        T::destroy(unsafe { T::Pool::from_foreign(pool) })
>> +    }
>> +    extern "C" fn malloc_(
>> +        pool: *mut c_void,
>> +        size: usize,
>> +        gfp: u32,
>> +        handle: *mut usize,
>> +        nid: c_int,
>> +    ) -> c_int {
>> +        // SAFETY: The pointer originates from an `into_foreign` call. If `pool` is passed to
>> +        // `from_foreign`, then that happens in `_destroy` which will not be called during this
>> +        // method.
>> +        let pool = unsafe { T::Pool::borrow_mut(pool) };
> 
> Wait, can't this happen concurrently to all the other functions that borrow the
> pool? This would be undefined behavior, no?

Theoretically, yes, but since pool is actually Box<T>, it's only the 
inner T that is mutable.

Anyway, the only reason for malloc() to require a mutable reference is 
that the backend implementation *may* use RBTree::cursor_lower_bound() 
which requires a mutable reference of the tree.

Would it be okay if I
* change the Zpool API so that malloc takes an immutable reference
* extend the RBTree API with a cursor_lower_bound analog which doesn't 
require a mutable tree?

>> +        from_result(|| {
>> +            let real_nid = match nid {
>> +                bindings::NUMA_NO_NODE => NumaNode::NO_NODE,
>> +                _ => NumaNode::new(nid)?,
>> +            };
>> +            let h = T::malloc(pool, size, Flags::from_raw(gfp), real_nid)?;
>> +            // SAFETY: handle is guaranteed to be a valid pointer by zpool.
>> +            unsafe { *handle = h };
>> +            Ok(0)
>> +        })
>> +    }
>> +    extern "C" fn free_(pool: *mut c_void, handle: usize) {
>> +        // SAFETY: The pointer originates from an `into_foreign` call. If `pool` is passed to
>> +        // `from_foreign`, then that happens in `_destroy` which will not be called during this
>> +        // method.
>> +        let pool = unsafe { T::Pool::borrow(pool) };
>> +
>> +        // SAFETY: the caller (zswap) guarantees that `handle` is a valid handle previously
> 
> Why does this mention zwap here and in the other functions below?

Should have been "(e. g. zswap)".

>> +        // allocated by `malloc`.
>> +        unsafe { T::free(pool, handle) }
>> +    }
> 

Re: [PATCH v4 2/2] rust: zpool: add abstraction for zpool drivers

[Vitaly Wool]

<snip>
>>> +    extern "C" fn destroy_(pool: *mut c_void) {
>>> +        // SAFETY: The pointer originates from an `into_foreign` call.
>>> +        T::destroy(unsafe { T::Pool::from_foreign(pool) })
>>> +    }
>>> +    extern "C" fn malloc_(
>>> +        pool: *mut c_void,
>>> +        size: usize,
>>> +        gfp: u32,
>>> +        handle: *mut usize,
>>> +        nid: c_int,
>>> +    ) -> c_int {
>>> +        // SAFETY: The pointer originates from an `into_foreign` 
>>> call. If `pool` is passed to
>>> +        // `from_foreign`, then that happens in `_destroy` which 
>>> will not be called during this
>>> +        // method.
>>> +        let pool = unsafe { T::Pool::borrow_mut(pool) };
>>
>> Wait, can't this happen concurrently to all the other functions that 
>> borrow the
>> pool? This would be undefined behavior, no?
> 
> Theoretically, yes, but since pool is actually Box<T>, it's only the 
> inner T that is mutable.
> 
> Anyway, the only reason for malloc() to require a mutable reference is 
> that the backend implementation *may* use RBTree::cursor_lower_bound() 
> which requires a mutable reference of the tree.
> 
> Would it be okay if I
> * change the Zpool API so that malloc takes an immutable reference
> * extend the RBTree API with a cursor_lower_bound analog which doesn't 
> require a mutable tree?

As a matter of fact, the RBTree change may be postponed and submitted 
together with zblock, it is not relevant until then.

~Vitaly

Re: [PATCH v4 2/2] rust: zpool: add abstraction for zpool drivers

[Benno Lossin]

On Sat Aug 23, 2025 at 3:05 PM CEST, Vitaly Wool wrote:
> +pub trait ZpoolDriver {
> +    /// Opaque Rust representation of `struct zpool`.
> +    type Pool: ForeignOwnable;

I think this is the same question that Danilo asked a few versions ago,
but why do we need this? Why can't we just use `Self` instead?

> +
> +    /// Create a pool.
> +    fn create(name: &'static CStr, gfp: Flags) -> Result<Self::Pool>;
> +
> +    /// Destroy the pool.
> +    fn destroy(pool: Self::Pool);

This should just be done via the normal `Drop` trait?

---
Cheers,
Benno

> +
> +    /// Allocate an object of size `size` bytes from `pool`, with the allocation flags `gfp` and
> +    /// preferred NUMA node `nid`. If the allocation is successful, an opaque handle is returned.
> +    fn malloc(
> +        pool: <Self::Pool as ForeignOwnable>::BorrowedMut<'_>,
> +        size: usize,
> +        gfp: Flags,
> +        nid: NumaNode,
> +    ) -> Result<usize>;

Re: [PATCH v4 2/2] rust: zpool: add abstraction for zpool drivers

[Vitaly Wool]

> On Aug 26, 2025, at 7:02 PM, Benno Lossin <lossin@kernel.org> wrote:
> 
> On Sat Aug 23, 2025 at 3:05 PM CEST, Vitaly Wool wrote:
>> +pub trait ZpoolDriver {
>> +    /// Opaque Rust representation of `struct zpool`.
>> +    type Pool: ForeignOwnable;
> 
> I think this is the same question that Danilo asked a few versions ago,
> but why do we need this? Why can't we just use `Self` instead?

It’s convenient to use it in the backend implementation, like in the toy example supplied in the documentation part:

+/// struct MyZpool {
+///     name: &'static CStr,
+///     bytes_used: AtomicU64,
+/// }
…
+/// impl ZpoolDriver for MyZpoolDriver {
+///     type Pool = KBox<MyZpool>;

Does that make sense?
 
> 
>> +
>> +    /// Create a pool.
>> +    fn create(name: &'static CStr, gfp: Flags) -> Result<Self::Pool>;
>> +
>> +    /// Destroy the pool.
>> +    fn destroy(pool: Self::Pool);
> 
> This should just be done via the normal `Drop` trait?

Let me check if I’m getting you right here. I take what you are suggesting is that we require that Pool implements Drop trait and then just do something like:

    extern "C" fn destroy_(pool: *mut c_void) {
        // SAFETY: The pointer originates from an `into_foreign` call.
        unsafe { drop(T::Pool::from_foreign(pool)) }
    }

Is that understanding correct?

~Vitaly


> 
> ---
> Cheers,
> Benno
> 
>> +
>> +    /// Allocate an object of size `size` bytes from `pool`, with the allocation flags `gfp` and
>> +    /// preferred NUMA node `nid`. If the allocation is successful, an opaque handle is returned.
>> +    fn malloc(
>> +        pool: <Self::Pool as ForeignOwnable>::BorrowedMut<'_>,
>> +        size: usize,
>> +        gfp: Flags,
>> +        nid: NumaNode,
>> +    ) -> Result<usize>;
> 

Re: [PATCH v4 2/2] rust: zpool: add abstraction for zpool drivers

[Benno Lossin]

On Wed Aug 27, 2025 at 4:24 PM CEST, Vitaly Wool wrote:
>
>
>> On Aug 26, 2025, at 7:02 PM, Benno Lossin <lossin@kernel.org> wrote:
>> 
>> On Sat Aug 23, 2025 at 3:05 PM CEST, Vitaly Wool wrote:
>>> +pub trait ZpoolDriver {
>>> +    /// Opaque Rust representation of `struct zpool`.
>>> +    type Pool: ForeignOwnable;
>> 
>> I think this is the same question that Danilo asked a few versions ago,
>> but why do we need this? Why can't we just use `Self` instead?
>
> It’s convenient to use it in the backend implementation, like in the toy example supplied in the documentation part:
>
> +/// struct MyZpool {
> +///     name: &'static CStr,
> +///     bytes_used: AtomicU64,
> +/// }
> …
> +/// impl ZpoolDriver for MyZpoolDriver {
> +///     type Pool = KBox<MyZpool>;
>
> Does that make sense?

No, why can't it just be like this:

    struct MyZpool {
        name: &'static CStr,
        bytes_used: AtomicU64,
    }
    
    struct MyZpoolDriver;
    
    impl ZpoolDriver for MyZpoolDriver {
        type Error = Infallible;
    
        fn create(name: &'static CStr) -> impl PinInit<Self, Self::Error> {
            MyZpool { name, bytes_used: AtomicU64::new(0) }
        }
    
        fn malloc(&mut self, size: usize, gfp: Flags, _nid: NumaNode) -> Result<usize> {
            let mut pow: usize = 0;
            for n in 6..=PAGE_SHIFT {
                if size <= 1 << n {
                    pow = n;
                    break;
                }
            }
            match pow {
                0 => Err(EINVAL),
                _ => {
                    let vec = KVec::<u64>::with_capacity(1 << (pow - 3), gfp)?;
                    let (ptr, _len, _cap) = vec.into_raw_parts();
                    self.bytes_used.fetch_add(1 << pow, Ordering::Relaxed);
                    Ok(ptr as usize | (pow - 6))
                }
            }
        }
    
        unsafe fn free(&self, handle: usize) {
            let n = (handle & 0x3F) + 3;
            let uptr = handle & !0x3F;
    
            // SAFETY:
            // - uptr comes from handle which points to the KVec allocation from `alloc`.
            // - size == capacity and is coming from the first 6 bits of handle.
            let vec = unsafe { KVec::<u64>::from_raw_parts(uptr as *mut u64, 1 << n, 1 << n) };
            drop(vec);
            self.bytes_used.fetch_sub(1 << (n + 3), Ordering::Relaxed);
        }
    
        unsafe fn read_begin(&self, handle: usize) -> NonNull<u8> {
            let uptr = handle & !0x3F;
            // SAFETY: uptr points to a memory area allocated by KVec
            unsafe { NonNull::new_unchecked(uptr as *mut u8) }
        }
    
        unsafe fn read_end(&self, _handle: usize, _handle_mem: NonNull<u8>) {}
    
        unsafe fn write(&self, handle: usize, handle_mem: NonNull<u8>, mem_len: usize) {
            let uptr = handle & !0x3F;
            // SAFETY: handle_mem is a valid non-null pointer provided by zpool, uptr points to
            // a KVec allocated in `malloc` and is therefore also valid.
            unsafe {
                copy_nonoverlapping(handle_mem.as_ptr().cast(), uptr as *mut c_void, mem_len)
            };
        }
    
        fn total_pages(&self) -> u64 {
            self.bytes_used.load(Ordering::Relaxed) >> PAGE_SHIFT
        }
    }

Also using a `usize` as a handle seems like a bad idea. Use a newtype
wrapper of usize instead. You can also not implement `Copy` and thus get
rid of one of the safety requirements of the `free` function. Not sure
if we can remove the other one as well using more type system magic, but
we could try.


>>> +
>>> +    /// Create a pool.
>>> +    fn create(name: &'static CStr, gfp: Flags) -> Result<Self::Pool>;
>>> +
>>> +    /// Destroy the pool.
>>> +    fn destroy(pool: Self::Pool);
>> 
>> This should just be done via the normal `Drop` trait?
>
> Let me check if I’m getting you right here. I take what you are suggesting is that we require that Pool implements Drop trait and then just do something like:
>
>     extern "C" fn destroy_(pool: *mut c_void) {
>         // SAFETY: The pointer originates from an `into_foreign` call.
>         unsafe { drop(T::Pool::from_foreign(pool)) }
>     }
>
> Is that understanding correct?

Yes, but you don't need to require the type to implement drop.

---
Cheers,
Benno

Re: [PATCH v4 2/2] rust: zpool: add abstraction for zpool drivers

[Vitaly Wool]

On 8/27/25 17:59, Benno Lossin wrote:
> On Wed Aug 27, 2025 at 4:24 PM CEST, Vitaly Wool wrote:
>>
>>
>>> On Aug 26, 2025, at 7:02 PM, Benno Lossin <lossin@kernel.org> wrote:
>>>
>>> On Sat Aug 23, 2025 at 3:05 PM CEST, Vitaly Wool wrote:
>>>> +pub trait ZpoolDriver {
>>>> +    /// Opaque Rust representation of `struct zpool`.
>>>> +    type Pool: ForeignOwnable;
>>>
>>> I think this is the same question that Danilo asked a few versions ago,
>>> but why do we need this? Why can't we just use `Self` instead?
>>
>> It’s convenient to use it in the backend implementation, like in the toy example supplied in the documentation part:
>>
>> +/// struct MyZpool {
>> +///     name: &'static CStr,
>> +///     bytes_used: AtomicU64,
>> +/// }
>> …
>> +/// impl ZpoolDriver for MyZpoolDriver {
>> +///     type Pool = KBox<MyZpool>;
>>
>> Does that make sense?
> 
> No, why can't it just be like this:
> 
>      struct MyZpool {
>          name: &'static CStr,
>          bytes_used: AtomicU64,
>      }
>      
>      struct MyZpoolDriver;
>      
>      impl ZpoolDriver for MyZpoolDriver {
>          type Error = Infallible;
>      
>          fn create(name: &'static CStr) -> impl PinInit<Self, Self::Error> {
>              MyZpool { name, bytes_used: AtomicU64::new(0) }
>          }
>      
>          fn malloc(&mut self, size: usize, gfp: Flags, _nid: NumaNode) -> Result<usize> {
>              let mut pow: usize = 0;
>              for n in 6..=PAGE_SHIFT {
>                  if size <= 1 << n {
>                      pow = n;
>                      break;
>                  }
>              }
>              match pow {
>                  0 => Err(EINVAL),
>                  _ => {
>                      let vec = KVec::<u64>::with_capacity(1 << (pow - 3), gfp)?;
>                      let (ptr, _len, _cap) = vec.into_raw_parts();
>                      self.bytes_used.fetch_add(1 << pow, Ordering::Relaxed);
>                      Ok(ptr as usize | (pow - 6))
>                  }
>              }
>          }
>      
>          unsafe fn free(&self, handle: usize) {
>              let n = (handle & 0x3F) + 3;
>              let uptr = handle & !0x3F;
>      
>              // SAFETY:
>              // - uptr comes from handle which points to the KVec allocation from `alloc`.
>              // - size == capacity and is coming from the first 6 bits of handle.
>              let vec = unsafe { KVec::<u64>::from_raw_parts(uptr as *mut u64, 1 << n, 1 << n) };
>              drop(vec);
>              self.bytes_used.fetch_sub(1 << (n + 3), Ordering::Relaxed);
>          }
>      
>          unsafe fn read_begin(&self, handle: usize) -> NonNull<u8> {
>              let uptr = handle & !0x3F;
>              // SAFETY: uptr points to a memory area allocated by KVec
>              unsafe { NonNull::new_unchecked(uptr as *mut u8) }
>          }
>      
>          unsafe fn read_end(&self, _handle: usize, _handle_mem: NonNull<u8>) {}
>      
>          unsafe fn write(&self, handle: usize, handle_mem: NonNull<u8>, mem_len: usize) {
>              let uptr = handle & !0x3F;
>              // SAFETY: handle_mem is a valid non-null pointer provided by zpool, uptr points to
>              // a KVec allocated in `malloc` and is therefore also valid.
>              unsafe {
>                  copy_nonoverlapping(handle_mem.as_ptr().cast(), uptr as *mut c_void, mem_len)
>              };
>          }
>      
>          fn total_pages(&self) -> u64 {
>              self.bytes_used.load(Ordering::Relaxed) >> PAGE_SHIFT
>          }
>      }

It can indeed but then the ZpoolDriver trait will have to be extended 
with functions like into_raw() and from_raw(), because zpool expects 
*mut c_void, so on the Adapter side it will look like

     extern "C" fn create_(name: *const c_uchar, gfp: u32) -> *mut c_void {
         // SAFETY: the memory pointed to by name is guaranteed by zpool 
to be a valid string
         let pool = unsafe { T::create(CStr::from_char_ptr(name), 
Flags::from_raw(gfp)) };
         match pool {
             Err(_) => null_mut(),
             Ok(p) => T::into_raw(p).cast(),
         }
     }

The question is, why does this make it better?

> Also using a `usize` as a handle seems like a bad idea. Use a newtype
> wrapper of usize instead. You can also not implement `Copy` and thus get
> rid of one of the safety requirements of the `free` function. Not sure
> if we can remove the other one as well using more type system magic, but
> we could try.

Thanks, I'll surely look into this.

>>>> +
>>>> +    /// Create a pool.
>>>> +    fn create(name: &'static CStr, gfp: Flags) -> Result<Self::Pool>;
>>>> +
>>>> +    /// Destroy the pool.
>>>> +    fn destroy(pool: Self::Pool);
>>>
>>> This should just be done via the normal `Drop` trait?
>>
>> Let me check if I’m getting you right here. I take what you are suggesting is that we require that Pool implements Drop trait and then just do something like:
>>
>>      extern "C" fn destroy_(pool: *mut c_void) {
>>          // SAFETY: The pointer originates from an `into_foreign` call.
>>          unsafe { drop(T::Pool::from_foreign(pool)) }
>>      }
>>
>> Is that understanding correct?
> 
> Yes, but you don't need to require the type to implement drop.
> 
> ---
> Cheers,
> Benno

Re: [PATCH v4 0/2] rust: zpool: add abstraction for zpool drivers

[Miguel Ojeda]

On Sat, Aug 23, 2025 at 3:04 PM Vitaly Wool <vitaly.wool@konsulko.se> wrote:
>
> This patch provides the interface to use Zpool in Rust kernel code,
> thus enabling Rust implementations of Zpool allocators for Zswap.

In v1 the usual use case question was asked -- could we get some more
details in the cover letter or ideally in the patch itself?

>  bindings/bindings_helper.h |    1
>  helpers/helpers.c          |    1
>  helpers/zpool.c            |    6 +
>  kernel/alloc.rs            |    5
>  kernel/lib.rs              |    2
>  kernel/zpool.rs            |  338 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

Should a `MAINTAINERS` change be added? Was maintenance in general discussed?

By the way, the diffstat here in the cover letter seems to be
generated w.r.t. `rust/` for some reason.

Thanks!

Cheers,
Miguel

Re: [PATCH v4 0/2] rust: zpool: add abstraction for zpool drivers

[Danilo Krummrich]

On 8/26/25 12:43 PM, Miguel Ojeda wrote:
> On Sat, Aug 23, 2025 at 3:04 PM Vitaly Wool <vitaly.wool@konsulko.se> wrote:
>>
>> This patch provides the interface to use Zpool in Rust kernel code,
>> thus enabling Rust implementations of Zpool allocators for Zswap.
> 
> In v1 the usual use case question was asked -- could we get some more
> details in the cover letter or ideally in the patch itself?
> 
>>   bindings/bindings_helper.h |    1
>>   helpers/helpers.c          |    1
>>   helpers/zpool.c            |    6 +
>>   kernel/alloc.rs            |    5
>>   kernel/lib.rs              |    2
>>   kernel/zpool.rs            |  338 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 
> Should a `MAINTAINERS` change be added? Was maintenance in general discussed?

@Alice: I assume this should go under the Rust MM entry?

Re: [PATCH v4 0/2] rust: zpool: add abstraction for zpool drivers

[Alice Ryhl]

On Tue, Aug 26, 2025 at 01:37:22PM +0200, Danilo Krummrich wrote:
> On 8/26/25 12:43 PM, Miguel Ojeda wrote:
> > On Sat, Aug 23, 2025 at 3:04 PM Vitaly Wool <vitaly.wool@konsulko.se> wrote:
> > > 
> > > This patch provides the interface to use Zpool in Rust kernel code,
> > > thus enabling Rust implementations of Zpool allocators for Zswap.
> > 
> > In v1 the usual use case question was asked -- could we get some more
> > details in the cover letter or ideally in the patch itself?
> > 
> > >   bindings/bindings_helper.h |    1
> > >   helpers/helpers.c          |    1
> > >   helpers/zpool.c            |    6 +
> > >   kernel/alloc.rs            |    5
> > >   kernel/lib.rs              |    2
> > >   kernel/zpool.rs            |  338 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 
> > Should a `MAINTAINERS` change be added? Was maintenance in general discussed?
> 
> @Alice: I assume this should go under the Rust MM entry?

That makes sense to me.

Alice

Re: [PATCH v4 0/2] rust: zpool: add abstraction for zpool drivers

[Johannes Weiner]

On Sat, Aug 23, 2025 at 03:04:19PM +0200, Vitaly Wool wrote:
> Zpool is a common frontend for memory storage pool implementations.
> These pools are typically used to store compressed memory objects,
> e. g. for Zswap, the lightweight compressed cache for swap pages.
> 
> This patch provides the interface to use Zpool in Rust kernel code,
> thus enabling Rust implementations of Zpool allocators for Zswap.

The zpool indirection is on its way out.

When you submitted an alternate allocator backend recently, the
resounding feedback from the zswap maintainers was that improvements
should happen to zsmalloc incrementally. It is a lot of code and has a
lot of features that go beyond allocation strategy. We do not want to
fork it and fragment this space again with niche, incomplete backends.

It's frustrating that you not only ignored this, but then went ahead
and made other people invest their time and effort into this as well.

Re: [PATCH v4 0/2] rust: zpool: add abstraction for zpool drivers

[Vitaly Wool]

> On Aug 26, 2025, at 2:44 PM, Johannes Weiner <hannes@cmpxchg.org> wrote:
> 
> On Sat, Aug 23, 2025 at 03:04:19PM +0200, Vitaly Wool wrote:
>> Zpool is a common frontend for memory storage pool implementations.
>> These pools are typically used to store compressed memory objects,
>> e. g. for Zswap, the lightweight compressed cache for swap pages.
>> 
>> This patch provides the interface to use Zpool in Rust kernel code,
>> thus enabling Rust implementations of Zpool allocators for Zswap.
> 
> The zpool indirection is on its way out.
> 
> When you submitted an alternate allocator backend recently, the
> resounding feedback from the zswap maintainers was that improvements
> should happen to zsmalloc incrementally. It is a lot of code and has a
> lot of features that go beyond allocation strategy. We do not want to
> fork it and fragment this space again with niche, incomplete backends.
> 
> It's frustrating that you not only ignored this, but then went ahead
> and made other people invest their time and effort into this as well.
> 

I don’t think we have a consensus on that.

And zblock is, after some additional improvements, just better than zsmalloc in all meaningful aspects, let alone the simplicity. It is fas easier to implement in Rust than zsmalloc, too. Besides, zram is a good candidate to be rewritten in Rust as well and after that is done, zblock will be even safer and faster. So while not being “incomplete", it’s zsmalloc that is becoming a niche backend moving forward, and I would argue that it could make more sense to eventually obsolete *it* rather than the zpool API.

~Vitaly

Re: [PATCH v4 0/2] rust: zpool: add abstraction for zpool drivers

[Johannes Weiner]

On Tue, Aug 26, 2025 at 04:56:46PM +0200, Vitaly Wool wrote:
> 
> 
> > On Aug 26, 2025, at 2:44 PM, Johannes Weiner <hannes@cmpxchg.org> wrote:
> > 
> > On Sat, Aug 23, 2025 at 03:04:19PM +0200, Vitaly Wool wrote:
> >> Zpool is a common frontend for memory storage pool implementations.
> >> These pools are typically used to store compressed memory objects,
> >> e. g. for Zswap, the lightweight compressed cache for swap pages.
> >> 
> >> This patch provides the interface to use Zpool in Rust kernel code,
> >> thus enabling Rust implementations of Zpool allocators for Zswap.
> > 
> > The zpool indirection is on its way out.
> > 
> > When you submitted an alternate allocator backend recently, the
> > resounding feedback from the zswap maintainers was that improvements
> > should happen to zsmalloc incrementally. It is a lot of code and has a
> > lot of features that go beyond allocation strategy. We do not want to
> > fork it and fragment this space again with niche, incomplete backends.
> > 
> > It's frustrating that you not only ignored this, but then went ahead
> > and made other people invest their time and effort into this as well.
> > 
> 
> I don’t think we have a consensus on that.
> 
> And zblock is, after some additional improvements, just better than
> zsmalloc in all meaningful aspects, let alone the simplicity. It is
> fas easier to implement in Rust than zsmalloc, too. Besides, zram is
> a good candidate to be rewritten in Rust as well and after that is
> done, zblock will be even safer and faster. So while not being
> “incomplete", it’s zsmalloc that is becoming a niche backend moving
> forward, and I would argue that it could make more sense to
> eventually obsolete *it* rather than the zpool API.

That's your opinion, and I disagree with all of these claims. I would
also be surprised if you found much alignment on this with the other
folks who develop and use these features on a daily basis.

That being said, by all means, you can propose alternate
allocators. But you don't need the zpool API for that. Just provide
alternate implementations of the "zs_*" API and make it compile-time
selectable.

As it stands, it's hard to justify the almost 700 lines of code to
support *runtime-switching* of zswap backends when there is only one
backend in-tree (and even you suggest there should only be one, albeit
a different one).

Re: [PATCH v4 0/2] rust: zpool: add abstraction for zpool drivers

[Vitaly Wool]

> On Aug 27, 2025, at 3:07 PM, Johannes Weiner <hannes@cmpxchg.org> wrote:
> 
> On Tue, Aug 26, 2025 at 04:56:46PM +0200, Vitaly Wool wrote:
>> 
>> 
>>> On Aug 26, 2025, at 2:44 PM, Johannes Weiner <hannes@cmpxchg.org> wrote:
>>> 
>>> On Sat, Aug 23, 2025 at 03:04:19PM +0200, Vitaly Wool wrote:
>>>> Zpool is a common frontend for memory storage pool implementations.
>>>> These pools are typically used to store compressed memory objects,
>>>> e. g. for Zswap, the lightweight compressed cache for swap pages.
>>>> 
>>>> This patch provides the interface to use Zpool in Rust kernel code,
>>>> thus enabling Rust implementations of Zpool allocators for Zswap.
>>> 
>>> The zpool indirection is on its way out.
>>> 
>>> When you submitted an alternate allocator backend recently, the
>>> resounding feedback from the zswap maintainers was that improvements
>>> should happen to zsmalloc incrementally. It is a lot of code and has a
>>> lot of features that go beyond allocation strategy. We do not want to
>>> fork it and fragment this space again with niche, incomplete backends.
>>> 
>>> It's frustrating that you not only ignored this, but then went ahead
>>> and made other people invest their time and effort into this as well.
>>> 
>> 
>> I don’t think we have a consensus on that.
>> 
>> And zblock is, after some additional improvements, just better than
>> zsmalloc in all meaningful aspects, let alone the simplicity. It is
>> fas easier to implement in Rust than zsmalloc, too. Besides, zram is
>> a good candidate to be rewritten in Rust as well and after that is
>> done, zblock will be even safer and faster. So while not being
>> “incomplete", it’s zsmalloc that is becoming a niche backend moving
>> forward, and I would argue that it could make more sense to
>> eventually obsolete *it* rather than the zpool API.
> 
> That's your opinion, and I disagree with all of these claims. I would
> also be surprised if you found much alignment on this with the other
> folks who develop and use these features on a daily basis.

By features you mean zsmalloc? I certainly respect the effort put in it but if not for it being rather sluggish and error prone in non-4K page environments, we’d not come up with zblock.

> That being said, by all means, you can propose alternate
> allocators. But you don't need the zpool API for that. Just provide
> alternate implementations of the "zs_*" API and make it compile-time
> selectable.
> 
zpool API is neutral and well-defined, I don’t see *any* good reason for it to be phased out.

> As it stands, it's hard to justify the almost 700 lines of code to
> support *runtime-switching* of zswap backends when there is only one
> backend in-tree (and even you suggest there should only be one, albeit
> a different one).

No, I don’t. What I said was that zsmalloc was arguably becoming a niche allocator. And the point here is, the toy allocator written in Rust (just as an example of how zpool API could be used) shows in some tests similar results to zsmalloc both performance and compression density wise.