Re: [PATCH v2 3/3] rust: sync: Add SpinLockIrq

[Benno Lossin <benno.lossin@proton.me>]

On 01.08.24 19:10, Lyude Paul wrote:
> On Thu, 2024-08-01 at 10:29 +0000, Benno Lossin wrote:
>> On 01.08.24 00:35, Lyude Paul wrote:
>>> +unsafe impl super::Backend for SpinLockIrqBackend {
>>> +    type State = bindings::spinlock_t;
>>> +    type GuardState = ();
>>> +    type Context<'a> = IrqDisabled<'a>;
>>> +
>>> +    unsafe fn init(
>>> +        ptr: *mut Self::State,
>>> +        name: *const core::ffi::c_char,
>>> +        key: *mut bindings::lock_class_key,
>>> +    ) {
>>> +        // SAFETY: The safety requirements ensure that `ptr` is valid for writes, and `name` and
>>> +        // `key` are valid for read indefinitely.
>>> +        unsafe { bindings::__spin_lock_init(ptr, name, key) }
>>> +    }
>>> +
>>> +    unsafe fn lock(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.
>>> +        unsafe { bindings::spin_lock(ptr) }
>>
>> Should this really be the same function as `SpinLock`? (ie is there not
>> a special version that expects IRQ to already be disabled? eg this could
>> add additional debug calls)
> 
> Yes, unless there's some spinlock API function I missed (I checked right
> before sending this response) we don't really have a variant of spin_lock*()
> that assumes IRQs are disabled. You really just have:
> 
> spin_lock() - Grab lock, no IRQ changes
> 
> spin_lock_irq() - Grab lock, unconditionally disable IRQs (regardless of
> current flags) until spin_unlock_irq()
> 
> spin_lock_irqsave() - Grab lock, save IRQ flags and restore upon
> spin_unlock_irqrestore()

I see, I have very limited knowledge of the C side and using the same
function for both seemed strange.

> Usually lockdep is the one to actually warn about the interrupt state being
> incorrect, as it will throw up a warning if you grab a spinlock in both an
> interrupt enabled and disabled context (which means you are forgetting to
> disable interrupts before lock acquisition somewhere).

Cool.

> As well, I think having further debug calls would be unneeded? As of right now
> there's only really two entrypoints for getting IrqDisabled:
> with_irqs_disabled(), and IrqDisabled::new(). And since IrqDisabled::new() now
> has a debug assertion for disabled interrupts, that means all paths to
> creating IrqDisabled are either already guaranteed to disable interrupts - or
> would be making use of the debug assertion for verifying interrupt state.

Yes, but if you eg call some FFI function in the meantime it might
re-enable IRQs (or just a plain old bug in the Rust side). I don't know
how expensive this will be for debug builds, if it is too much, we could
try to introduce different levels of debugging.
But as you already said above, we don't need it for `SpinLockIrq`, since
lockdep should take care of that.

---
Cheers,
Benno