Messages in this thread | | | Date | Tue, 11 Apr 2023 23:25:19 -0700 | From | Boqun Feng <> | Subject | Re: [PATCH v4 11/13] rust: lock: add `Guard::do_unlocked` |
| |
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"
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?
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 >
| |