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

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

On Wed, Aug 14, 2024 at 01:57:55PM -0700, Boqun Feng wrote:
> 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.
> 
[...]
> > > 
> > > 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.
> > 

Hmm.. on a second thought, `Copy` doesn't affect what I'm proposing
here, yes one could have as many `IrqDisabled<'a>` as one wants, but
making `cb` returns a `(IrqDisabled<'a>, T)` means the `cb` has to prove
at least one of the `IrqDisabled<'a>` exists, i.e. it must prove the irq
is still disabled, which the requirement of `with_irqs_disabled`, right?
Or you're saying there could exist an `IrqDisabled<'a>` but the
interrupts are enabled?

Regards,
Boqun

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