Re: [PATCH 1/3] rust: Introduce irq module

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

On 26.07.24 20:18, Lyude Paul wrote:
> On Fri, 2024-07-26 at 07:23 +0000, Benno Lossin wrote:
>> On 26.07.24 00:27, Lyude Paul wrote:
>>> +}
>>> +
>>> +/// Run the closure `cb` with interrupts disabled on the local CPU.
>>> +///
>>> +/// Interrupts will be re-enabled once the closure returns. If interrupts were already disabled on
>>> +/// this CPU, this is a no-op.
>>> +#[inline]
>>> +pub fn with_irqs_disabled<T, F>(cb: F) -> T
>>> +where
>>> +    F: FnOnce(IrqDisabled<'_>) -> T,
>>> +{
>>> +    // SAFETY: FFI call with no special requirements
>>
>> I vaguely remember that there were some problems with sleeping in IRQ
>> disabled contexts, is that me just misremembering (eg confusing it with
>> atomic context), or do we need to watch out for that?
> 
> You're correct - sleeping isn't allowed in no-irq contexts.
> 
>> Because if that is the case, then we would need to use klint.
> 
> Ok - I've never used klint before but I'm happy to look into it and try to
> implement something for it.

I am not 100% up to date, last time I heard Gary (the maintainer/author
of klint) talk about it, I remember that it wasn't fully ready for this
stuff. Don't know if this has changed in the meantime.
I don't think this is anything that you can do much, since it's rather
complex, so I will just ping Gary on the status.

> FWIW too: I assume we would still want klint anyway, but I think it's at least
> worth mentioning the kernel does have a compile option for WARNing on sleeps
> in sleepless contexts

So the plan always was to not make it a hard error. Essentially instead
of having `unsafe` littered al over the place when you switch context,
klint would ensure (like the borrow checker for ownership) that
everything is fine.

>>> +    let flags = unsafe { bindings::local_irq_save() };
>>> +
>>> +    let ret = cb(IrqDisabled(PhantomData));
>>> +
>>> +    // SAFETY: `flags` comes from our previous call to local_irq_save
>>> +    unsafe { bindings::local_irq_restore(flags) };
>>
>> Just to make sure, this function only enables interrupts, if they were
>> enabled before the call to `local_irq_save` above, right?
> 
> Correct - `local_irq_save()` only saves the CPU's current IRQ flags. So if
> interrupts were already disabled in the context we call `local_irq_save()`, we
> end up restoring the same IRQ-disabled flags on the processor when we get to
> `local_irq_restore()`. This is also why the closure interface for this is
> necessary - to ensure that nested interrupt flag saves are always undone in
> the reverse order to ensure this assumption always holds.

Thanks, that was exactly what I assumed.

---
Cheers,
Benno