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

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

On 14.08.24 22:17, Boqun Feng wrote:
> On Wed, Aug 14, 2024 at 03:38:47PM -0400, Lyude Paul wrote:
>> On Wed, 2024-08-14 at 10:35 -0700, Boqun Feng wrote:
>>> On Thu, Aug 01, 2024 at 08:10:00PM -0400, Lyude Paul wrote:
>>> [...]
>>>> +/// Run the closure `cb` with interrupts disabled on the local CPU.
>>>> +///
>>>> +/// This creates an [`IrqDisabled`] token, which can be passed to functions that must be run
>>>> +/// without interrupts.
>>>> +///
>>>> +/// # Examples
>>>> +///
>>>> +/// Using [`with_irqs_disabled`] to call a function that can only be called with interrupts
>>>> +/// disabled:
>>>> +///
>>>> +/// ```
>>>> +/// use kernel::irq::{IrqDisabled, with_irqs_disabled};
>>>> +///
>>>> +/// // Requiring interrupts be disabled to call a function
>>>> +/// fn dont_interrupt_me(_irq: IrqDisabled<'_>) {
>>>> +///     /* When this token is available, IRQs are known to be disabled. Actions that rely on this
>>>> +///      * can be safely performed
>>>> +///      */
>>>> +/// }
>>>> +///
>>>> +/// // Disabling interrupts. They'll be re-enabled once this closure completes.
>>>> +/// with_irqs_disabled(|irq| dont_interrupt_me(irq));
>>>> +/// ```
>>>> +#[inline]
>>>> +pub fn with_irqs_disabled<T>(cb: impl for<'a> FnOnce(IrqDisabled<'a>) -> T) -> T {
>>>
>>> Given the current signature, can `cb` return with interrupts enabled (if
>>> it re-enables interrupt itself)? For example:
>>>
>>> 	with_irqs_disabled(|irq_disabled| {
>>>
>>> 	    // maybe a unsafe function.
>>> 	    reenable_irq(irq_disabled);
>>
>> JFYI: this wouldn't be unsafe, it would be broken code in all circumstances
>> Simply put: `with_irqs_disabled()` does not provide the guarantee that
>> interrupts were enabled previously, only that they're disabled now. And it is
>> never a sound operation in C or Rust to ever enable interrupts without a
>> matching disable in the same scope because that immediately risks a deadlock
>> or other undefined behavior. There's no usecase for this, I'd consider any
>> kind of function that returns with a different interrupt state then it had
>> upon being called to simply be broken.
>>
>> Also - like we previously mentioned, `IrqDisabled` is just a marker type. It
>> doesn't enable or disable anything itself, the most it does is run a debug
> 
> Yes, I know, but my question is more that should `cb` return a
> `IrqDisabled` to prove the interrupt is still in the disabled state?
> I.e. no matter what `cb` does, the interrupt remains disabled.

What does this help with? I don't think this will add value (at least
with how `IrqDisabled` is designed at the moment).

>> assertion to ensure interrupts are disabled upon creation. So dropping it
>> doesn't change interrupt state. I think this actually does make sense
>> semantically: even if IrqDisabled wasn't a no-op in a world where we could
> 
> Just to be clear, I'm not suggesting making IrqDisable not a no-op.
> 
>> somehow implement that without running into the drop order issue - there still
>> would not be a guarantee that dropping `IrqDisabled` would enable interrupts
>> simply because it could be a nested disable. And there's no way we could make
>> interrupt enabled sections explicit without either klint, or carrying around a
>> `IrqEnabled` (which we would have to do for every function that could sleep,
>> so I don't think that's ideal). So without a token like this all code can do
>> is assume it doesn't know the interrupt state, and rely on solutions like
>> lockdep to complain if code within an interrupt context tries to perform an
>> operation that would be unsound there like sleeping.
>>
>> This being said - I would be totally alright with us making it so that we
>> assert that interrupts are still disabled upon dropping the token. But

We can't implement `Drop`, since it already implements `Copy`. But we
could add a debug assert before we call `local_irq_restore`. I think
it's a good idea to add a debug assert.

>> interrupts have to disabled throughout the entire closure regardless of the
>> presence of IrqDisabled. The same rules apply to C code using
>> local_irq_save()/local_irq_restore() - between those two function calls, it is
>> always a bug to re-enable interrupts even if they get turned back off. Unsafe
> 
> Do you mean the particular local_irq_save() and local_irq_restore(), or
> do you mean any interrupt disable critical sections? Note that we have
> wait_event_interruptible_locked_irq() which does exactly re-enabling
> interrupt in the middle to sleep and I'm pretty sure we have other cases
> where interrupts are re-enabled. So I'm not sure when you say "the same
> rules apply to C code ..."
> 
>> functions are no exceptions, nor are C bindings, and should simply be
>> considered broken (not unsafe) if they violate this. I suppose that's
>> something else we could document if people think it's necessary.
>>
>>
>>> 	})
>>>
>>> note that `cb` is a `-> T` function, other than `-> (IrqDisabled<'a>,
>>> T)`, so semantically, it doesn't require IRQ still disabled after
>>> return.
>>
>> This was the reason I originally had us pass IrqDisabled as a reference and
>> not a value - specifically since it seemed to make more sense to treat
>> IrqDisabled as an object which exists throughout the lifetime of the closure
>> regardless of whether we drop our reference to it or not - since it's a no-op.
>>
> 
> I haven't found a problem with `&IrqDisabled` as the closure parameter,
> but I may miss something.

We could also use `&'a IrqDisabled` instead of `IrqDisabled<'a>` (note
the first one doesn't have a lifetime). But there is no behavioral
difference between the two. Originally the intended API was to use `&'a
IrqDisabled<'a>` as the closure parameter and `IrqDisabled<'a>` in
functions that require irqs being disabled. As long as we decide on a
consistent type, I don't mind either (since then we can avoid
reborrowing).

> So the key ask from me is: it looks like we are on the same page that
> when `cb` returns, the IRQ should be in the same disabled state as when
> it gets called. So how do we express this "requirement" then? Type
> sytem, comments, safety comments?

I don't think that expressing this in the type system makes sense, since
the type that we select (`&'a IrqDisabled` or `IrqDisabled<'a>`) will be
`Copy`. And thus you can just produce as many of those as you want.

---
Cheers,
Benno