Re: [PATCH v4] rust: lock: Export Guard::do_unlocked()

[Paolo Bonzini <pbonzini@redhat.com>]

On 10/29/25 19:35, Lyude Paul wrote:
> +    /// // Since we hold work.lock, which work will also try to acquire in WorkItem::run. Dropping
> +    /// // the lock temporarily while we wait for completion works around this.
> +    /// g.do_unlocked(|| work.done.wait_for_completion());
> +    ///
> +    /// assert_eq!(*g, 42);
> +    /// ```
> +    pub fn do_unlocked<U>(&mut self, cb: impl FnOnce() -> U) -> U {
>           // SAFETY: The caller owns the lock, so it is safe to unlock it.
>           unsafe { B::unlock(self.lock.state.get(), &self.state) };

Getting self as &mut is incorrect.  That's because owning a lock guard 
implicitly tells you that no other thread can observe the intermediate 
states of the object.  (The same is even more obviously true for a 
RefCell's mutable borrow, i.e. core::cell::RefMut)

Let's say you have a lock-protected data structure with an invariant 
that is preserved at the end of every critical section.  Let's say also 
that you have a function

     fn do_something() {
         let g = self.inner.lock();
         g.mess_up_the_invariant();          // (1)
         self.do_something_else(&mut g);     // uses do_unlocked()
         g.fix_the_invariant();              // (2)
     }

Because the function holds a guard between the calls (1) and (2), it 
expects that other thread cannot observe the temporary state.  The fact 
that do_unlocked() takes a &mut doesn't help, because the common case 
for RAII objects is that they're passed around mutably.

Instead, do_unlocked should take the guard and return another one:

     fn do_something() {
         let mut g = self.inner.lock();
         g.mess_up_the_invariant();          // (1)
         g = self.do_something_else(g);      // uses do_unlocked()
         g.fix_the_invariant();              // (2)
     }

This version of the interface makes it clear that (1) and (2) are in a 
separate critical section.  Unfortunately it makes the signature uglier 
for do_unlocked() itself:

     #[must_use]
     pub fn do_unlocked<U>(self, cb: impl FnOnce() -> U) -> (Self, U)

Paolo