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
> 

Can't do that due to:

    error[E0277]: the size for values of type `T` cannot be known at
    compilation time --> rust/kernel/sync/lock/global.rs:119:12
         |
    118  | pub struct GlobalLockedBy<T: ?Sized, B: GlobalLockBackend> {
         |                           - this type parameter needs to be
    `Sized` 119  |     value: UnsafeCell<T>,
         |            ^^^^^^^^^^^^^ doesn't have a size known at
    compile-time |


It's the same reason we did the same thing on GlobalLockedBy like this:

	pub struct GlobalLockedBy<T: ?Sized, B: GlobalLockBackend> {
            _backend: PhantomData<B>,
            value: UnsafeCell<T>,
        }

[...]
> > +    /// Tries to lock the mutex on this [`AcquireCtx`] without
> > blocking.
> > +    ///
> > +    /// Unlike `lock`, no deadlock handling is performed.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// The given `mutex` must be created with the [`Class`] that
> > was used
> > +    /// to initialize this [`AcquireCtx`].
> > +    pub unsafe fn try_lock<'a, T>(&'a self, mutex: &'a Mutex<'a,
> > T>) -> Result<MutexGuard<'a, T>> {
> > +        // SAFETY: By the safety contract, `mutex` belongs to the
> > same `Class`
> > +        // as `self` does.
> > +        unsafe { lock_common(mutex, Some(self), LockKind::Try) }
> > +    }
> > +}
> > +
> > +#[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