Message-Id: <DC76DSYUY978.3NR8S2K13I9RX@kernel.org>
Date: Wed, 20 Aug 2025 12:16:19 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Vitaly Wool" <vitaly.wool@...sulko.se>
Cc: <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 "Uladzislau Rezki" <urezki@...il.com>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Vlastimil Babka" <vbabka@...e.cz>, "Lorenzo Stoakes"
 <lorenzo.stoakes@...cle.com>, "Liam R . Howlett" <Liam.Howlett@...cle.com>,
 "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...il.com>,
 "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>, "Bjorn
 Roy Baron" <bjorn3_gh@...tonmail.com>, "Benno Lossin" <lossin@...nel.org>,
 "Andreas Hindborg" <a.hindborg@...nel.org>, "Trevor Gross"
 <tmgross@...ch.edu>, "Johannes Weiner" <hannes@...xchg.org>, "Yosry Ahmed"
 <yosry.ahmed@...ux.dev>, "Nhat Pham" <nphamcs@...il.com>,
 <linux-mm@...ck.org>
Subject: Re: [PATCH] rust: zpool: add abstraction for zpool drivers

On Wed Aug 20, 2025 at 11:15 AM CEST, 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.

Do you work on such a user? Do you have code using this API already?

More specifically, do you plan to re-implement Zswap in Rust?

> Signed-off-by: Vitaly Wool <vitaly.wool@...sulko.se>
> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>

If Alice contributed to the patch you may want to add a Co-developed-by: tag,
otherwise this doesn't need her SoB.

> +/// zpool API

It would be nice to have some more documentation on this trait, including a
doc-test illustrating some example usage.

> +pub trait Zpool {
> +    /// Opaque Rust representation of `struct zpool`.
> +    type Pool: ForeignOwnable;

Something that embedds a struct zpool, such as struct zswap_pool? If so, isn't
this type simply Self?

> +
> +    /// Create a pool.
> +    fn create(name: *const c_uchar, gfp: Flags) -> Result<Self::Pool>;

This shouldn't be a raw pointer, but rather &CStr.

> +
> +    /// Destroy the pool.
> +    fn destroy(pool: Self::Pool);
> +
> +    /// Allocate an object of size `size` using GFP flags `gfp` from the pool `pool`, wuth the
> +    /// 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`.
> +    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.
> +    fn read_begin(pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>, handle: usize)
> +        -> *mut c_void;

Why does this return a raw pointer? I think this needs a proper type
representation.

> +
> +    /// Finish reading from a previously allocated `handle`. `handle_mem` must be the pointer
> +    /// previously returned by `read_begin`.
> +    fn read_end(
> +        pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>,
> +        handle: usize,
> +        handle_mem: *mut c_void,
> +    );

Same here...

> +
> +    /// 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.
> +    fn write(
> +        pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>,
> +        handle: usize,
> +        handle_mem: *mut c_void,

...and here.

> +        mem_len: usize,
> +    );
> +
> +    /// Get the number of pages used by the `pool`.
> +    fn total_pages(pool: <Self::Pool as ForeignOwnable>::Borrowed<'_>) -> u64;
> +}
> +
> +/// Zpool driver registration trait.
> +pub trait Registration {

I think you should use the kernel::driver::Registration instead, it's
specifically for the purpose you defined this trait and ZpoolDriver for.

As for the C callbacks, they should go into the Adapter type (which implements
kernel::driver::RegistrationOps) directly, they don't need to be in a trait.

This way a new Zpool Registration is created with:

	driver::Registration<zpool::Adapter>::new()

This also allows you to take advantage of the module_driver!() macro to provide
your own module_zpool_driver!() macro.

> +    /// Register a zpool driver.
> +    fn register(&self, name: &'static CStr, module: &'static ThisModule) -> Result;
> +
> +    /// Pool creation callback.
> +    extern "C" fn _create(name: *const c_uchar, gfp: u32) -> *mut c_void;
> +
> +    /// Pool destruction callback.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that `pool` is a valid pointer to `struct zpool`.
> +    unsafe extern "C" fn _destroy(pool: *mut c_void);
> +
> +    /// Callback for object allocation in the pool.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that `pool` is a valid pointer to `struct zpool` and that `handle`
> +    /// is a valid pointer to usize.
> +    unsafe extern "C" fn _malloc(
> +        pool: *mut c_void,
> +        size: usize,
> +        gfp: u32,
> +        handle: *mut usize,
> +        nid: c_int,
> +    ) -> c_int;
> +
> +    /// Callback for object release.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that `pool` is a valid pointer to `struct zpool`.
> +    unsafe extern "C" fn _free(pool: *mut c_void, handle: usize);
> +
> +    /// Callback to prepare the object for reading.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that `pool` is a valid pointer to `struct zpool`.
> +    unsafe extern "C" fn _obj_read_begin(
> +        pool: *mut c_void,
> +        handle: usize,
> +        local_copy: *mut c_void,
> +    ) -> *mut c_void;
> +
> +    /// Callback to signal the end of reading from an object.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that `pool` is a valid pointer to `struct zpool`.
> +    unsafe extern "C" fn _obj_read_end(pool: *mut c_void, handle: usize, handle_mem: *mut c_void);
> +
> +    /// Callback for writing to an object.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that `pool` is a valid pointer to `struct zpool`.
> +    unsafe extern "C" fn _obj_write(
> +        pool: *mut c_void,
> +        handle: usize,
> +        handle_mem: *mut c_void,
> +        mem_len: usize,
> +    );
> +
> +    /// Callback to return the number of pages in the pool.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that `pool` is a valid pointer to `struct zpool`.
> +    unsafe extern "C" fn _total_pages(pool: *mut c_void) -> u64;
> +}
> +
> +/// Zpool driver structure.
> +pub struct ZpoolDriver<T: Zpool> {
> +    inner: Opaque<bindings::zpool_driver>,

I think this needs pin-init, another reason to use kernel::driver::Registration
instead. :)

> +
> +    /// Zpool callback functions that a zpool driver must provide
> +    pub callbacks: T,
> +}
> +
> +impl<T: Zpool> Clone for ZpoolDriver<T> {
> +    fn clone(&self) -> Self {
> +        todo!()
> +    }
> +}

Cloning the driver structure? Why? Please also consider that struct zpool_driver
needs to be pinned.

Powered by blists - more mailing lists