Message-ID: <20250623211740.00a5bfea@nimda.home>
Date: Mon, 23 Jun 2025 21:17:40 +0300
From: Onur <work@...rozkan.dev>
To: Boqun Feng <boqun.feng@...il.com>
Cc: linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
 ojeda@...nel.org, alex.gaynor@...il.com, gary@...yguo.net,
 lossin@...nel.org, 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 Mon, 23 Jun 2025 06:26:14 -0700
Boqun Feng <boqun.feng@...il.com> wrote:

> On Sat, Jun 21, 2025 at 09:44:53PM +0300, Onur Özkan wrote:
> [...]
> > +#[pin_data(PinnedDrop)]
> > +pub struct WwAcquireCtx<'a> {
> > +    #[pin]
> > +    inner: Opaque<bindings::ww_acquire_ctx>,
> > +    _p: PhantomData<&'a WwClass>,
> > +}
> > +
> > +// SAFETY: Used in controlled ways during lock acquisition. No
> > race risk. +unsafe impl Sync for WwAcquireCtx<'_> {}
> > +// SAFETY: Doesn't rely on thread-local state. Safe to move
> > between threads. +unsafe impl Send for WwAcquireCtx<'_> {}
> > +
> 
> I don't think `WwAcquireCtx` is `Send`, if you look at C code when
> LOCKDEP is enabled, `ww_acquire_init()` calls a few `mutex_acquire()`
> and expects `ww_acquire_fini()` to call the corresponding
> `mutex_release()`, and these two have to be on the same task. Also I
> don't think there is a need for sending `WwAcquireCtx` to another
> thread.

I wasn't very sure about myself analyzing the C API of ww_mutex thank
you. I will address this along with the other notes you pointed out.

> Besides, the `Sync` of `WwAcquireCtx` also doesn't make sense, I would
> drop it if there is no real usage for now.
> 
> > +impl<'ctx> WwAcquireCtx<'ctx> {
> > +    /// Initializes `Self` with calling C side `ww_acquire_init`
> > inside.
> > +    pub fn new<'class: 'ctx>(ww_class: &'class WwClass) -> impl
> > PinInit<Self> {
> > +        let raw_ptr = ww_class.inner.get();
> > +        pin_init!(WwAcquireCtx {
> > +            inner <- Opaque::ffi_init(|slot: *mut
> > bindings::ww_acquire_ctx| {
> > +                // SAFETY: The caller guarantees that `ww_class`
> > remains valid.
> > +                unsafe { bindings::ww_acquire_init(slot, raw_ptr) }
> > +            }),
> > +            _p: PhantomData
> > +        })
> > +    }
> > +
> > +    /// Marks the end of the acquire phase with C side
> > `ww_acquire_done`.
> > +    ///
> > +    /// After calling this function, no more mutexes can be
> > acquired with this context.
> > +    pub fn done(self: Pin<&mut Self>) {
> > +        // SAFETY: The context is pinned and valid.
> > +        unsafe { bindings::ww_acquire_done(self.inner.get()) };
> > +    }
> > +
> > +    /// Returns a raw pointer to the inner `ww_acquire_ctx`.
> > +    fn as_ptr(&self) -> *mut bindings::ww_acquire_ctx {
> > +        self.inner.get()
> > +    }
> > +}
> > +
> > +#[pinned_drop]
> > +impl PinnedDrop for WwAcquireCtx<'_> {
> > +    fn drop(self: Pin<&mut Self>) {
> > +        // SAFETY: The context is being dropped and is pinned.
> > +        unsafe { bindings::ww_acquire_fini(self.inner.get()) };
> > +    }
> > +}
> > +
> [...]
> > +#[pin_data]
> > +pub struct WwMutex<'a, T: ?Sized> {
> > +    _p: PhantomData<&'a WwClass>,
> > +    #[pin]
> > +    mutex: Opaque<bindings::ww_mutex>,
> > +    data: UnsafeCell<T>,
> > +}
> > +
> > +// 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> {}
> 
> I believe this requires `T` being `Send` as well, because if
> `&WwMutex` is shared between threads, that means any thread can
> access `&mut T` when the lock acquired.
> 
> > +
> > +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,
> > +        })
> > +    }
> > +}
> > +
> [...]
> > +    /// Checks if the mutex is currently locked.
> > +    pub fn is_locked(&self) -> bool {
> 
> Did I miss a reply from you regarding:
> 
> 	https://lore.kernel.org/rust-for-linux/aFReIdlPPg4MmaYX@tardis.local/
> 
> no public is_lock() please. Do an assert_is_locked() instead. We need
> to avoid users from abusing this.

Sorry, I missed that. Perhaps, using `#[cfg(CONFIG_KUNIT)]` e.g.,:

    /// Checks if the mutex is currently locked.
    #[cfg(CONFIG_KUNIT)]
    fn is_locked(&self) -> bool {
        // SAFETY: The mutex is pinned and valid.
        unsafe { bindings::ww_mutex_is_locked(self.mutex.get()) }
    }

would be better? What do you think?

---

On Mon, 23 Jun 2025 11:51:56 +0000
Alice Ryhl <aliceryhl@...gle.com> wrote:

> On Sat, Jun 21, 2025 at 09:44:53PM +0300, 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
> > 
> > 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
> > 
> > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> > index 3dee3139fcd4..28157541e12c 100644
> > --- a/rust/kernel/error.rs
> > +++ b/rust/kernel/error.rs
> > @@ -64,6 +64,7 @@ macro_rules! declare_err {
> >      declare_err!(EPIPE, "Broken pipe.");
> >      declare_err!(EDOM, "Math argument out of domain of func.");
> >      declare_err!(ERANGE, "Math result not representable.");
> > +    declare_err!(EDEADLK, "Resource deadlock avoided.");
> >      declare_err!(EOVERFLOW, "Value too large for defined data
> > type."); declare_err!(ERESTARTSYS, "Restart the system call.");
> >      declare_err!(ERESTARTNOINTR, "System call was interrupted by a
> > signal and will be restarted."); 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..dcb23941813c
> > --- /dev/null
> > +++ b/rust/kernel/sync/lock/ww_mutex.rs
> > @@ -0,0 +1,421 @@
> > +// 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. +//!
> > +//! C header:
> > [`include/linux/ww_mutex.h`](srctree/include/linux/ww_mutex.h) +//!
> > +//! For more information:
> > <https://docs.kernel.org/locking/ww-mutex-design.html> +
> > +use crate::bindings;
> > +use crate::error::to_result;
> > +use crate::prelude::*;
> > +use crate::types::{NotThreadSafe, Opaque};
> > +use core::cell::UnsafeCell;
> > +use core::marker::PhantomData;
> > +
> > +/// 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")); +/// ```
> 
> should we split this into two macros define_ww_class! and
> define_wd_class! to match the C macros?

I don't have a strong opinion on that. I like having small helper
macros but Benno doesn't seem to like having any macro at all for
creating global `WwClass`.

---

On Sun, 22 Jun 2025 11:18:24 +0200
"Benno Lossin" <lossin@...nel.org> wrote:

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

Yes, they can. This is just a helper to make things a bit simpler.
I am fine with either keeping or removing it, I don't have a strong
opinion on this macro.

Alice also made a suggestion about it (it should be visible in this
e-mail as I replied that as well): splitting `define_ww_class!` into two
macros, `define_wd_class!` and `define_ww_class!`.

In my opinion, having this macro doesn't add much value but it also
doesn't introduce any complexity, so it leaves us with a small gain, I
guess.

> > +
> > +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]`.

It should be `unsafe` function, thanks.


---

Regards,
Onur

Powered by blists - more mailing lists