[PATCH V3] implement `ww_mutex` abstraction for the Rust tree

[PATCH V3] implement `ww_mutex` abstraction for the Rust tree

[Onur Özkan]

From: onur-ozkan <work@onurozkan.dev>

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@gmail.com
Signed-off-by: Onur Özkan <work@onurozkan.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

diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 0f1b5d115985..fd071cbe20a1 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -44,3 +44,4 @@
 #include "wait.c"
 #include "workqueue.c"
 #include "xarray.c"
+#include "ww_mutex.c"
diff --git a/rust/helpers/ww_mutex.c b/rust/helpers/ww_mutex.c
new file mode 100644
index 000000000000..61a487653394
--- /dev/null
+++ b/rust/helpers/ww_mutex.c
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/ww_mutex.h>
+
+void rust_helper_ww_mutex_init(struct ww_mutex *lock, struct ww_class *ww_class)
+{
+	ww_mutex_init(lock, ww_class);
+}
+
+void rust_helper_ww_acquire_init(struct ww_acquire_ctx *ctx, struct ww_class *ww_class)
+{
+	ww_acquire_init(ctx, ww_class);
+}
+
+void rust_helper_ww_acquire_done(struct ww_acquire_ctx *ctx)
+{
+	ww_acquire_done(ctx);
+}
+
+void rust_helper_ww_acquire_fini(struct ww_acquire_ctx *ctx)
+{
+	ww_acquire_fini(ctx);
+}
+
+void rust_helper_ww_mutex_lock_slow(struct ww_mutex *lock, struct ww_acquire_ctx *ctx)
+{
+	ww_mutex_lock_slow(lock, ctx);
+}
+
+int rust_helper_ww_mutex_lock_slow_interruptible(struct ww_mutex *lock, struct ww_acquire_ctx *ctx)
+{
+	return ww_mutex_lock_slow_interruptible(lock, ctx);
+}
+
+bool rust_helper_ww_mutex_is_locked(struct ww_mutex *lock)
+{
+	return ww_mutex_is_locked(lock);
+}
+
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.
+///
+/// # 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 {
+    ($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() },
+                    // 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`.
+///
+/// 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>,
+}
+
+// SAFETY: `WwClass` can be safely accessed from multiple threads concurrently.
+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`.
+///
+/// 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.
+///
+/// # Examples
+///
+/// ```
+/// use kernel::sync::lock::ww_mutex::{WwClass, WwAcquireCtx, WwMutex};
+/// use kernel::c_str;
+/// use pin_init::stack_pin_init;
+/// use kernel::alloc::KBox;
+///
+/// stack_pin_init!(let class = WwClass::new_wound_wait(c_str!("my_class")));
+///
+/// // Create mutexes
+/// stack_pin_init!(let mutex1 = WwMutex::new(1, &class));
+/// stack_pin_init!(let mutex2 = WwMutex::new(2, &class));
+///
+/// // Create acquire context for deadlock avoidance
+/// let mut ctx = KBox::pin_init(WwAcquireCtx::new(&class), GFP_KERNEL).unwrap();
+///
+/// // Acquire multiple locks safely
+/// let guard1 = mutex1.as_ref().lock(Some(&ctx)).unwrap();
+/// let guard2 = mutex2.as_ref().lock(Some(&ctx)).unwrap();
+///
+/// // Mark acquisition phase as complete
+/// ctx.as_mut().done();
+///
+/// # Ok::<(), Error>(())
+/// ```
+#[pin_data(PinnedDrop)]
+pub struct WwAcquireCtx<'a> {
+    #[pin]
+    inner: Opaque<bindings::ww_acquire_ctx>,
+    _p: PhantomData<&'a WwClass>,
+}
+
+// SAFETY: `WwAcquireCtx` can be safely accessed from multiple threads concurrently.
+unsafe impl Sync for WwAcquireCtx<'_> {}
+// SAFETY: `WwAcquireCtx` can be shared between threads.
+unsafe impl Send for WwAcquireCtx<'_> {}
+
+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()) };
+    }
+}
+
+/// 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();
+///
+/// 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>,
+}
+
+// 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>,
+        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))
+    }
+
+    /// Locks the mutex with the given acquire context, interruptible.
+    ///
+    /// Similar to `lock`, but can be interrupted by signals.
+    pub fn lock_interruptible<'a>(
+        self: Pin<&'a Self>,
+        ctx: Option<&WwAcquireCtx<'_>>,
+    ) -> Result<WwMutexGuard<'a, T>> {
+        // SAFETY: The mutex is pinned and valid.
+        let ret = unsafe {
+            bindings::ww_mutex_lock_interruptible(
+                self.mutex.get(),
+                ctx.map_or(core::ptr::null_mut(), |c| c.as_ptr()),
+            )
+        };
+
+        to_result(ret)?;
+
+        Ok(WwMutexGuard::new(self))
+    }
+
+    /// Locks the mutex in the slow path after a die case.
+    ///
+    /// This should be called after releasing all held mutexes when `lock` returns `EDEADLK`.
+    pub fn lock_slow<'a>(
+        self: Pin<&'a Self>,
+        ctx: &WwAcquireCtx<'_>,
+    ) -> Result<WwMutexGuard<'a, T>> {
+        // SAFETY: The mutex is pinned and valid, and we're in the slow path.
+        unsafe { bindings::ww_mutex_lock_slow(self.mutex.get(), ctx.as_ptr()) };
+
+        Ok(WwMutexGuard::new(self))
+    }
+
+    /// Locks the mutex in the slow path after a die case, interruptible.
+    pub fn lock_slow_interruptible<'a>(
+        self: Pin<&'a Self>,
+        ctx: &WwAcquireCtx<'_>,
+    ) -> Result<WwMutexGuard<'a, T>> {
+        // SAFETY: The mutex is pinned and valid, and we are in the slow path.
+        let ret =
+            unsafe { bindings::ww_mutex_lock_slow_interruptible(self.mutex.get(), ctx.as_ptr()) };
+
+        to_result(ret)?;
+
+        Ok(WwMutexGuard::new(self))
+    }
+
+    /// Tries to lock the mutex without blocking.
+    pub fn try_lock<'a>(
+        self: Pin<&'a Self>,
+        ctx: Option<&WwAcquireCtx<'_>>,
+    ) -> Result<WwMutexGuard<'a, T>> {
+        // SAFETY: The mutex is pinned and valid.
+        let ret = unsafe {
+            bindings::ww_mutex_trylock(
+                self.mutex.get(),
+                ctx.map_or(core::ptr::null_mut(), |c| c.as_ptr()),
+            )
+        };
+
+        if ret == 0 {
+            return Err(EBUSY);
+        }
+
+        to_result(if ret < 0 { ret } else { 0 })?;
+
+        Ok(WwMutexGuard::new(self))
+    }
+
+    /// Checks if the mutex is currently locked.
+    pub fn is_locked(self: Pin<&Self>) -> bool {
+        // SAFETY: The mutex is pinned and valid.
+        unsafe { bindings::ww_mutex_is_locked(self.mutex.get()) }
+    }
+
+    /// Returns a raw pointer to the inner mutex.
+    fn as_ptr(&self) -> *mut bindings::ww_mutex {
+        self.mutex.get()
+    }
+}
+
+/// 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.
+        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()) };
+    }
+}
+
+#[kunit_tests(rust_kernel_ww_mutex)]
+mod tests {
+    use crate::alloc::KBox;
+    use crate::c_str;
+    use crate::prelude::*;
+    use pin_init::stack_pin_init;
+
+    use super::*;
+
+    // A simple coverage on `define_ww_class` macro.
+    define_ww_class!(TEST_WOUND_WAIT_CLASS, wound_wait, c_str!("test_wound_wait"));
+    define_ww_class!(TEST_WAIT_DIE_CLASS, wait_die, c_str!("test_wait_die"));
+
+    #[test]
+    fn test_ww_mutex_basic_lock_unlock() {
+        stack_pin_init!(let class = WwClass::new_wound_wait(c_str!("test_mutex_class")));
+
+        stack_pin_init!(let mutex = WwMutex::new(42, &class));
+
+        // Lock without context
+        let guard = mutex.as_ref().lock(None).unwrap();
+        assert_eq!(*guard, 42);
+
+        // Drop the lock
+        drop(guard);
+
+        // Lock it again
+        let mut guard = mutex.as_ref().lock(None).unwrap();
+        *guard = 100;
+        assert_eq!(*guard, 100);
+    }
+
+    #[test]
+    fn test_ww_mutex_trylock() {
+        stack_pin_init!(let class = WwClass::new_wound_wait(c_str!("trylock_class")));
+
+        stack_pin_init!(let mutex = WwMutex::new(123, &class));
+
+        // trylock on unlocked mutex should succeed
+        let guard = mutex.as_ref().try_lock(None).unwrap();
+        assert_eq!(*guard, 123);
+        drop(guard);
+
+        // lock it first
+        let _guard1 = mutex.as_ref().lock(None).unwrap();
+
+        // trylock should fail with EBUSY when already locked
+        let result = mutex.as_ref().try_lock(None);
+        match result {
+            Err(e) => assert_eq!(e, EBUSY),
+            Ok(_) => panic!("Expected `EBUSY` but got success"),
+        }
+    }
+
+    #[test]
+    fn test_ww_mutex_is_locked() {
+        stack_pin_init!(let class = WwClass::new_wait_die(c_str!("locked_check_class")));
+
+        stack_pin_init!(let mutex = WwMutex::new("hello", &class));
+
+        // should not be locked initially
+        assert!(!mutex.as_ref().is_locked());
+
+        let guard = mutex.as_ref().lock(None).unwrap();
+        assert!(mutex.as_ref().is_locked());
+
+        drop(guard);
+        assert!(!mutex.as_ref().is_locked());
+    }
+
+    #[test]
+    fn test_ww_acquire_context() {
+        stack_pin_init!(let class = WwClass::new_wound_wait(c_str!("ctx_class")));
+
+        stack_pin_init!(let mutex1 = WwMutex::new(1, &class));
+        stack_pin_init!(let mutex2 = WwMutex::new(2, &class));
+
+        let mut ctx = KBox::pin_init(WwAcquireCtx::new(&class), GFP_KERNEL).unwrap();
+
+        // acquire multiple mutexes with same context
+        let guard1 = mutex1.as_ref().lock(Some(&ctx)).unwrap();
+        let guard2 = mutex2.as_ref().lock(Some(&ctx)).unwrap();
+
+        assert_eq!(*guard1, 1);
+        assert_eq!(*guard2, 2);
+
+        ctx.as_mut().done();
+
+        // we shouldn't be able to lock once it's `done`.
+        assert!(mutex1.as_ref().lock(Some(&ctx)).is_err());
+        assert!(mutex2.as_ref().lock(Some(&ctx)).is_err());
+    }
+
+    #[test]
+    fn test_with_global_classes() {
+        stack_pin_init!(let wound_wait_mutex = WwMutex::new(100, &TEST_WOUND_WAIT_CLASS));
+        stack_pin_init!(let wait_die_mutex = WwMutex::new(200, &TEST_WAIT_DIE_CLASS));
+
+        let ww_guard = wound_wait_mutex.as_ref().lock(None).unwrap();
+        let wd_guard = wait_die_mutex.as_ref().lock(None).unwrap();
+
+        assert_eq!(*ww_guard, 100);
+        assert_eq!(*wd_guard, 200);
+
+        assert!(wound_wait_mutex.as_ref().is_locked());
+        assert!(wait_die_mutex.as_ref().is_locked());
+
+        drop(ww_guard);
+        drop(wd_guard);
+
+        assert!(!wound_wait_mutex.as_ref().is_locked());
+        assert!(!wait_die_mutex.as_ref().is_locked());
+    }
+}
2.49.0

Re: [PATCH V3] implement `ww_mutex` abstraction for the Rust tree

[Peter Zijlstra]

On Thu, Jun 19, 2025 at 05:06:56PM +0300, Onur Özkan wrote:
> +bool rust_helper_ww_mutex_is_locked(struct ww_mutex *lock)
> +{
> +	return ww_mutex_is_locked(lock);
> +}

Do we really need this? In general I dislike all the _is_locked()
functions and would ideally like to remove them.

Pretty much the only useful pattern for any of the _is_locked()
functions is:

  WARN_ON_ONCE(!foo_is_locked(&foo));

Any other use is dodgy as heck.

Re: [PATCH V3] implement `ww_mutex` abstraction for the Rust tree

[Onur]

On Thu, 19 Jun 2025 16:14:01 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Jun 19, 2025 at 05:06:56PM +0300, Onur Özkan wrote:
> > +bool rust_helper_ww_mutex_is_locked(struct ww_mutex *lock)
> > +{
> > +	return ww_mutex_is_locked(lock);
> > +}
> 
> Do we really need this? In general I dislike all the _is_locked()
> functions and would ideally like to remove them.
> 
> Pretty much the only useful pattern for any of the _is_locked()
> functions is:
> 
>   WARN_ON_ONCE(!foo_is_locked(&foo));
> 
> Any other use is dodgy as heck.
> 
> 

It's an abstraction of `ww_mutex_is_locked`. Since this is an
abstraction module, as long as `ww_mutex_is_locked` exists I think
we should keep it. FWIW it's also quite useful for tests.

Re: [PATCH V3] implement `ww_mutex` abstraction for the Rust tree

[Benno Lossin]

On Thu Jun 19, 2025 at 4:06 PM CEST, Onur Özkan wrote:
> From: onur-ozkan <work@onurozkan.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@gmail.com
> Signed-off-by: Onur Özkan <work@onurozkan.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

> +    }
> +}

Re: [PATCH V3] implement `ww_mutex` abstraction for the Rust tree

[Benno Lossin]

On Thu Jun 19, 2025 at 4:06 PM CEST, Onur Özkan wrote:
> From: onur-ozkan <work@onurozkan.dev>
>
> 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.

I don't know the design of `struct ww_mutex`, but from the code below I
gathered that it has some special error return values that signify that
one should release other locks.

Did anyone think about making a more Rusty API that would allow one to
try to lock multiple mutexes at the same time (in a specified order) and
if it fails, it would do the resetting automatically?

---
Cheers,
Benno

> Link: https://rust-for-linux.zulipchat.com/#narrow/channel/291566-Library/topic/Writing.20up.20wrappers.20for.20ww_mutex.3F/with/524269974
> Suggested-by: thatslyude@gmail.com
> Signed-off-by: Onur Özkan <work@onurozkan.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

Re: [PATCH V3] implement `ww_mutex` abstraction for the Rust tree

[Alice Ryhl]

On Thu, Jun 19, 2025 at 4:33 PM Onur <work@onurozkan.dev> wrote:
>
> On Thu, 19 Jun 2025 16:14:01 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
>
> > On Thu, Jun 19, 2025 at 05:06:56PM +0300, Onur Özkan wrote:
> > > +bool rust_helper_ww_mutex_is_locked(struct ww_mutex *lock)
> > > +{
> > > +   return ww_mutex_is_locked(lock);
> > > +}
> >
> > Do we really need this? In general I dislike all the _is_locked()
> > functions and would ideally like to remove them.
> >
> > Pretty much the only useful pattern for any of the _is_locked()
> > functions is:
> >
> >   WARN_ON_ONCE(!foo_is_locked(&foo));
> >
> > Any other use is dodgy as heck.
> >
> >
>
> It's an abstraction of `ww_mutex_is_locked`. Since this is an
> abstraction module, as long as `ww_mutex_is_locked` exists I think
> we should keep it. FWIW it's also quite useful for tests.

We're not just adding copies of all of the C methods - instead we
focus on the things we have a use-case for. If you're using them in
tests, then that could make sense, but otherwise you shouldn't add
them.

Alice

Re: [PATCH V3] implement `ww_mutex` abstraction for the Rust tree

[Peter Zijlstra]

On Thu, Jun 19, 2025 at 04:44:01PM +0200, Alice Ryhl wrote:
> On Thu, Jun 19, 2025 at 4:33 PM Onur <work@onurozkan.dev> wrote:
> >
> > On Thu, 19 Jun 2025 16:14:01 +0200
> > Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > > On Thu, Jun 19, 2025 at 05:06:56PM +0300, Onur Özkan wrote:
> > > > +bool rust_helper_ww_mutex_is_locked(struct ww_mutex *lock)
> > > > +{
> > > > +   return ww_mutex_is_locked(lock);
> > > > +}
> > >
> > > Do we really need this? In general I dislike all the _is_locked()
> > > functions and would ideally like to remove them.
> > >
> > > Pretty much the only useful pattern for any of the _is_locked()
> > > functions is:
> > >
> > >   WARN_ON_ONCE(!foo_is_locked(&foo));
> > >
> > > Any other use is dodgy as heck.
> > >
> > >
> >
> > It's an abstraction of `ww_mutex_is_locked`. Since this is an
> > abstraction module, as long as `ww_mutex_is_locked` exists I think
> > we should keep it. FWIW it's also quite useful for tests.
> 
> We're not just adding copies of all of the C methods - instead we
> focus on the things we have a use-case for. If you're using them in
> tests, then that could make sense, but otherwise you shouldn't add
> them.

It might make sense to include the assert in the method. That is,
instead of providing .is_locked() that returns a boolean, have a void
method .assert_is_locked() that traps if not locked.

Re: [PATCH V3] implement `ww_mutex` abstraction for the Rust tree

[Boqun Feng]

On Thu, Jun 19, 2025 at 04:53:34PM +0200, Peter Zijlstra wrote:
> On Thu, Jun 19, 2025 at 04:44:01PM +0200, Alice Ryhl wrote:
> > On Thu, Jun 19, 2025 at 4:33 PM Onur <work@onurozkan.dev> wrote:
> > >
> > > On Thu, 19 Jun 2025 16:14:01 +0200
> > > Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > > On Thu, Jun 19, 2025 at 05:06:56PM +0300, Onur Özkan wrote:
> > > > > +bool rust_helper_ww_mutex_is_locked(struct ww_mutex *lock)
> > > > > +{
> > > > > +   return ww_mutex_is_locked(lock);
> > > > > +}
> > > >
> > > > Do we really need this? In general I dislike all the _is_locked()
> > > > functions and would ideally like to remove them.
> > > >
> > > > Pretty much the only useful pattern for any of the _is_locked()
> > > > functions is:
> > > >
> > > >   WARN_ON_ONCE(!foo_is_locked(&foo));
> > > >
> > > > Any other use is dodgy as heck.
> > > >
> > > >
> > >
> > > It's an abstraction of `ww_mutex_is_locked`. Since this is an
> > > abstraction module, as long as `ww_mutex_is_locked` exists I think
> > > we should keep it. FWIW it's also quite useful for tests.
> > 
> > We're not just adding copies of all of the C methods - instead we
> > focus on the things we have a use-case for. If you're using them in

Agreed. And as Peter mentioned allowing a public API of is_locked()
doesn't make much sense.

> > tests, then that could make sense, but otherwise you shouldn't add
> > them.
> 
> It might make sense to include the assert in the method. That is,
> instead of providing .is_locked() that returns a boolean, have a void
> method .assert_is_locked() that traps if not locked.
> 

Moreover, we should also make it not public in the beginning if there is
no real user, which would still be usable in tests. I.e.

    impl<T: ..> WwMutex<'_, T> {
        fn assert_is_locked() { }
    }

Regards,
Boqun

> 

Re: [PATCH V3] implement `ww_mutex` abstraction for the Rust tree

[Onur]

On Thu, 19 Jun 2025 16:42:15 +0200
"Benno Lossin" <lossin@kernel.org> wrote:

> On Thu Jun 19, 2025 at 4:06 PM CEST, Onur Özkan wrote:
> > From: onur-ozkan <work@onurozkan.dev>
> 
> Can you double-check your name in your git config? This doesn't match
> the Signed-off-by below.

That's strange. It should be "Onur Özkan", gitconfig is the correct
one. I will re-check that on V4 patch.

> > <work@onurozkan.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.

I will try to separate them. It's my first big (relatively) patch-based
work. I am still tryin to get used to it :)


> > +/// ```
> > +/// 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?

It's for creating global classes which was suggested in previous
reviews. A similar approach is used on the C side as well with
`DEFINE_WD_CLASS`.

> > +    ($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.

I didn't want to duplicate the docs. I will update it (and others) on
V4.

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

It was for `define_ww_class` macro. It obviously says you shouldn't do
that but sure, I can undo the `pub` and create a `const` function for
`define_ww_class`.


Regards,
Onur