Message-Id: <2B16DBF4-1F6C-4025-8373-5651867B7D49@collabora.com>
Date: Sat, 6 Sep 2025 12:04:34 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Onur <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



> On 6 Sep 2025, at 08:13, Onur <work@...rozkan.dev> wrote:
> 
> 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.

Why?

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

That’s my point, you can have “different T” using T = KBox<dyn
SomeTrait> if you really want it. Of course that is not free, but it’s also
optional.

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

I’m not sure we need this, but it can always come later.

Powered by blists - more mailing lists