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

[Boqun Feng <boqun.feng@gmail.com>]

On Wed, Aug 14, 2024 at 08:44:15PM +0000, Benno Lossin wrote:
> 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).
> 

I was trying to make sure that user shouldn't mess up with interrupt
state in the callback function, but as you mention below, type system
cannot help here.

> >> 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.
> 

You're right, we then probably need a doc part of the function saying
the `cb` cannot return with interrupt enabled.

Regards,
Boqun

> ---
> Cheers,
> Benno
> 
>