Message-Id: <DAQL7NBDD68Q.1ZS3PUR0AGN0R@kernel.org>
Date: Thu, 19 Jun 2025 16:42:15 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: Onur Özkan <work@...rozkan.dev>,
 <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Cc: <peterz@...radead.org>, <mingo@...hat.com>, <will@...nel.org>,
 <boqun.feng@...il.com>, <longman@...hat.com>, <ojeda@...nel.org>,
 <alex.gaynor@...il.com>, <gary@...yguo.net>, <bjorn3_gh@...tonmail.com>,
 <a.hindborg@...nel.org>, <aliceryhl@...gle.com>, <tmgross@...ch.edu>,
 <dakr@...nel.org>, <thatslyude@...il.com>
Subject: Re: [PATCH V3] implement `ww_mutex` abstraction for the Rust tree

On Thu Jun 19, 2025 at 4:06 PM CEST, Onur Özkan wrote:
> From: onur-ozkan <work@...rozkan.dev>

Can you double-check your name in your git config? This doesn't match
the Signed-off-by below.

> Adds Rust bindings for the kernel's `ww_mutex` infrastructure to enable
> deadlock-free acquisition of multiple related locks.
>
> The implementation 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
>
> Some of the kernel's `ww_mutex` functions are implemented as `static inline`,
> so they are inaccessible from Rust as bindgen can't generate code on them.
> The `rust/helpers/ww_mutex.c` file provides C function wrappers around these inline
> implementations, so bindgen can see them and generate the corresponding Rust code.
>
> Link: https://rust-for-linux.zulipchat.com/#narrow/channel/291566-Library/topic/Writing.20up.20wrappers.20for.20ww_mutex.3F/with/524269974
> Suggested-by: thatslyude@...il.com
> Signed-off-by: Onur Özkan <work@...rozkan.dev>
> ---
>  rust/helpers/helpers.c            |   1 +
>  rust/helpers/ww_mutex.c           |  39 +++
>  rust/kernel/error.rs              |   1 +
>  rust/kernel/sync/lock.rs          |   1 +
>  rust/kernel/sync/lock/ww_mutex.rs | 556 ++++++++++++++++++++++++++++++
>  5 files changed, 598 insertions(+)
>  create mode 100644 rust/helpers/ww_mutex.c
>  create mode 100644 rust/kernel/sync/lock/ww_mutex.rs

Can you split this patch into multiple smaller ones? For example all the
tests can be done separately as well as the abstractions for `ww_class`,
`ww_acquire_ctx` and `ww_mutex`.

Thanks.

> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index 3dee3139fcd4..94d8014b236b 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -84,6 +84,7 @@ macro_rules! declare_err {
>      declare_err!(EIOCBQUEUED, "iocb queued, will get completion event.");
>      declare_err!(ERECALLCONFLICT, "Conflict with recalled state.");
>      declare_err!(ENOGRACE, "NFS file lock reclaim refused.");
> +    declare_err!(EDEADLK, "Resource deadlock avoided.");
>  }
>
>  /// Generic integer kernel error.
> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> index e82fa5be289c..8824ebc81084 100644
> --- a/rust/kernel/sync/lock.rs
> +++ b/rust/kernel/sync/lock.rs
> @@ -15,6 +15,7 @@
>
>  pub mod mutex;
>  pub mod spinlock;
> +pub mod ww_mutex;
>
>  pub(super) mod global;
>  pub use global::{GlobalGuard, GlobalLock, GlobalLockBackend, GlobalLockedBy};
> diff --git a/rust/kernel/sync/lock/ww_mutex.rs b/rust/kernel/sync/lock/ww_mutex.rs
> new file mode 100644
> index 000000000000..888db286fc4b
> --- /dev/null
> +++ b/rust/kernel/sync/lock/ww_mutex.rs
> @@ -0,0 +1,556 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! A kernel Wound/Wait Mutex.
> +//!
> +//! This module provides Rust abstractions for the Linux kernel's `ww_mutex` implementation,
> +//! which provides deadlock avoidance through a wait-wound or wait-die algorithm.
> +
> +use crate::error::{to_result, Result};
> +use crate::prelude::EBUSY;
> +use crate::{bindings, str::CStr, types::Opaque};
> +use core::marker::PhantomData;
> +use core::{cell::UnsafeCell, pin::Pin};
> +use macros::kunit_tests;
> +use pin_init::{pin_data, pin_init, pinned_drop, PinInit};
> +
> +/// A helper macro for creating static `WwClass` instances.

s/A helper macro for creating/Create/

> +///
> +/// # Examples
> +///
> +/// ```
> +/// use kernel::c_str;
> +/// use kernel::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 {

What's the reason for this being a macro?

> +    ($name:ident, wound_wait, $class_name:expr) => {
> +        static $name: $crate::sync::lock::ww_mutex::WwClass = {
> +            $crate::sync::lock::ww_mutex::WwClass {
> +                inner: $crate::types::Opaque::new($crate::bindings::ww_class {
> +                    stamp: $crate::bindings::atomic_long_t { counter: 0 },
> +                    acquire_name: $class_name.as_char_ptr(),
> +                    mutex_name: $class_name.as_char_ptr(),
> +                    is_wait_die: 0,
> +                    // 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://github.com/torvalds/linux/blob/master/include/linux/ww_mutex.h#L85-L89
> +                    acquire_key: unsafe { core::mem::zeroed() },

This (and the others) can use [1] instead of `unsafe`. That series will
most likely land in v6.17.

[1]: https://lore.kernel.org/all/20250523145125.523275-1-lossin@kernel.org

> +                    // 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://github.com/torvalds/linux/blob/master/include/linux/ww_mutex.h#L85-L89
> +                    mutex_key: unsafe { core::mem::zeroed() },
> +                }),
> +            }
> +        };
> +    };
> +    ($name:ident, wait_die, $class_name:expr) => {
> +        static $name: $crate::sync::lock::ww_mutex::WwClass = {
> +            $crate::sync::lock::ww_mutex::WwClass {
> +                inner: $crate::types::Opaque::new($crate::bindings::ww_class {
> +                    stamp: $crate::bindings::atomic_long_t { counter: 0 },
> +                    acquire_name: $class_name.as_char_ptr(),
> +                    mutex_name: $class_name.as_char_ptr(),
> +                    is_wait_die: 1,
> +                    // 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://github.com/torvalds/linux/blob/master/include/linux/ww_mutex.h#L85-L89
> +                    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://github.com/torvalds/linux/blob/master/include/linux/ww_mutex.h#L85-L89
> +                    mutex_key: unsafe { core::mem::zeroed() },
> +                }),
> +            }
> +        };
> +    };
> +}
> +
> +/// Implementation of C side `ww_class`.

This isn't informative at all. The names already match, so I wouldn't
have thought otherwise.

> +///
> +/// Represents a group of mutexes that can participate in deadlock avoidance 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 {
> +    /// Wrapper of the underlying C `ww_class`.
> +    ///
> +    /// You should not construct this type manually. Use the `define_ww_class` macro
> +    /// or call `WwClass::new_wait_die` or `WwClass::new_wound_wait` instead.
> +    #[pin]
> +    pub inner: Opaque<bindings::ww_class>,

Why `pub`? Abstractions normally don't expose `Opaque` wrappers for
bindings. Especially because this type is marked `#[pin_data]` this
seems wrong, because this would allow people to construct it in a
non-pinned state & also non-initialized state.

> +}
> +
> +// SAFETY: `WwClass` can be safely accessed from multiple threads concurrently.

Why? This is supposed to justify that.

> +unsafe impl Sync for WwClass {}
> +// SAFETY: `WwClass` can be shared between threads.
> +unsafe impl Send for WwClass {}
> +
> +impl WwClass {
> +    fn new(name: &'static CStr, is_wait_die: bool) -> impl PinInit<Self> {
> +        pin_init!(WwClass {
> +            inner: 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,
> +                acquire_key: bindings::lock_class_key::default(),
> +                mutex_key: bindings::lock_class_key::default(),
> +            })
> +        })
> +    }
> +
> +    /// Creates wait-die `WwClass` that wraps C side `ww_class`.
> +    pub fn new_wait_die(name: &'static CStr) -> impl PinInit<Self> {
> +        Self::new(name, true)
> +    }
> +
> +    /// Creates wound-wait `WwClass` that wraps C side `ww_class`.
> +    pub fn new_wound_wait(name: &'static CStr) -> impl PinInit<Self> {
> +        Self::new(name, false)
> +    }
> +}
> +
> +/// Implementation of C side `ww_acquire_ctx`.

This also isn't informative.

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

[...]

> +/// A wound/wait mutex backed with C side `ww_mutex`.
> +///
> +/// This is a mutual exclusion primitive that provides deadlock avoidance when
> +/// acquiring multiple locks of the same class.
> +///
> +/// # Examples
> +///
> +/// ## Basic Usage
> +///
> +/// ```
> +/// use kernel::sync::lock::ww_mutex::{WwClass, WwMutex};
> +/// use kernel::c_str;
> +/// use pin_init::stack_pin_init;
> +///
> +/// stack_pin_init!(let class = WwClass::new_wound_wait(c_str!("buffer_class")));
> +/// stack_pin_init!(let mutex = WwMutex::new(42, &class));
> +///
> +/// // Simple lock without context
> +/// let guard = mutex.as_ref().lock(None).unwrap();
> +/// assert_eq!(*guard, 42);
> +///
> +/// # Ok::<(), Error>(())
> +/// ```
> +///
> +/// ## Multiple Locks with KBox
> +///
> +/// ```
> +/// use kernel::sync::lock::ww_mutex::{WwClass, WwAcquireCtx, WwMutex};
> +/// use kernel::alloc::KBox;
> +/// use kernel::c_str;
> +/// use kernel::error::code::*;
> +///
> +/// let class = KBox::pin_init(WwClass::new_wait_die(c_str!("resource_class")), GFP_KERNEL).unwrap();
> +/// let mutex_a = KBox::pin_init(WwMutex::new("Resource A", &class), GFP_KERNEL).unwrap();
> +/// let mutex_b = KBox::pin_init(WwMutex::new("Resource B", &class), GFP_KERNEL).unwrap();

Storing mutexes in `KBox` doesn't really make sense there might be
special cases, but for this example, we should use `Arc` instead.

> +///
> +/// let mut ctx = KBox::pin_init(WwAcquireCtx::new(&class), GFP_KERNEL).unwrap();
> +///
> +/// // Try to acquire both locks
> +/// let guard_a = match mutex_a.as_ref().lock(Some(&ctx)) {
> +///     Ok(guard) => guard,
> +///     Err(e) if e == EDEADLK => {
> +///         // Deadlock detected, use slow path
> +///         mutex_a.as_ref().lock_slow(&ctx).unwrap()
> +///     }
> +///     Err(e) => return Err(e),
> +/// };
> +///
> +/// let guard_b = mutex_b.as_ref().lock(Some(&ctx)).unwrap();
> +/// ctx.as_mut().done();
> +///
> +/// # Ok::<(), Error>(())
> +/// ```
> +#[pin_data]
> +pub struct WwMutex<'a, T: ?Sized> {
> +    _p: PhantomData<&'a WwClass>,
> +    #[pin]
> +    mutex: Opaque<bindings::ww_mutex>,
> +    #[pin]
> +    data: UnsafeCell<T>,

You marked the data as `#[pin]`, but `lock()` gives access to
`WwMutexGuard` which implements `DerefMut`, circumventing the pinning.
So either remove the `#[pin]` on `data`, or only return
`Pin<WwMutexGuard>`.

> +}
> +
> +// 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();
> +        pin_init!(WwMutex {
> +            mutex <- Opaque::ffi_init(|slot: *mut bindings::ww_mutex| {
> +                // SAFETY: The caller guarantees that `ww_class` remains valid.
> +                unsafe { bindings::ww_mutex_init(slot, raw_ptr) }
> +            }),
> +            data: UnsafeCell::new(t),
> +            _p: PhantomData,
> +        })
> +    }
> +}
> +
> +impl<T: ?Sized> WwMutex<'_, T> {
> +    /// Locks the mutex with the given acquire context.
> +    pub fn lock<'a>(
> +        self: Pin<&'a Self>,

This receiver type is pretty annoying, because `Arc<T>` and
`Pin<Box<T>>` deref to `&T` and thus you don't get `Pin<&T>`. Just use
`&self` instead, if your type is only constructible using `impl
PinInit<Self>`, then it's guaranteed that all instances of the type are
pinned.

> +        ctx: Option<&WwAcquireCtx<'_>>,
> +    ) -> Result<WwMutexGuard<'a, T>> {
> +        // SAFETY: The mutex is pinned and valid.
> +        let ret = unsafe {
> +            bindings::ww_mutex_lock(
> +                self.mutex.get(),
> +                ctx.map_or(core::ptr::null_mut(), |c| c.as_ptr()),
> +            )
> +        };
> +
> +        to_result(ret)?;
> +
> +        Ok(WwMutexGuard::new(self))
> +    }

[...]

> +/// A guard that provides exclusive access to the data protected by a
> +// [`WwMutex`] (a.k.a `ww_mutex` on the C side).
> +pub struct WwMutexGuard<'a, T: ?Sized> {
> +    mutex: Pin<&'a WwMutex<'a, T>>,
> +}
> +
> +// SAFETY: `WwMutexGuard` can be transferred across thread boundaries if the data can.
> +unsafe impl<T: ?Sized + Send> Send for WwMutexGuard<'_, T> {}
> +
> +// SAFETY: `WwMutexGuard` can be shared between threads if the data can.
> +unsafe impl<T: ?Sized + Send + Sync> Sync for WwMutexGuard<'_, T> {}
> +
> +impl<'a, T: ?Sized> WwMutexGuard<'a, T> {
> +    /// Creates a new guard for a locked mutex.
> +    fn new(mutex: Pin<&'a WwMutex<'a, T>>) -> Self {
> +        Self { mutex }
> +    }
> +}
> +
> +impl<T: ?Sized> core::ops::Deref for WwMutexGuard<'_, T> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: We hold the lock, so we have exclusive access.

This needs a type invariant on `Self`.

> +        unsafe { &*self.mutex.data.get() }
> +    }
> +}
> +
> +impl<T: ?Sized> core::ops::DerefMut for WwMutexGuard<'_, T> {
> +    fn deref_mut(&mut self) -> &mut Self::Target {
> +        // SAFETY: We hold the lock, so we have exclusive access.
> +        unsafe { &mut *self.mutex.data.get() }
> +    }
> +}
> +
> +impl<T: ?Sized> Drop for WwMutexGuard<'_, T> {
> +    fn drop(&mut self) {
> +        // SAFETY: We hold the lock and are about to release it.
> +        unsafe { bindings::ww_mutex_unlock(self.mutex.as_ptr()) };

What happens when this guard is forgotten and the lock is never
released?

---
Cheers,
Benno

> +    }
> +}

Powered by blists - more mailing lists