Message-Id: <6D30FEF7-07E7-4851-A7A2-76649AD0B217@collabora.com>
Date: Fri, 5 Sep 2025 16:42:09 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Onur Özkan <work@...rozkan.dev>
Cc: rust-for-linux@...r.kernel.org,
 linux-kernel@...r.kernel.org,
 lossin@...nel.org,
 lyude@...hat.com,
 ojeda@...nel.org,
 alex.gaynor@...il.com,
 boqun.feng@...il.com,
 gary@...yguo.net,
 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 v6 6/7] rust: ww_mutex/exec: add high-level API

Hi Onur,

> On 3 Sep 2025, at 10:13, Onur Özkan <work@...rozkan.dev> wrote:
> 
> `ExecContext` is a helper built on top of ww_mutex

Again, I wonder what people think about this particular name.

> that provides a retrying interface for lock acquisition.
> When `EDEADLK` is hit, it drops all held locks, resets
> the acquire context and retries the given (by the user)
> locking algorithm until it succeeds.
> 
> The API keeps track of acquired locks, cleans them up
> automatically and allows data access to the protected
> data through `with_locked()`. The `lock_all()` helper
> allows implementing multi-mutex algorithms in a simpler
> and less error-prone way while keeping the ww_mutex
> semantics.
> 

Great, this was exactly what I was looking for! :)

> Signed-off-by: Onur Özkan <work@...rozkan.dev>
> ---
> rust/kernel/sync/lock/ww_mutex.rs      |   2 +
> rust/kernel/sync/lock/ww_mutex/exec.rs | 176 +++++++++++++++++++++++++
> 2 files changed, 178 insertions(+)
> create mode 100644 rust/kernel/sync/lock/ww_mutex/exec.rs
> 
> diff --git a/rust/kernel/sync/lock/ww_mutex.rs b/rust/kernel/sync/lock/ww_mutex.rs
> index b415d6deae9b..7de6578513e5 100644
> --- a/rust/kernel/sync/lock/ww_mutex.rs
> +++ b/rust/kernel/sync/lock/ww_mutex.rs
> @@ -16,6 +16,8 @@
> use core::cell::UnsafeCell;
> use core::marker::PhantomData;
> 
> +pub mod exec;
> +
> /// Create static [`WwClass`] instances.
> ///
> /// # Examples
> diff --git a/rust/kernel/sync/lock/ww_mutex/exec.rs b/rust/kernel/sync/lock/ww_mutex/exec.rs
> new file mode 100644
> index 000000000000..2f1fc540f0b8
> --- /dev/null
> +++ b/rust/kernel/sync/lock/ww_mutex/exec.rs
> @@ -0,0 +1,176 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! A high-level [`WwMutex`] execution helper.
> +//!
> +//! Provides a retrying lock mechanism on top of [`WwMutex`] and [`WwAcquireCtx`].
> +//! It detects [`EDEADLK`] and handles it by rolling back and retrying the
> +//! user-supplied locking algorithm until success.
> +
> +use crate::prelude::*;
> +use crate::sync::lock::ww_mutex::{WwAcquireCtx, WwClass, WwMutex, WwMutexGuard};
> +use core::ptr;
> +
> +/// High-level execution type for ww_mutex.
> +///
> +/// Tracks a series of locks acquired under a common [`WwAcquireCtx`].
> +/// It ensures proper cleanup and retry mechanism on deadlocks and provides
> +/// type-safe access to locked data via [`with_locked`].
> +///
> +/// Typical usage is through [`lock_all`], which retries a user-supplied
> +/// locking algorithm until it succeeds without deadlock.
> +pub struct ExecContext<'a> {
> +    class: &'a WwClass,
> +    acquire: Pin<KBox<WwAcquireCtx<'a>>>,
> +    taken: KVec<WwMutexGuard<'a, ()>>,
> +}
> +
> +impl<'a> Drop for ExecContext<'a> {
> +    fn drop(&mut self) {
> +        self.release_all_locks();

If we move this to the acquire context, then we can do away with this drop impl.

> +    }
> +}
> +
> +impl<'a> ExecContext<'a> {
> +    /// Creates a new [`ExecContext`] for the given lock class.
> +    ///
> +    /// All locks taken through this context must belong to the same class.
> +    ///
> +    /// TODO: Add some safety mechanism to ensure classes are not different.

core::ptr::eq()?

> +    pub fn new(class: &'a WwClass) -> Result<Self> {
> +        Ok(Self {
> +            class,
> +            acquire: KBox::pin_init(WwAcquireCtx::new(class), GFP_KERNEL)?,
> +            taken: KVec::new(),
> +        })
> +    }
> +
> +    /// Attempts to lock a [`WwMutex`] and records the guard.
> +    ///
> +    /// Returns [`EDEADLK`] if lock ordering would cause a deadlock.
> +    pub fn lock<T>(&mut self, mutex: &'a WwMutex<'a, T>) -> Result<()> {
> +        let guard = self.acquire.lock(mutex)?;
> +        // SAFETY: Type is erased for storage. Actual access uses `with_locked`
> +        // which safely casts back.

Why?

> +        let erased: WwMutexGuard<'a, ()> = unsafe { core::mem::transmute(guard) };

We should really try our very best to avoid transmuting things.

Why can’t you store a KVec<MutexGuard<‘a, T>>? Seems straightforward if
you add a T parameter to ExecContext.

Also, someone correct me if I am wrong, but users can explicitly have T be
e.g.: KBox<dyn SomeTrait> if they want to.

> +        self.taken.push(erased, GFP_KERNEL)?;
> +
> +        Ok(())
> +    }
> +
> +    /// Runs `locking_algorithm` until success with retrying on deadlock.
> +    ///
> +    /// `locking_algorithm` should attempt to acquire all needed locks.
> +    /// If [`EDEADLK`] is detected, this function will roll back, reset
> +    /// the context and retry automatically.
> +    ///
> +    /// Once all locks are acquired successfully, `on_all_locks_taken` is
> +    /// invoked for exclusive access to the locked values. Afterwards, all
> +    /// locks are released.
> +    ///
> +    /// # Example
> +    ///
> +    /// ```
> +    /// use kernel::alloc::KBox;
> +    /// use kernel::c_str;
> +    /// use kernel::prelude::*;
> +    /// use kernel::sync::Arc;
> +    /// use kernel::sync::lock::ww_mutex;
> +    /// use pin_init::stack_pin_init;
> +    ///
> +    /// stack_pin_init!(let class = ww_mutex::WwClass::new_wound_wait(c_str!("lock_all_example")));
> +    ///
> +    /// let mutex1 = Arc::pin_init(ww_mutex::WwMutex::new(0, &class), GFP_KERNEL)?;
> +    /// let mutex2 = Arc::pin_init(ww_mutex::WwMutex::new(0, &class), GFP_KERNEL)?;
> +    /// let mut ctx = KBox::pin_init(ww_mutex::exec::ExecContext::new(&class)?, GFP_KERNEL)?;
> +    ///
> +    /// ctx.lock_all(
> +    ///     |ctx| {
> +    ///         // Try to lock both mutexes.
> +    ///         ctx.lock(&mutex1)?;
> +    ///         ctx.lock(&mutex2)?;
> +    ///
> +    ///         Ok(())
> +    ///     },
> +    ///     |ctx| {
> +    ///         // Safely mutate both values while holding the locks.
> +    ///         ctx.with_locked(&mutex1, |v| *v += 1)?;
> +    ///         ctx.with_locked(&mutex2, |v| *v += 1)?;
> +    ///
> +    ///         Ok(())
> +    ///     },
> +    /// )?;

Can you add intermediary variables to hold the closures, just for extra clarity?

i.e.:

let locking_algorithm = …;
let on_all_locks_taken = …;

This is of course identical, but it conveys the meaning just a bit better.

> +    ///
> +    /// # Ok::<(), Error>(())
> +    /// ```
> +    pub fn lock_all<T, Y, Z>(
> +        &mut self,
> +        mut locking_algorithm: T,
> +        mut on_all_locks_taken: Y,
> +    ) -> Result<Z>
> +    where
> +        T: FnMut(&mut ExecContext<'a>) -> Result<()>,

Just “Result”.

> +        Y: FnMut(&mut ExecContext<'a>) -> Result<Z>,
> +    {
> +        loop {
> +            match locking_algorithm(self) {
> +                Ok(()) => {
> +                    // All locks in `locking_algorithm` succeeded.
> +                    // The user can now safely use them in `on_all_locks_taken`.
> +                    let res = on_all_locks_taken(self);
> +                    self.release_all_locks();
> +
> +                    return res;
> +                }
> +                Err(e) if e == EDEADLK => {
> +                    // Deadlock detected, retry from scratch.
> +                    self.cleanup_on_deadlock()?;
> +                    continue;
> +                }
> +                Err(e) => {
> +                    return Err(e);
> +                }
> +            }
> +        }
> +    }

This apparently looks ok.

> +
> +    /// Executes `f` with a mutable reference to the data behind `mutex`.
> +    ///
> +    /// Fails with [`EINVAL`] if the mutex was not locked in this context.
> +    pub fn with_locked<T, Y>(
> +        &mut self,
> +        mutex: &'a WwMutex<'a, T>,
> +        f: impl FnOnce(&mut T) -> Y,
> +    ) -> Result<Y> {
> +        // Find the matching guard.
> +        for guard in &mut self.taken {
> +            if mutex.as_ptr() == guard.mutex.as_ptr() {

core::ptr::eq() ?

> +                // SAFETY: We know this guard belongs to `mutex` and holds the lock.
> +                let typed = unsafe { &mut *ptr::from_mut(guard).cast::<WwMutexGuard<'a, T>>() };
> +                return Ok(f(&mut **typed));

This doesn’t look good, but it will probably improve once we get rid of the transmute.

 Also, can you find a comparable use-case for this in the C code?

> +            }
> +        }
> +
> +        // `mutex` isn't locked in this `ExecContext`.
> +        Err(EINVAL)
> +    }
> +
> +    /// Releases all currently held locks in this context.
> +    ///
> +    /// It is intended to be used for internal implementation only.
> +    fn release_all_locks(&mut self) {
> +        self.taken.clear();
> +    }
> +
> +    /// Resets this context after a deadlock detection.
> +    ///
> +    /// Drops all held locks and reinitializes the [`WwAcquireCtx`].
> +    ///
> +    /// It is intended to be used for internal implementation only.

^ This last line can go away as this is private.

> +    fn cleanup_on_deadlock(&mut self) -> Result {
> +        self.release_all_locks();
> +        // Re-init fresh `WwAcquireCtx`.
> +        self.acquire = KBox::pin_init(WwAcquireCtx::new(self.class), GFP_KERNEL)?;

This will require one allocation per rollback.

> +
> +        Ok(())
> +    }
> +}
> --
> 2.50.0
> 

Powered by blists - more mailing lists