Message-ID: <2b139d06-c0e0-4896-8747-d62499aec82f@proton.me>
Date: Thu, 15 Aug 2024 06:40:28 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Boqun Feng <boqun.feng@...il.com>
Cc: Lyude Paul <lyude@...hat.com>, rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org, Danilo Krummrich <dakr@...hat.com>, airlied@...hat.com, Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>, Waiman Long <longman@...hat.com>, Peter Zijlstra <peterz@...radead.org>, Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Wedson Almeida Filho <wedsonaf@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...sung.com>, Alice Ryhl <aliceryhl@...gle.com>, FUJITA Tomonori <fujita.tomonori@...il.com>, Aakash Sen Sharma <aakashsensharma@...il.com>, Valentin Obst <kernel@...entinobst.de>, Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v3 1/3] rust: Introduce irq module

On 15.08.24 06:53, Boqun Feng wrote:
> 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?

Yes, but that doesn't do anything. If the token is `Copy`, then we are
not allowed to have the following API:

    fn enable_irq(irq: IrqDisabled<'_>);

Since if the token is `Copy`, you can just copy it, call the function
and still return an `IrqDisabled<'a>` to satisfy the closure. It only
adds verbosity IMO.

> Or you're saying there could exist an `IrqDisabled<'a>` but the
> interrupts are enabled?

No.

---
Cheers,
Benno

Powered by blists - more mailing lists