Messages in this thread | | | From | Wedson Almeida Filho <> | Date | Wed, 12 Apr 2023 08:07:40 -0300 | Subject | Re: [PATCH v4 11/13] rust: lock: add `Guard::do_unlocked` |
| |
On Wed, 12 Apr 2023 at 03:25, Boqun Feng <boqun.feng@gmail.com> 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 > >
| |