Re: [PATCH v10 6/7] rust: ww_mutex: implement LockSet

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

On Wed, 30 Sep 2026 19:10:16 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:

> Hi Onur,
> 
> The bot has caught it already, but remember to compile with CLIPPY=1 for
> v11.
> 
> If push() fails, two things drop:
> 
> > +    /// Attempts to lock the given [`Mutex`] and stores a guard for it.
> > +    pub fn lock<T>(&mut self, mutex: &'a Mutex<'a, T>) -> Result {
> > +        let guard = self.acquire_ctx.lock(mutex)?; <--- this guard
> > +
> > +        self.taken.push(
> > +            RawGuard {
> > +                // SAFETY: We just locked it above so it's a valid pointer.
> > +                mutex_ptr: unsafe { NonNull::new_unchecked(guard.mutex.inner.get()) },
> > +                _not_send: NotThreadSafe,
> > +            }, <------- also the raw guard, which drops in push()
> > +            GFP_KERNEL,
> > +        )?;
> > +
> > +        // Avoid unlocking here; `release_all_locks` (also run by `Drop`)
> > +        // performs the unlock for `LockSet`.
> > +        core::mem::forget(guard);
> > +
> > +        Ok(())
> > +    }
> > +
> 
> Both unlock. I suggest that RawGuard consumes Guard on creation, or some
> variation of this.

We can forget the original guard after constructing RawGuard before calling
push(). I think we can keep `RawGuard` as simple as it is.

> 
> 
> When locking, I don't think it makes sense to immediately retry on
> -EDEADLK. This will busy-loop on a lock that will probably still be
> taken. We should release all locks and sleep instead.
> 
> > +    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 LockSet<'a>) -> Result,
> > +        Y: FnMut(&mut LockSet<'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; <------ sleep on the contended lock before continuing
> > +                }
> > +                Err(e) => {
> > +                    self.release_all_locks();
> > +                    return Err(e);
> > +                }
> > +            }
> > +        }
> > +    }
> > +
> 
> In order to sleep, we must record the contended lock though, which is
> something that this patch does not do yet. I suggest that we store a
> pointer to it and call from_raw() on that. Also, when acquiring the
> contended lock, we must hold it before retrying, or we will race for it
> again.
> 
> > +    fn cleanup_on_deadlock(&mut self) {
> > +        self.release_all_locks();
> > +
> > +        // SAFETY: We released all the locks just above.
> > +        unsafe { self.acquire_ctx.as_mut().reinit() }; 
> > +    }
> 
> 
> IIUC, calling reinit() will forfeit our ticket and acquire a new one, so
> LockSet breaks the whole premise of wwmutex: when there's contention,
> the older ticket eventually wins. IMHO, remove that line.
> 
> 
> -- Daniel