`ExecContext` is a helper built on top of ww_mutex
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.
Signed-off-by: Onur Özkan <work@onurozkan.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();
+ }
+}
+
+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.
+ 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.
+ let erased: WwMutexGuard<'a, ()> = unsafe { core::mem::transmute(guard) };
+ 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(())
+ /// },
+ /// )?;
+ ///
+ /// # 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<()>,
+ 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);
+ }
+ }
+ }
+ }
+
+ /// 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() {
+ // 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));
+ }
+ }
+
+ // `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.
+ 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)?;
+
+ Ok(())
+ }
+}
--
2.50.0
On Wed, Sep 03, 2025 at 04:13:12PM +0300, Onur Özkan wrote: > `ExecContext` is a helper built on top of ww_mutex > 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. > > Signed-off-by: Onur Özkan <work@onurozkan.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(); > + } > +} > + > +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. > + 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. > + let erased: WwMutexGuard<'a, ()> = unsafe { core::mem::transmute(guard) }; > + 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(()) > + /// }, > + /// )?; > + /// > + /// # 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<()>, > + 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); > + } > + } > + } > + } > + > + /// 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() { > + // 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)); > + } > + } > + > + // `mutex` isn't locked in this `ExecContext`. > + Err(EINVAL) > + } This can be refactored using a more idiomatic functional style: diff --git a/rust/kernel/sync/lock/ww_mutex/exec.rs b/rust/kernel/sync/lock/ww_mutex/exec.rs index 543c5218232a..8fdfb11104b9 100644 --- a/rust/kernel/sync/lock/ww_mutex/exec.rs +++ b/rust/kernel/sync/lock/ww_mutex/exec.rs @@ -142,16 +142,17 @@ pub fn with_locked<T, Y>( 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() { + self.taken + .iter_mut() + .find_map(|guard| if mutex.as_ptr() == guard.mutex.as_ptr() { // 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)); - } - } - - // `mutex` isn't locked in this `ExecContext`. - Err(EINVAL) + Some(unsafe { &mut *ptr::from_mut(guard).cast::<WwMutexGuard<'a, T>>() }) + } else { + None + }) + .map(|typed| f(&mut **typed)) + // `mutex` isn't locked in this `ExecContext`. + .ok_or(EINVAL) } /// Releases all currently held locks in this context. > + > + /// 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. > + 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)?; > + > + Ok(()) > + } > +} > -- > 2.50.0 > > Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
On Fri, 5 Sep 2025 23:11:01 +0000 Elle Rhumsaa <elle@weathered-steel.dev> wrote: > On Wed, Sep 03, 2025 at 04:13:12PM +0300, Onur Özkan wrote: > > `ExecContext` is a helper built on top of ww_mutex > > 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. > > > > Signed-off-by: Onur Özkan <work@onurozkan.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(); > > + } > > +} > > + > > +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. > > + 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. > > + let erased: WwMutexGuard<'a, ()> = unsafe { > > core::mem::transmute(guard) }; > > + 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(()) > > + /// }, > > + /// )?; > > + /// > > + /// # 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<()>, > > + 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); > > + } > > + } > > + } > > + } > > + > > + /// 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() { > > + // 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)); > > + } > > + } > > + > > + // `mutex` isn't locked in this `ExecContext`. > > + Err(EINVAL) > > + } > > This can be refactored using a more idiomatic functional style: > > diff --git a/rust/kernel/sync/lock/ww_mutex/exec.rs > b/rust/kernel/sync/lock/ww_mutex/exec.rs index > 543c5218232a..8fdfb11104b9 100644 --- > a/rust/kernel/sync/lock/ww_mutex/exec.rs +++ > b/rust/kernel/sync/lock/ww_mutex/exec.rs @@ -142,16 +142,17 @@ pub fn > with_locked<T, Y>( 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() { > + self.taken > + .iter_mut() > + .find_map(|guard| if mutex.as_ptr() == > guard.mutex.as_ptr() { // 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)); > - } > - } > - > - // `mutex` isn't locked in this `ExecContext`. > - Err(EINVAL) > + Some(unsafe { &mut > *ptr::from_mut(guard).cast::<WwMutexGuard<'a, T>>() }) > + } else { > + None > + }) > + .map(|typed| f(&mut **typed)) > + // `mutex` isn't locked in this `ExecContext`. > + .ok_or(EINVAL) > } > Looks nice, I will apply it if the flow doesn't change in the next patch. Thanks! -Onur > /// Releases all currently held locks in this context. > > > + > > + /// 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. > > + 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)?; + > > + Ok(()) > > + } > > +} > > -- > > 2.50.0 > > > > > > Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
Hi Onur, > On 3 Sep 2025, at 10:13, Onur Özkan <work@onurozkan.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@onurozkan.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 >
On Fri, 5 Sep 2025 16:42:09 -0300 Daniel Almeida <daniel.almeida@collabora.com> wrote: > Hi Onur, > > > On 3 Sep 2025, at 10:13, Onur Özkan <work@onurozkan.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@onurozkan.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 > > >
> On 6 Sep 2025, at 08:13, Onur <work@onurozkan.dev> wrote: > > On Fri, 5 Sep 2025 16:42:09 -0300 > Daniel Almeida <daniel.almeida@collabora.com> wrote: > >> Hi Onur, >> >>> On 3 Sep 2025, at 10:13, Onur Özkan <work@onurozkan.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@onurozkan.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.
On Sat, 6 Sep 2025 12:04:34 -0300 Daniel Almeida <daniel.almeida@collabora.com> wrote: > > > > On 6 Sep 2025, at 08:13, Onur <work@onurozkan.dev> wrote: > > > > On Fri, 5 Sep 2025 16:42:09 -0300 > > Daniel Almeida <daniel.almeida@collabora.com> wrote: > > > >> Hi Onur, > >> > >>> On 3 Sep 2025, at 10:13, Onur Özkan <work@onurozkan.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@onurozkan.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? > So that wait-wound and wait-die classes don't get mixed up in the same `ExecContext` by using type validation at compile time. Of course, `core::ptr::eq()` is still useful/required when the classes are of the same type but not exactly the same value. Maybe we can do both. Thanks, Onur
On Sun, 7 Sep 2025 11:20:06 +0300 Onur <work@onurozkan.dev> wrote: > On Sat, 6 Sep 2025 12:04:34 -0300 > Daniel Almeida <daniel.almeida@collabora.com> wrote: > > > > > > > > On 6 Sep 2025, at 08:13, Onur <work@onurozkan.dev> wrote: > > > > > > On Fri, 5 Sep 2025 16:42:09 -0300 > > > Daniel Almeida <daniel.almeida@collabora.com> wrote: > > > > > >> Hi Onur, > > >> > > >>> On 3 Sep 2025, at 10:13, Onur Özkan <work@onurozkan.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@onurozkan.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? > > > > So that wait-wound and wait-die classes don't get mixed up in the > same `ExecContext` by using type validation at compile time. > > Of course, `core::ptr::eq()` is still useful/required when the classes > are of the same type but not exactly the same value. Maybe we can do > both. > > > Thanks, > Onur I will also look into whether it's possible to remove the class from the mutex and instead derive it from ExecContext and WwAcquireCtx. This would fix both issues at once in a better way. -Onur
© 2016 - 2025 Red Hat, Inc.