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

[Daniel Almeida <daniel.almeida@collabora.com>]

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.


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