Re: [PATCH v4 11/13] rust: lock: add `Guard::do_unlocked`

[Wedson Almeida Filho]

On Wed, 12 Apr 2023 at 11:35, Boqun Feng <boqun.feng@xxxxxxxxx> wrote:
>
> On Wed, Apr 12, 2023 at 08:07:40AM -0300, Wedson Almeida Filho wrote:
> > On Wed, 12 Apr 2023 at 03:25, Boqun Feng <boqun.feng@xxxxxxxxx> wrote:
> > >
> > > On Tue, Apr 11, 2023 at 02:45:41AM -0300, Wedson Almeida Filho wrote:
> > > [...]
> > > > +
> > > > +    unsafe fn relock(ptr: *mut Self::State, guard_state: &mut Self::GuardState) {
> > > > +        let _ = match guard_state {
> > > > +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> > > > +            // initialised.
> > > > +            None => unsafe { Self::lock(ptr) },
> > > > +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> > > > +            // initialised.
> > > > +            Some(_) => unsafe { Self::lock_irqsave(ptr) },
> > > > +        };
> > > > +    }
> > > >  }
> > > >
> > >
> > > One thing I'm little worried about the above is that we don't store back
> > > the new GuardState into `guard_state`, the particular case I'm worried
> > > about is as follow:
> > >
> > >         // IRQ is enabled.
> > >         // Disabling IRQ
> > >         unsafe { bindings::local_irq_disable(); }
> > >
> > >         let mut g = unsafe { SpinLockBackend::lock(&mut lock as *mut _) };
> > >         // `g` records irq state is "irq disabled"
> > >
> > >         unsafe { SpinLockBackend::unlock(&mut lock as *mut _, &g); }
> > >         // restore into "irq disabled" mode.
> > >         // IRQ is disabled.
> > >
> > >         // Enabling IRQ
> > >         unsafe { bindings::local_irq_enable(); }
> > >         // IRQ is enabled.
> > >
> > >         unsafe { SpinLockBackend::relock(&mut lock as *mut _, &mut g) }
> > >         // `g` still records irq state is "irq disabled"
> >
> > Yes, that's by design. If you want it to record the new "irq enabled"
> > state, then you should call `lock()`, not `relock()`.
> >
> > >         unsafe { SpinLockBackend::unlock(&mut lock as *mut _, &g); }
> > >         // restore into "irq disabled" mode.
> > >         // IRQ is disabled.
> > >
> > >
> > > This looks pretty scary to me, I would expect `relock()` updates the
> > > latest GuardState to the guard. Any reason it's implemented this way?
> >
> > A `relock()` followed by an `unlock()` takes the state back to how it
> > was when `lock()` was originally called: this is precisely why
> > `relock()` exists.
> >
> > Consider the following case:
> >
> > ```
> > local_disable_irq();
> > let mut guard = spinlock.lock();
>
> I think you meant `spinlock.lock_irqsave()` here, right?

Yes, sorry, I meant `lock_irqsave()`.

> >
> > guard.do_unlocked(|| {
> >     local_irq_enable();
> >     schedule();
> > });
> >
> > drop(guard);
> > ```
> >
> > What would you expect the state to be? It's meant to be the state
>
> I understand your point but I would expect people to code like:
>
> ```
> local_disable_irq();
> let mut guard = spinlock.lock(); // or lock_irqsave(), doesn't matter
>
> guard.do_unlocked(|| {
>     local_irq_enable();
>     schedule();
>     local_irq_disable();
> });
>
> drop(guard);
> ```

Well, `relock` works with the code above as well.

> And the closure in do_unlocked() can also be something like:
> ```
>         guard.do_unlocked(|| {
>             local_irq_enabled();
>             let _g = ScopeGuard::new(|| {
>                 local_irq_disabled();
>             });
>
>             schedule();
>
>             if (some_cond) {
>                 return; // return early
>             }
>
>             if (other_cond) {
>                 return;
>             }
>         })
>
> ```
>
> One benefit (other that code looks symmetric) is we can use the same
> closure in other place. Also it helps klint since we keep the irq
> enablement state change as local as possible: we can go ahead and
> require irq enabled state should not be changed between the closure in
> do_unlock().

Note that the only user of `do_unlocked` at the moment works for any
type of lock, including mutexes, so we can't really have this kind of
code there. All irq handling needs to happen on the backends.

> Maybe I'm missing something, but the current `relock` semantics is
> really tricky to get ;-)

It seems straightforward to me: reacquire the lock in the same mode as
the original lock() call (e.g., lock() vs lock_irqsave()) such that
the `unlock()` will restore any state it manages to what it was right
before the original locking call.

But callers are not going to deal with these unsafe backend functions
directly, they'll deal with guards, so the high-level requirement that
`relock()` enforces is the following, given something like:

```
// 1
let guard = spinlock.lock_irqsave();
// Some code, which may include calls to a condvar, and may change the
irq state.
drop(guard);
// 2
```

At 2, the irq state must be the same as in 1, that's why one would use
the `lock_irqsave` variant.

This is a common pattern (seen in a bunch of unrelated places): save
the current state, make changes to the state, and eventually restore
the saved state (independently of what changes were made between
saving and restoring).

> Regards,
> Boqun
>
> > right before `spinlock.lock()` was called, that's what the guard
> > represents.
> >
> > If you want to preserve a new state, then you don't want `relock()`,
> > you just want a new `lock()` call.
> >
> > > Regards,
> > > Boqun
> > >
> > > >  // SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion. We use the `irqsave`
> > > >  // variant of the C lock acquisition functions to disable interrupts and retrieve the original
> > > >  // interrupt state, and the `irqrestore` variant of the lock release functions to restore the state
> > > >  // in `unlock` -- we use the guard context to determine which method was used to acquire the lock.
> > > > -unsafe impl super::IrqSaveBackend for SpinLockBackend {
> > > > +unsafe impl IrqSaveBackend for SpinLockBackend {
> > > >      unsafe fn lock_irqsave(ptr: *mut Self::State) -> Self::GuardState {
> > > >          // SAFETY: The safety requirements of this function ensure that `ptr` points to valid
> > > >          // memory, and that it has been initialised before.
> > > > --
> > > > 2.34.1
> > > >