Message-Id: <DASY7BECFRCT.332X5ZHZMV2W@kernel.org>
Date: Sun, 22 Jun 2025 11:18:24 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: Onur Özkan <work@...rozkan.dev>,
 <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Cc: <ojeda@...nel.org>, <alex.gaynor@...il.com>, <boqun.feng@...il.com>,
 <gary@...yguo.net>, <a.hindborg@...nel.org>, <aliceryhl@...gle.com>,
 <tmgross@...ch.edu>, <dakr@...nel.org>, <peterz@...radead.org>,
 <mingo@...hat.com>, <will@...nel.org>, <longman@...hat.com>,
 <felipe_life@...e.com>, <daniel@...lak.dev>, <bjorn3_gh@...tonmail.com>
Subject: Re: [PATCH v5 2/3] implement ww_mutex abstraction for the Rust tree

On Sat Jun 21, 2025 at 8:44 PM CEST, Onur Özkan wrote:
> Adds Rust bindings for the kernel's `ww_mutex` infrastructure to enable
> deadlock-free acquisition of multiple related locks.
>
> The patch abstracts `ww_mutex.h` header and wraps the existing
> C `ww_mutex` with three main types:
>     - `WwClass` for grouping related mutexes
>     - `WwAcquireCtx` for tracking lock acquisition context
>     - `WwMutex<T>` for the actual lock

Going to repeat my question from the previous version:

    I don't know the design of `struct ww_mutex`, but from the code below I
    gathered that it has some special error return values that signify that
    one should release other locks.
    
    Did anyone think about making a more Rusty API that would allow one to
    try to lock multiple mutexes at the same time (in a specified order) and
    if it fails, it would do the resetting automatically?

I'm not familiar with ww_mutex, so I can't tell if there is something
good that we could do.

> Signed-off-by: Onur Özkan <work@...rozkan.dev>
> ---
>  rust/kernel/error.rs              |   1 +
>  rust/kernel/sync/lock.rs          |   1 +
>  rust/kernel/sync/lock/ww_mutex.rs | 421 ++++++++++++++++++++++++++++++
>  3 files changed, 423 insertions(+)
>  create mode 100644 rust/kernel/sync/lock/ww_mutex.rs

Thanks for splitting the tests into its own patch, but I still think
it's useful to do the abstractions for `ww_class`, `ww_acquire_ctx` and
`ww_mutex` separately.

> +/// Create static [`WwClass`] instances.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// use kernel::{c_str, define_ww_class};
> +///
> +/// define_ww_class!(WOUND_WAIT_GLOBAL_CLASS, wound_wait, c_str!("wound_wait_global_class"));
> +/// define_ww_class!(WAIT_DIE_GLOBAL_CLASS, wait_die, c_str!("wait_die_global_class"));
> +/// ```
> +#[macro_export]
> +macro_rules! define_ww_class {
> +    ($name:ident, wound_wait, $class_name:expr) => {
> +        static $name: $crate::sync::lock::ww_mutex::WwClass =
> +            { $crate::sync::lock::ww_mutex::WwClass::new($class_name, false) };
> +    };
> +    ($name:ident, wait_die, $class_name:expr) => {
> +        static $name: $crate::sync::lock::ww_mutex::WwClass =
> +            { $crate::sync::lock::ww_mutex::WwClass::new($class_name, true) };
> +    };
> +}

I really don't see the value in having a macro here. The user can just
declare the static themselves.

> +
> +/// A group of mutexes that can participate in deadlock avoidance together.

This isn't a group of mutexes, but rather a class that can be used to
group mutexes together.

> +///
> +/// All mutexes that might be acquired together should use the same class.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// use kernel::sync::lock::ww_mutex::WwClass;
> +/// use kernel::c_str;
> +/// use pin_init::stack_pin_init;
> +///
> +/// stack_pin_init!(let _wait_die_class = WwClass::new_wait_die(c_str!("graphics_buffers")));
> +/// stack_pin_init!(let _wound_wait_class = WwClass::new_wound_wait(c_str!("memory_pools")));
> +///
> +/// # Ok::<(), Error>(())
> +/// ```
> +#[pin_data]
> +pub struct WwClass {
> +    #[pin]
> +    inner: Opaque<bindings::ww_class>,
> +}
> +
> +// SAFETY: [`WwClass`] is set up once and never modified. It's fine to share it across threads.
> +unsafe impl Sync for WwClass {}
> +// SAFETY: Doesn't hold anything thread-specific. It's safe to send to other threads.
> +unsafe impl Send for WwClass {}
> +
> +macro_rules! ww_class_init_helper {
> +    ($name:expr, $is_wait_die:expr) => {
> +        Opaque::new(bindings::ww_class {
> +            stamp: bindings::atomic_long_t { counter: 0 },
> +            acquire_name: $name.as_char_ptr(),
> +            mutex_name: $name.as_char_ptr(),
> +            is_wait_die: $is_wait_die as u32,
> +            // TODO: Replace with `bindings::lock_class_key::default()` once stabilized for `const`.
> +            //
> +            // SAFETY: This is always zero-initialized when defined with `DEFINE_WD_CLASS`
> +            // globally on C side.
> +            //
> +            // Ref: <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/ww_mutex.h?h=v6.16-rc2#n85>
> +            acquire_key: unsafe { core::mem::zeroed() },
> +            // TODO: Replace with `bindings::lock_class_key::default()` once stabilized for `const`.
> +            //
> +            // SAFETY: This is always zero-initialized when defined with `DEFINE_WD_CLASS`
> +            // globally on C side.
> +            //
> +            // Ref: <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/ww_mutex.h?h=v6.16-rc2#n85>
> +            mutex_key: unsafe { core::mem::zeroed() },
> +        })
> +    };
> +}

Is this really needed? Can't we do without this macro?

> +
> +impl WwClass {
> +    /// Creates a [`WwClass`].
> +    ///
> +    /// It's `pub` only so it can be used by the `define_ww_class!` macro.
> +    ///
> +    /// You should not use this function directly. Use the `define_ww_class!`
> +    /// macro or call [`WwClass::new_wait_die`] or [`WwClass::new_wound_wait`] instead.
> +    pub const fn new(name: &'static CStr, is_wait_die: bool) -> Self {

This doesn't work together with `#[pin_data]`, you shouldn't return it
by-value if it is supposed to be pin-initialized.

Is this type address sensitive? If yes, this function needs to be
`unsafe`, if no, then we can remove `#[pin_data]`.

> +        WwClass {
> +            inner: ww_class_init_helper!(name, is_wait_die),
> +        }
> +    }
> +
> +    /// Creates wait-die [`WwClass`].
> +    pub fn new_wait_die(name: &'static CStr) -> impl PinInit<Self> {
> +        pin_init!(WwClass {
> +            inner: ww_class_init_helper!(name, true),
> +        })

This can just be `new(name, true)` instead.

> +    }
> +
> +    /// Creates wound-wait [`WwClass`].
> +    pub fn new_wound_wait(name: &'static CStr) -> impl PinInit<Self> {
> +        pin_init!(WwClass {
> +            inner: ww_class_init_helper!(name, false),
> +        })

Ditto with `false`.

> +    }
> +}
> +
> +/// An acquire context is used to group multiple mutex acquisitions together
> +/// for deadlock avoidance. It must be used when acquiring multiple mutexes
> +/// of the same class.

The first paragraph will be displayed by rustdoc in several summary
views, so it should be a single short sentence.

> +///
> +/// # Examples
> +///
> +/// ```
> +/// use kernel::sync::lock::ww_mutex::{WwClass, WwAcquireCtx, WwMutex};
> +/// use kernel::c_str;
> +/// use kernel::alloc::KBox;
> +/// use pin_init::stack_pin_init;
> +///
> +/// stack_pin_init!(let class = WwClass::new_wound_wait(c_str!("my_class")));
> +///
> +/// // Create mutexes.
> +/// stack_pin_init!(let mutex1 = WwMutex::new(1, &class));
> +/// stack_pin_init!(let mutex2 = WwMutex::new(2, &class));

I don't think it makes sense to have examples using mutexes that are on
the stack. Please use `Arc` instead.

> +///
> +/// // Create acquire context for deadlock avoidance.
> +/// let mut ctx = KBox::pin_init(WwAcquireCtx::new(&class), GFP_KERNEL)?;
> +///
> +/// // Acquire multiple locks safely.
> +/// let guard1 = mutex1.lock(Some(&ctx))?;
> +/// let guard2 = mutex2.lock(Some(&ctx))?;
> +///
> +/// // Mark acquisition phase as complete.
> +/// ctx.as_mut().done();
> +///
> +/// # Ok::<(), Error>(())
> +/// ```
> +#[pin_data(PinnedDrop)]
> +pub struct WwAcquireCtx<'a> {
> +    #[pin]
> +    inner: Opaque<bindings::ww_acquire_ctx>,
> +    _p: PhantomData<&'a WwClass>,
> +}

> +// SAFETY: [`WwMutex`] can be shared between threads.
> +unsafe impl<T: ?Sized + Send> Send for WwMutex<'_, T> {}
> +// SAFETY: [`WwMutex`] can be safely accessed from multiple threads concurrently.
> +unsafe impl<T: ?Sized + Sync> Sync for WwMutex<'_, T> {}
> +
> +impl<'ww_class, T> WwMutex<'ww_class, T> {
> +    /// Creates `Self` with calling `ww_mutex_init` inside.
> +    pub fn new(t: T, ww_class: &'ww_class WwClass) -> impl PinInit<Self> {
> +        let raw_ptr = ww_class.inner.get();

s/raw_ptr/class/

> +        pin_init!(WwMutex {
> +            mutex <- Opaque::ffi_init(|slot: *mut bindings::ww_mutex| {
> +                // SAFETY: The caller guarantees that `ww_class` remains valid.

The caller doesn't need to guarantees that (and in fact they don't),
because it's a reference that lives until `'ww_class` which is captured
by `Self`. So the borrow checker guarantees that the class remains
valid.

---
Cheers,
Benno

> +                unsafe { bindings::ww_mutex_init(slot, raw_ptr) }
> +            }),
> +            data: UnsafeCell::new(t),
> +            _p: PhantomData,
> +        })
> +    }
> +}

Powered by blists - more mailing lists