Message-Id: <C642BA32-4EA5-4843-9625-5DBF40A42C6C@konsulko.se>
Date: Wed, 20 Aug 2025 12:44:40 +0200
From: Vitaly Wool <vitaly.wool@...sulko.se>
To: Danilo Krummrich <dakr@...nel.org>
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 Aug 20, 2025, at 12:16 PM, Danilo Krummrich <dakr@...nel.org> wrote:
> 
> 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?

I don’t plan to reimplement zswap as a whole, just the allocation backend for it. I do have the code but it’s not quite ready for submission yet. However, it does work and is publicly available at https://github.com/vwool/linux-mm/commit/6fde78b24c8f8716c17ffa20cec390b516c7882b
> 
>> 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.

Will do, thanks.

> 
>> +/// 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?

I think ForeignOwnable provides a good representation of 'struct zpool’ and it’s convenient to borrow it, as done later in the patch.

> 
>> +
>> +    /// 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.

The zpool API wants a raw pointer here so I decided not to overcomplicate it. I thought of using NonNull<[u8]> but it doesn’t seem to be a good fit. We’re basically returning a pointer to a place in memory which is guaranteed to be allocated and owned by us, but it is a raw pointer at the end of the day. What would you recommend here instead?

> 
>> +
>> +    /// 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.

There was once a problem with that but I don’t remember what the problem was :) I’ll try that one more time, thanks.

> 
>> +    /// 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.

Totally unnecessary, I agree. Will remove.

Thanks for the quick review!

~Vitaly

Powered by blists - more mailing lists