On Wed, Aug 14, 2024 at 08:44:15PM +0000, Benno Lossin wrote:

On 14.08.24 22:17, Boqun Feng wrote:

quoted

On Wed, Aug 14, 2024 at 03:38:47PM -0400, Lyude Paul wrote:

quoted

On Wed, 2024-08-14 at 10:35 -0700, Boqun Feng wrote:

quoted

On Thu, Aug 01, 2024 at 08:10:00PM -0400, Lyude Paul wrote:
[...]

quoted

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

quoted

quoted

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.

quoted

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.

quoted

quoted

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

quoted

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.

quoted

	})

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

quoted

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