Message-Id: <DC90ONOZLKAZ.35P6HOCCAIPW1@kernel.org>
Date: Fri, 22 Aug 2025 16:13:43 +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 v3] rust: zpool: add abstraction for zpool drivers

On Fri Aug 22, 2025 at 3:39 PM CEST, Vitaly Wool wrote:
> diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> index b39c279236f5..0fec5337908c 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 fn new(f: u32) -> Self {
> +        Self(f)
> +    }

I didn't notice this change before.

This function should be crate private, called from_raw() and document that the
given value must be a valid combination of GFP flags.

Please also make the addition of alloc::Flags::from_raw() a separate patch.

> +/// # Example
> +///
> +/// A zpool driver implementation which does nothing but prints pool name on its creation and
> +/// destruction, and panics if zswap tries to actually read from a pool's alleged object.
> +///
> +/// ```
> +/// use core::ptr::NonNull;
> +/// use kernel::alloc::{Flags, KBox, NumaNode};
> +/// use kernel::zpool::*;
> +///
> +/// struct MyZpool {
> +///     name: &'static CStr,
> +/// }
> +///
> +/// struct MyZpoolDriver;
> +///
> +/// impl ZpoolDriver for MyZpoolDriver {
> +///     type Pool = KBox<MyZpool>;
> +///
> +///     fn create(name: &'static CStr, gfp: Flags) -> Result<KBox<MyZpool>> {
> +///         let myPool = MyZpool { name };
> +///         let mut pool = KBox::new(myPool, gfp)?;

Why mutable?

> +///
> +///         pr_info!("Created pool {}\n", pool.name);

I think this print is unnecessary.

> +///         Ok(pool)
> +///     }

Please add empty lines between functions.

> +///     fn destroy(p: KBox<MyZpool>) {
> +///         let pool = KBox::into_inner(p);

Why the call to into_inner()? What do we get from moving the value?

> +///         pr_info!("Removed pool {}\n", pool.name);

Same as above, I think the print is unnecessary.

> +///     }
> +///     fn malloc(_pool: &mut MyZpool, _size: usize, _gfp: Flags, _nid: NumaNode) -> Result<usize> {
> +///         Ok(0) // TODO
> +///     }
> +///     unsafe fn free(_pool: &MyZpool, _handle: usize) {
> +///         // TODO

I'm not sure the TODO comments add any value.

> +///     }
> +///     unsafe fn read_begin(_pool: &MyZpool, _handle: usize) -> NonNull<u8> {
> +///         panic!("read_begin not implemented\n"); // TODO

Please don't use panic!() here, we only ever panic the kernel when there is no
other way to prevent undefined bahvior, i.e. as a last resort.

I know it's just an example and it's not even run, but it might trick people
into thinking that is is OK, given that it is an example.

> +///     }
> +///     unsafe fn read_end(_pool: &MyZpool, _handle: usize, _handle_mem: NonNull<u8>) {}
> +///     unsafe fn write(_p: &MyZpool, _h: usize, _handle_mem: NonNull<u8>, _mem_len: usize) {}
> +///     fn total_pages(_pool: &MyZpool) -> u64 { 0 }
> +/// }

Overall, I feel like the example doesn't add much value as it is. I think it
would be better to provide a real example that actually implements the
callbacks.

What about doing adding the most simple implementation using a plain VVec for
backing the pool? This way we can even run the example. :)

> +/// ```
> +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` using GFP flags `gfp` from the pool `pool`, with the

I think you can just write "Allocate an object of `size` bytes from `pool`",
which should sound a bit more organic.

> +    /// 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` should be a valid handle previously returned by `malloc`

"must be"; please also add a period at the end of a sentences (applies to the
whole patch).

> +    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` should be a valid handle previously returned by `malloc`

A handle that has been given to free() already cannot be used here anymore.
However, as by your safety documentation this would be valid.

More generally, for the safety documentation I recommend having a look at the
documentation of the Allocator trait [1].

[1] https://rust.docs.kernel.org/src/kernel/alloc.rs.html#139-216

Powered by blists - more mailing lists