Re: [PATCH v7 5/6] rust: ww_mutex: implement LockSet

["Onur Özkan" <work@onurozkan.dev>]

On Tue, 25 Nov 2025 16:35:21 -0500
Lyude Paul <lyude@redhat.com> wrote:

> On Mon, 2025-11-24 at 18:49 +0300, Onur Özkan wrote:
> > > 
> > > I wonder if there's some way we can get rid of the safety contract
> > > here and verify this at compile time, it would be a shame if every
> > > single lock invocation needed to be unsafe.
> > > 
> > 
> > Yeah :(. We could get rid of them easily by keeping the class that
> > was passed to the constructor functions but that becomes a problem
> > for the from_raw implementations.
> > 
> > I think the best solution would be to expose ww_class type from
> > ww_acquire_ctx and ww_mutex unconditionally (right now it depends on
> > DEBUG_WW_MUTEXES). That way we can just access the class and verify
> > that the mutex and acquire_ctx classes match.
> > 
> > What do you think? I can submit a patch for the C-side
> > implementation. It should be straightforward and shouldn't have any
> > runtime impact.
> 
> I would be fine with this, and think this is definitely the right way
> to go
> 

It would be great to reach a consensus on this (whether we should send a
patch to the C side or instead pass the Class to the from_raw functions
without modifying the C code).

-Onur 

> > 
> > > > +    ///
> > > > +    ///         Ok(())
> > > > +    ///     },
> > > > +    ///     // `on_all_locks_taken` closure
> > > > +    ///     |lock_set| {
> > > > +    ///         // Safely mutate both values while holding the
> > > > locks.
> > > > +    ///         lock_set.with_locked(&mutex1, |v| *v += 1)?;
> > > > +    ///         lock_set.with_locked(&mutex2, |v| *v += 1)?;
> > > > +    ///
> > > > +    ///         Ok(())
> > > > +    ///     },
> > > 
> > > I'm still pretty confident we don't need or want both closures and
> > > can combine them into a single closure. And I am still pretty sure
> > > the only thing that needs to be tracked here is which lock we
> > > failed to acquire in the event of a deadlock.
> > > 
> > > Let me see if I can do a better job of explaining why. Or, if I'm
> > > actually wrong about this - maybe this will help you correct me
> > > and see where I've misunderstood something :).
> > > 
> > > First, let's pretend we've made a couple of changes here:
> > > 
> > >   * We remove `taken: KVec<RawGuard>` and replace it with
> > > `failed: *mut Mutex<…>`
> > >   * lock_set.lock():
> > >      - Now returns a `Guard` that executes `ww_mutex_unlock` in
> > > its destructor
> > >      - If `ww_mutex_lock` fails due to -EDEADLK, this function
> > > stores a pointer to the respective mutex in `lock_set.failed`.
> > >      - Before acquiring a lock, we now check:
> > >         + if lock_set.failed == lock
> > >            * Return a Guard for lock without calling
> > > ww_mutex_lock()
> > >            * lock_set.failed = null_mut();
> > >   * We remove `on_all_locks_taken()`, and rename
> > > `locking_algorithm` to `ww_cb`.
> > >   * If `ww_cb()` returns Err(EDEADLK):
> > >      - if !lock_set.failed.is_null()
> > >         + ww_mutex_lock(lock_set.failed) // Don't store a guard
> > >   * If `ww_cb()` returns Ok(…):
> > >      - if !lock_set.failed.is_null()
> > >        // This could only happen if we hit -EDEADLK but then
> > > `ww_cb` did not // re-acquire `lock_set.failed` on the next
> > > attempt
> > >         + ww_mutex_unlock(lock_set.failed)
> > > 
> > > With all of those changes, we can rewrite `ww_cb` to look like
> > > this:
> > > 
> > > > lock_set| {
> > > // SAFETY: Both `lock_set` and `mutex1` uses the same class.
> > > let g1 = unsafe { lock_set.lock(&mutex1)? };
> > > 
> > > // SAFETY: Both `lock_set` and `mutex2` uses the same class.
> > > let g2 = unsafe { lock_set.lock(&mutex2)? };
> > > 
> > > *g1 += 1;
> > > *g2 += 2;
> > > 
> > > Ok(())
> > > }
> > > 
> > > If we hit -EDEADLK when trying to acquire g2, this is more or less
> > > what would happen:
> > > 
> > >   * let res = ww_cb():
> > >      - let g1 = …; // (we acquire g1 successfully)
> > >      - let g2 = …; // (enter .lock())
> > >         + res = ww_mutex_lock(mutex2);
> > >         + if (res) == EDEADLK
> > >            * lock_set.failed = mutex2;
> > >         + return Err(EDEADLK);
> > >      - return Err(-EDEADLK);
> > >        // Exiting ww_cb(), so rust will drop all variables in this
> > > scope:
> > >         + ww_mutex_unlock(mutex1) // g1's Drop
> > > 
> > >   * // (res == Err(EDEADLK))
> > >     // All locks have been released at this point
> > > 
> > >   * if !lock_set.failed.is_null()
> > >      - ww_mutex_lock(lock_set.failed) // Don't create a guard
> > >     // We've now re-acquired the lock we dead-locked on
> > > 
> > >   * let res = ww_cb():
> > >      - let g1 = …; // (we acquire g1 successfully)
> > >      - let g2 = …; // (enter .lock())
> > >         + if lock_set.failed == lock
> > >            * lock_set.failed = null_mut();
> > >            * return Guard(…); // but don't call ww_mutex_lock(),
> > > it's already locked
> > >      - // We acquired g2 successfully!
> > >      - *g1 += 1;
> > >      - *g2 += 2;
> > > 
> > >   * etc…
> > > 
> > > The only challenge with this is that users need to write their
> > > ww_cb() implementations to be idempotent (so that calling it
> > > multiple times isn't unexpected). But that's already what we do
> > > on the C side, and is kind of what I expected we would want to do
> > > in rust anyhow.
> > > 
> > > Does this make sense, or was there something I made a mistake with
> > > here?
> > 
> > Thanks a lot for analyzing and providing an alternative on this!
> > 
> > However, collapsing everything into a single callback would require
> > the caller to self-police various disciplines like "don't touch gN
> > until gN+1 succeeded", which is exactly the foot-gun we are trying
> > avoid with 2 closures.
> > 
> > Separating acquire and use logics not just simpler API to implement
> > (and provide), but also more effective compare to your example
> > here. With single closure we basically move API responsibility to
> > the users (e.g., do not run this part of the code in the loop, do
> > not access to any data behind any guard if all the locks aren't
> > taken yet, etc.), which is not a good thing to do, especially from
> > the high-level API.
> 
> !!!!!
> OK - now I finally understand what I was missing, it totally slipped
> my mind that we would have this requirement. One thing I'm not sure
> this takes into account though: what about a situation where you
> can't actually know you need to acquire gN+1 until you've acquired gN
> and looked at it? This is at least a fairly common pattern with KMS,
> I'm not sure if it comes up with other parts of the kernel using ww
> mutexes.
>