Message-ID: <20250906141310.2c29aa8e@nimda.home>
Date: Sat, 6 Sep 2025 14:13:10 +0300
From: Onur <work@...rozkan.dev>
To: Daniel Almeida <daniel.almeida@...labora.com>
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

On Fri, 5 Sep 2025 16:42:09 -0300
Daniel Almeida <daniel.almeida@...labora.com> wrote:

> 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()?
>

I was thinking more of a type-level mechanism to do ensure that.

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

So it can run different types inside the same execution context (see
test_with_different_input_type). If there isn't a use-case for this, I
can change it into `T`.

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

Sure, I will do that in the following patch.

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

I think there is no use case in C code that can be compared to what I
was aiming for (the multi-type support in single context). I thought it
was cool thing to have but I am not sure if it's really needed. :)

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

Good point, will re-work on that too.

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

Powered by blists - more mailing lists