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

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

On Fri, 21 Nov 2025 16:00:18 -0500
Lyude Paul <lyude@redhat.com> wrote:

> Feedback down below:
> 
> On Sat, 2025-11-01 at 19:10 +0300, Onur Özkan wrote:
> > Implements full locking API (lock, try_lock, slow path,
> > interruptible variants) and integration with kernel bindings.
> > 
> > Signed-off-by: Onur Özkan <work@onurozkan.dev>
> > ---
> >  rust/kernel/sync/lock/ww_mutex.rs             | 276
> > ++++++++++++++++++ rust/kernel/sync/lock/ww_mutex/acquire_ctx.rs |
> > 211 +++++++++++++ 2 files changed, 487 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
> > 727c51cc73af..2a9c1c20281b 100644 ---
> > a/rust/kernel/sync/lock/ww_mutex.rs +++
> > b/rust/kernel/sync/lock/ww_mutex.rs @@ -1,7 +1,283 @@
> >  // 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. 
> > +use crate::error::to_result;
> > +use crate::prelude::*;
> > +use crate::types::{NotThreadSafe, Opaque};
> > +use crate::{bindings, container_of};
> > +
> > +use core::cell::UnsafeCell;
> > +use core::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::c_str;
> > +/// use kernel::sync::Arc;
> > +/// use kernel::sync::lock::ww_mutex::{AcquireCtx, Class, Mutex};
> > +/// use pin_init::stack_pin_init;
> > +///
> > +/// stack_pin_init!(let class =
> > Class::new_wound_wait(c_str!("some_class"))); +/// let mutex =
> > Arc::pin_init(Mutex::new(42, &class), GFP_KERNEL)?; +///
> > +/// let ctx = KBox::pin_init(AcquireCtx::new(&class), GFP_KERNEL)?;
> > +///
> > +/// // SAFETY: Both `ctx` and `mutex` uses the same class.
> > +/// let guard = unsafe { ctx.lock(&mutex)? };
> > +/// assert_eq!(*guard, 42);
> > +///
> > +/// # Ok::<(), Error>(())
> > +/// ```
> > +#[pin_data]
> 
> You're missing a #[repr(C)] here, because… (cont. down below)
> 
> > +pub struct Mutex<'a, T: ?Sized> {
> > +    #[pin]
> > +    inner: Opaque<bindings::ww_mutex>,
> > +    _p: PhantomData<&'a Class>,
> 
> This should be at the bottom of the class
> 
> > +    data: UnsafeCell<T>,
> > +}
> > +
> > +// 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` can be shared across threads if the protected
> > +// data `T` can be.
> > +unsafe impl<T: ?Sized + Send + Sync> Sync for Mutex<'_, T> {}
> 
> Looks like there's a funny pitfall here! According to Alice Rhyl (see
> the convo we had in zulip), for this to actually be safe we need to
> add an empty Drop implementation to Mutex to ensure the compiler
> doesn't actually allow it to be dropped after the lock class. So
> would be good to add that + a comment of why it's needed
> 

Mutex cannot live longer than the Class it includes. Which means this
case (compiler dropping Mutex because Class was dropped) can't ever
happen. Or, am I missing something here?

[...]
> > +}
> > +
> > +#[pinned_drop]
> > +impl PinnedDrop for AcquireCtx<'_> {
> > +    fn drop(self: Pin<&mut Self>) {
> > +        // SAFETY: Given the lifetime bounds we know no locks are
> > held,
> > +        // so calling `ww_acquire_fini` is safe.
> > +        unsafe { bindings::ww_acquire_fini(self.inner.get()) };
> > +    }
> > +}
> 

-Onur