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

[Wedson Almeida Filho]

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();

guard.do_unlocked(|| {
    local_irq_enable();
    schedule();
});

drop(guard);
```

What would you expect the state to be? It's meant to be the state
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
> >