Re: [PATCH v12 4/6] rust: ww_mutex: add Mutex, AcquireCtx and MutexGuard

["Onur Özkan" <work@onurozkan.dev>]

On Mon, 12 Oct 2026 12:40:57 +0100
Gary Guo <gary@garyguo.net> wrote:

> On Mon Oct 12, 2026 at 11:26 AM BST, Onur Özkan wrote:
> > Covers the low-level locking API (lock, try_lock and
> > interruptible variants) and integration with kernel bindings.
> >
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > Signed-off-by: Onur Özkan <work@onurozkan.dev>
> > ---
> >  rust/kernel/sync/lock/ww_mutex.rs             | 434 ++++++++++++++++++
> >  rust/kernel/sync/lock/ww_mutex/acquire_ctx.rs | 149 ++++++
> >  2 files changed, 583 insertions(+)
> >  create mode 100644 rust/kernel/sync/lock/ww_mutex/acquire_ctx.rs
> >
> > diff --git a/rust/kernel/sync/lock/ww_mutex.rs b/rust/kernel/sync/lock/ww_mutex.rs
> > index 727c51cc73af8..2423f89a7e762 100644
> > --- a/rust/kernel/sync/lock/ww_mutex.rs
> > +++ b/rust/kernel/sync/lock/ww_mutex.rs
> > @@ -1,7 +1,441 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  
> >  //! Rust abstractions for the kernel's wound-wait locking primitives.
> > +//!
> > +//! It is designed to avoid deadlocks when locking multiple [`Mutex`]es
> > +//! that belong to the same [`Class`]. Each lock acquisition uses an
> > +//! [`AcquireCtx`] to track ordering and ensure forward progress.
> > +//!
> > +//! See srctree/Documentation/locking/ww-mutex-design.rst for more details.
> >  
> > +use crate::{
> > +    bindings,
> > +    container_of,
> > +    error::to_result,
> > +    prelude::*,
> > +    types::{
> > +        NotThreadSafe,
> > +        Opaque, //
> > +    }, //
> > +};
> > +use core::{
> > +    cell::UnsafeCell,
> > +    marker::PhantomData, //
> > +};
> > +
> > +pub use acquire_ctx::AcquireCtx;
> >  pub use class::Class;
> >  
> > +mod acquire_ctx;
> >  mod class;
> > +
> > +/// A wound-wait (ww) mutex that is powered with deadlock avoidance
> > +/// when acquiring multiple locks of the same [`Class`].
> > +///
> > +/// Each mutex belongs to a [`Class`], which the wound-wait algorithm
> > +/// uses to figure out the order of acquisition and prevent deadlocks.
> > +///
> > +/// # Examples
> > +///
> > +/// ```
> > +/// use kernel::{
> > +///     define_ww_class,
> > +///     sync::{
> > +///         lock::ww_mutex::{
> > +///             AcquireCtx,
> > +///             Class,
> > +///             Mutex, //
> > +///         },
> > +///         Arc,
> > +///     },
> > +/// };
> > +/// use pin_init::stack_pin_init;
> > +///
> > +/// define_ww_class!(SOME_WW_CLASS);
> > +///
> > +/// let mutex = Arc::pin_init(Mutex::new(42, &SOME_WW_CLASS), GFP_KERNEL)?;
> > +/// let ctx = KBox::pin_init(AcquireCtx::new(&SOME_WW_CLASS), GFP_KERNEL)?;
> > +///
> > +/// // SAFETY: The guard is dropped before `ctx`.
> > +/// let guard = unsafe { ctx.lock(&mutex) }?;
> > +/// assert_eq!(*guard, 42);
> > +///
> > +/// # Ok::<(), Error>(())
> > +/// ```
> > +#[pin_data]
> > +#[repr(C)]
> > +pub struct Mutex<'a, T: ?Sized> {
> > +    _p: PhantomData<&'a Class>,
> 
> Do you expect any users that have non-static class?
> 
> So far all in-kernel user of ww mutex has a static class.

Good finding. No, I don't have a non-static class user in mind. The lifetime was
carried over from the earlier versions.

> 
> > +    #[pin]
> > +    inner: Opaque<bindings::ww_mutex>,
> > +    data: UnsafeCell<T>,
> > +}
> > +
> > +impl<'class, T> Mutex<'class, T> {
> > +    /// Initializes [`Mutex`] with the given `data` and [`Class`].
> > +    pub fn new(data: T, class: &'class Class) -> impl PinInit<Self> {
> > +        let class_ptr = class.inner.get();
> > +        pin_init!(Mutex {
> > +            inner <- Opaque::ffi_init(|slot: *mut bindings::ww_mutex| {
> > +                // SAFETY: `class` is valid for the lifetime `'class` captured by `Self`.
> > +                unsafe { bindings::ww_mutex_init(slot, class_ptr) }
> > +            }),
> > +            data: UnsafeCell::new(data),
> > +            _p: PhantomData
> > +        })
> > +    }
> > +}
> > +
> > +impl<'class, T: ?Sized> Mutex<'class, T> {
> > +    /// Checks if this [`Mutex`] is currently locked.
> > +    ///
> > +    /// The returned value is racy as another thread can acquire
> > +    /// or release the lock immediately after this call returns.
> > +    #[inline]
> > +    pub fn is_locked(&self) -> bool {
> > +        // SAFETY: It's safe to call `ww_mutex_is_locked` on
> > +        // a valid mutex.
> > +        unsafe { bindings::ww_mutex_is_locked(self.inner.get()) }
> > +    }
> > +
> > +    /// Locks this [`Mutex`] without [`AcquireCtx`].
> > +    pub fn lock(&self) -> Result<MutexGuard<'_, T>> {
> > +        lock_common(self, None, LockKind::Regular)
> > +    }
> > +
> > +    /// Similar to [`Self::lock`], but can be interrupted by signals.
> > +    pub fn lock_interruptible(&self) -> Result<MutexGuard<'_, T>> {
> > +        lock_common(self, None, LockKind::Interruptible)
> > +    }
> > +
> > +    /// Tries to lock this [`Mutex`] with no [`AcquireCtx`] and without blocking.
> > +    ///
> > +    /// Unlike [`Self::lock`], no deadlock handling is performed.
> > +    pub fn try_lock(&self) -> Result<MutexGuard<'_, T>> {
> 
> `try_lock` failure should return `Option` instead.

Okay.

> 
> > +        lock_common(self, None, LockKind::Try)
> > +    }
> > +}
> > +
> > +impl<'class> Mutex<'class, ()> {
> > +    /// Creates a [`Mutex`] from a raw pointer.
> > +    ///
> > +    /// This function is intended for interoperability with C code.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// The caller must ensure that:
> > +    ///
> > +    /// - `ptr` is a valid pointer to a `ww_mutex`.
> > +    /// - `ptr` must remain valid for the lifetime `'a`.
> > +    /// - ww_class associated with this mutex must be valid for
> > +    ///   the lifetime `'class`.
> > +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::ww_mutex) -> &'a Self {
> > +        // SAFETY: By the safety contract, the caller guarantees that `ptr`
> > +        // points to a valid `ww_mutex` which is the `inner` field of `Mutex`,
> > +        // that it remains valid for the lifetime `'a` and the associated
> > +        // ww_class outlives `'class`.
> > +        //
> > +        // Because [`Mutex`] is `#[repr(C)]`, the `inner` field sits at a
> > +        // stable offset that `container_of!` can safely rely on.
> > +        unsafe { &*container_of!(Opaque::cast_from(ptr), Self, inner) }
> > +    }
> > +}
> > +
> > +// SAFETY: `Mutex` can be sent to another thread if the protected
> > +// data `T` can be.
> > +unsafe impl<T: ?Sized + Send> Send for Mutex<'_, T> {}
> > +
> > +// SAFETY: `Mutex` serialises access to the protected data, so it is
> > +// `Sync` as long as the data is `Send`.
> > +unsafe impl<T: ?Sized + Send> Sync for Mutex<'_, T> {}
> > +
> > +/// A guard that provides exclusive access to the data protected
> > +/// by a [`Mutex`].
> > +///
> > +/// # Invariants
> > +///
> > +/// The guard holds an exclusive lock on the associated [`Mutex`]. The lock is held
> > +/// for the entire lifetime of this guard and is automatically released when the
> > +/// guard is dropped.
> > +#[must_use = "the lock unlocks immediately when the guard is unused"]
> > +pub struct MutexGuard<'a, T: ?Sized> {
> > +    mutex: &'a Mutex<'a, T>,
> > +    _not_send: NotThreadSafe,
> > +}
> > +
> > +impl<'a, T: ?Sized> MutexGuard<'a, T> {
> > +    /// Creates a new guard for the given [`Mutex`].
> > +    fn new(mutex: &'a Mutex<'a, T>) -> Self {
> > +        assert!(mutex.is_locked());
> > +
> > +        // INVARIANT: `lock_common` and `from_raw` ensure that the current task holds the mutex
> > +        // and transfer sole responsibility for unlocking it to this guard.
> > +        Self {
> > +            mutex,
> > +            _not_send: NotThreadSafe,
> > +        }
> > +    }
> > +}
> > +
> > +impl<'a> MutexGuard<'a, ()> {
> > +    /// Creates a [`MutexGuard`] from a raw pointer.
> > +    ///
> > +    /// If the given pointer refers to a mutex that is not locked,
> > +    /// returns [`EINVAL`].
> > +    ///
> > +    /// This function is intended for interoperability with C code.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// The caller must ensure that:
> > +    ///
> > +    /// - `ptr` is a valid pointer to a `ww_mutex`.
> > +    /// - `ptr` must remain valid for the lifetime `'b`.
> > +    /// - The `ww_class` associated with the `ww_mutex` must be valid for the lifetime `'b`.
> > +    /// - The current task holds the mutex.
> > +    /// - The caller transfers responsibility for unlocking the mutex to the returned guard
> > +    ///   and must not unlock it separately (including through another guard).
> > +    /// - Any associated acquire context remains valid until the mutex is unlocked,
> > +    ///   even if the returned guard is forgotten. The context must not be finalized or
> > +    ///   reinitialized before then.
> > +    pub unsafe fn from_raw<'b>(ptr: *mut bindings::ww_mutex) -> Result<MutexGuard<'b, ()>> {
> > +        // SAFETY: By this function's safety contract, the caller guarantees that `ptr` points to a
> > +        // valid `ww_mutex` which is the `inner` field of a `Mutex`. The caller also guarantees
> > +        // that both `ptr` and the associated `ww_class` are valid for the lifetime `'b`.
> > +        let mutex = unsafe { Mutex::from_raw(ptr) };
> > +
> > +        if !mutex.is_locked() {
> > +            return Err(EINVAL);
> > +        }
> 
> Why is this checked despite being an unsafe function. Do you expect users to
> call this without knowing the lock is already locked? I think that's unlikely.
> Perhaps make this a debug_assert instead?

Sure, I don't have strong feeling on keeping it the same or making it
debug_assert.

> 
> > +
> > +        Ok(MutexGuard::new(mutex))
> > +    }
> > +}
> > +
> > +impl<T: ?Sized> core::ops::Deref for MutexGuard<'_, T> {
> > +    type Target = T;
> > +
> 
> `#[inline]` for this and others.
> 
> > +    fn deref(&self) -> &Self::Target {
> > +        // SAFETY: self.mutex is locked, so we have exclusive access.
> > +        unsafe { &*self.mutex.data.get() }
> > +    }
> > +}
> > +
> > +impl<T: ?Sized + Unpin> core::ops::DerefMut for MutexGuard<'_, T> {
> > +    fn deref_mut(&mut self) -> &mut Self::Target {
> > +        // SAFETY: self.mutex is locked, so we have exclusive access.
> > +        unsafe { &mut *self.mutex.data.get() }
> > +    }
> > +}
> > +
> > +impl<T: ?Sized> Drop for MutexGuard<'_, T> {
> > +    #[inline]
> > +    fn drop(&mut self) {
> > +        // SAFETY: self.mutex is locked and are about to release it.
> > +        unsafe { bindings::ww_mutex_unlock(self.mutex.inner.get()) };
> > +    }
> > +}
> > +
> > +// SAFETY: `MutexGuard` can be shared between threads if the data can.
> > +unsafe impl<T: ?Sized + Sync> Sync for MutexGuard<'_, T> {}
> > +
> > +/// Locking kinds used by [`lock_common`] to unify the internal
> > +/// locking logic.
> > +///
> > +/// It's best not to expose this type (and [`lock_common`]) to the
> > +/// kernel, as it allows internal API changes without worrying
> > +/// about breaking external compatibility.
> > +#[derive(Copy, Clone, Debug)]
> > +enum LockKind {
> > +    /// Blocks until lock is acquired.
> > +    Regular,
> > +    /// Blocks but can be interrupted by signals.
> > +    Interruptible,
> > +    /// Does not block, returns immediately if busy.
> > +    Try,
> > +}
> > +
> > +/// Internal helper that unifies the different locking kinds.
> > +///
> > +/// Returns [`EINVAL`] if the [`Mutex`] has a different [`Class`].
> > +fn lock_common<'a, T: ?Sized>(
> > +    mutex: &'a Mutex<'a, T>,
> > +    ctx: Option<&AcquireCtx<'_>>,
> > +    kind: LockKind,
> > +) -> Result<MutexGuard<'a, T>> {
> 
> I think we should have 3 helpers, to avoid having to branch on `kind` which is
> compile-tiome known information.
> 
> The only part between the 3 kinds that is shared is the ctx_ptr computation. I
> think it should be extracted to its own helper, and be used for 3 types of
> locking.

This one also carries the design from earlier versions. Previously we had slow
locks as well so it wasn't only 3. I will fix this one too.

> 
> > +    let mutex_ptr = mutex.inner.get();
> > +
> > +    let ctx_ptr = match ctx {
> > +        Some(acquire_ctx) => {
> > +            let ctx_ptr = acquire_ctx.inner.get();
> > +
> > +            // SAFETY: `ctx_ptr` is a valid pointer for the entire
> > +            // lifetime of `ctx`.
> > +            let ctx_class = unsafe { (*ctx_ptr).ww_class };
> > +
> > +            // SAFETY: `mutex_ptr` is a valid pointer for the entire
> > +            // lifetime of `mutex`.
> > +            let mutex_class = unsafe { (*mutex_ptr).ww_class };
> > +
> > +            // `ctx` and `mutex` must use the same class.
> > +            if ctx_class != mutex_class {
> > +                return Err(EINVAL);
> > +            }
> 
> This is supposed to be a error condition?

Yes, or panic. Both works.

> 
> > +
> > +            ctx_ptr
> > +        }
> > +        None => core::ptr::null_mut(),
> > +    };
> > +
> > +    match kind {
> > +        LockKind::Regular => {
> > +            // SAFETY: `Mutex` is always pinned. If `AcquireCtx` is `Some`, it is pinned,
> > +            // if `None`, it is set to `core::ptr::null_mut()`. Both cases are safe.
> > +            let ret = unsafe { bindings::ww_mutex_lock(mutex_ptr, ctx_ptr) };
> > +
> > +            to_result(ret)?;
> > +        }
> > +        LockKind::Interruptible => {
> > +            // SAFETY: `Mutex` is always pinned. If `AcquireCtx` is `Some`, it is pinned,
> > +            // if `None`, it is set to `core::ptr::null_mut()`. Both cases are safe.
> > +            let ret = unsafe { bindings::ww_mutex_lock_interruptible(mutex_ptr, ctx_ptr) };
> > +
> > +            to_result(ret)?;
> > +        }
> > +        LockKind::Try => {
> > +            // SAFETY: `Mutex` is always pinned. If `AcquireCtx` is `Some`, it is pinned,
> > +            // if `None`, it is set to `core::ptr::null_mut()`. Both cases are safe.
> > +            let ret = unsafe { bindings::ww_mutex_trylock(mutex_ptr, ctx_ptr) };
> > +
> > +            if ret == 0 {
> > +                return Err(EBUSY);
> > +            } else {
> > +                to_result(ret)?;
> > +            }
> > +        }
> > +    };
> > +
> > +    Ok(MutexGuard::new(mutex))
> > +}
> >
> > [snip]
> >
> > diff --git a/rust/kernel/sync/lock/ww_mutex/acquire_ctx.rs b/rust/kernel/sync/lock/ww_mutex/acquire_ctx.rs
> > new file mode 100644
> > index 0000000000000..18f4654e7df26
> > --- /dev/null
> > +++ b/rust/kernel/sync/lock/ww_mutex/acquire_ctx.rs
> > @@ -0,0 +1,149 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Provides [`AcquireCtx`] for managing multiple wound/wait
> > +//! mutexes from the same [`Class`].
> > +
> > +use super::{
> > +    lock_common,
> > +    Class,
> > +    LockKind,
> > +    Mutex,
> > +    MutexGuard, //
> > +};
> > +use crate::{
> > +    bindings,
> > +    prelude::*,
> > +    types::Opaque, //
> > +};
> > +use core::marker::PhantomData;
> > +
> > +/// Groups multiple [`Mutex`]es for deadlock avoidance when acquired
> > +/// with the same [`Class`].
> > +///
> > +/// # Examples
> > +///
> > +/// ```
> > +/// use kernel::{
> > +///     define_ww_class,
> > +///     sync::{
> > +///         lock::ww_mutex::{
> > +///             AcquireCtx,
> > +///             Class,
> > +///             Mutex, //
> > +///         },
> > +///         Arc,
> > +///     },
> > +/// };
> > +/// use pin_init::stack_pin_init;
> > +///
> > +/// define_ww_class!(SOME_WW_CLASS);
> > +///
> > +/// // Create mutexes.
> > +/// let mutex1 = Arc::pin_init(Mutex::new(1, &SOME_WW_CLASS), GFP_KERNEL)?;
> > +/// let mutex2 = Arc::pin_init(Mutex::new(2, &SOME_WW_CLASS), GFP_KERNEL)?;
> > +///
> > +/// // Create acquire context for deadlock avoidance.
> > +/// let ctx = KBox::pin_init(AcquireCtx::new(&SOME_WW_CLASS), GFP_KERNEL)?;
> > +///
> > +/// // SAFETY: The guard is dropped before `ctx`.
> > +/// let guard1 = unsafe { ctx.lock(&mutex1) }?;
> > +/// // SAFETY: The guard is dropped before `ctx`.
> > +/// let guard2 = unsafe { ctx.lock(&mutex2) }?;
> > +///
> > +/// // Mark acquisition phase as complete.
> > +/// ctx.done();
> > +///
> > +/// # Ok::<(), Error>(())
> > +/// ```
> > +#[pin_data(PinnedDrop)]
> > +#[repr(transparent)]
> > +pub struct AcquireCtx<'a> {
> > +    #[pin]
> > +    pub(super) inner: Opaque<bindings::ww_acquire_ctx>,
> > +    _p: PhantomData<&'a Class>,
> > +}
> > +
> > +impl<'class> AcquireCtx<'class> {
> > +    /// Initializes a new [`AcquireCtx`] with the given [`Class`].
> > +    pub fn new(class: &'class Class) -> impl PinInit<Self> {
> > +        let class_ptr = class.inner.get();
> > +        pin_init!(AcquireCtx {
> > +            inner <- Opaque::ffi_init(|slot: *mut bindings::ww_acquire_ctx| {
> > +                // SAFETY: `class` is valid for the lifetime `'class` captured
> > +                // by `AcquireCtx`.
> > +                unsafe { bindings::ww_acquire_init(slot, class_ptr) }
> > +            }),
> > +            _p: PhantomData
> > +        })
> > +    }
> 
> I wonder if a better API would be
> 
>     class.new_context()
> 
> vs.
> 
>     AcquireCtx::new(&class)
> 
> ?

I would prefer to keep AcquireCtx::new(&class). It follows the same constructor
style as Mutex::new(data, &class). I don't see much benefit in moving it to
Class::new_context().

> 
> > +
> > +    /// Creates a [`AcquireCtx`] from a raw pointer.
> > +    ///
> > +    /// This function is intended for interoperability with C code.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// The caller must ensure that `ptr` is a valid pointer to the `inner` field
> > +    /// of [`AcquireCtx`] and that it remains valid for the lifetime `'a`.
> > +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::ww_acquire_ctx) -> &'a Self {
> > +        // SAFETY: By the safety contract, `ptr` is valid to construct `AcquireCtx`.
> > +        unsafe { &*ptr.cast() }
> > +    }
> > +
> > +    /// Marks the end of the acquire phase.
> > +    ///
> > +    /// Calling this function is optional. It is just useful to document
> > +    /// the code and clearly designated the acquire phase from actually
> > +    /// using the locked data structures.
> > +    ///
> > +    /// After calling this function, no more mutexes can be acquired with
> > +    /// this context.
> > +    #[inline]
> > +    pub fn done(&self) {
> > +        // SAFETY: `self.inner` contains a valid, initialized acquire context.
> > +        unsafe { bindings::ww_acquire_done(self.inner.get()) };
> > +    }
> > +
> > +    /// Locks the given [`Mutex`] on this [`AcquireCtx`].
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// On success, the caller must keep this context valid until the mutex is unlocked,
> > +    /// even if the returned guard is forgotten. The context must not be finalized or
> > +    /// reinitialized while the mutex remains locked.
> > +    pub unsafe fn lock<'a, T>(&'a self, mutex: &'a Mutex<'a, T>) -> Result<MutexGuard<'a, T>> {
> > +        lock_common(mutex, Some(self), LockKind::Regular)
> > +    }
> > +
> > +    /// Similar to [`Self::lock`], but can be interrupted by signals.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// The same requirements as [`Self::lock`] apply.
> > +    pub unsafe fn lock_interruptible<'a, T>(
> > +        &'a self,
> > +        mutex: &'a Mutex<'a, T>,
> > +    ) -> Result<MutexGuard<'a, T>> {
> > +        lock_common(mutex, Some(self), LockKind::Interruptible)
> > +    }
> > +
> > +    /// Tries to lock the [`Mutex`] on this [`AcquireCtx`] without blocking.
> > +    ///
> > +    /// Unlike [`Self::lock`], no deadlock handling is performed.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// The same requirements as [`Self::lock`] apply.
> > +    pub unsafe fn try_lock<'a, T>(&'a self, mutex: &'a Mutex<'a, T>) -> Result<MutexGuard<'a, T>> {
> > +        lock_common(mutex, Some(self), LockKind::Try)
> > +    }
> > +}
> > +
> > +#[pinned_drop]
> > +impl PinnedDrop for AcquireCtx<'_> {
> > +    fn drop(self: Pin<&mut Self>) {
> 
> I wonder if we can just do
> 
>     let acquired = unsafe { (*self.inner.get()).acquired };
>     assert!(!acquired);
> 
> to get rid of the `unsafe`. Somewhat similar to what we do with srcu. Of course,
> ultimately we want to use `!Leak` for this and can remove the check.

I didn't understand this. You also wrote an unsafe code?

> 
> Thoughts?
> 
> Best,
> Gary
> 
> > +        // SAFETY: The locking methods require callers to release all acquired locks
> > +        // before this context is dropped.
> > +        unsafe { bindings::ww_acquire_fini(self.inner.get()) };
> > +    }
> > +}
> 
>