Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IrqDisabled variant which allows for re-enabling interrupts #1115

Open
Lyude opened this issue Sep 11, 2024 · 1 comment
Open

IrqDisabled variant which allows for re-enabling interrupts #1115

Lyude opened this issue Sep 11, 2024 · 1 comment
Labels
• lib Related to the `rust/` library.

Comments

@Lyude
Copy link

Lyude commented Sep 11, 2024

This is a follow-up of !998 now that we have a better idea of what a design for enabling and disabling IRQs will look like. Currently, the most recent version of the patch series we have for interrupt management is here:

https://lkml.org/lkml/2024/8/1/1454

During the last re-spin of this series though, it was pointed out that there was a situation which I entirely missed. with_irqs_disabled() works great for simple situations where you just want to turn IRQs off for the duration of a callback, and then turn them back on afterwards. However, there's more complicated scenarios that can occur in the kernel. One such example that Benno provided:

local_irq_save(flags);
while (todo) {
	todo = do_sth();

	if (too_long) {
		local_irq_restore(flags);
		if (!irqs_disabled())
			sleep();
		local_irq_save(flags);
	}
}
local_irq_restore(flags);

After quite a long discussion in https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/Spinlocks.20with.20IRQs.3F we were able to come up with a pretty decent idea for a solution to this that still allows the more simple with_irqs_disabled in its current form to remain safe code.

Basically: once we start wanting to handle more complicated scenarios, we would:

  • Introduce an OwnedIrqDisabled type which can hand out IrqDisabled<'a> tokens
  • Rename with_irqs_disabled to with_irqs_disabled_always
  • Introduce a new unsafe with_irqs_disabled function that would provide access to an OwnedIrqDisabled. This would come with a rather complicated safety contract.

The safety contract would basically be something like this (I've just copied the summary I typed up already. Note, the examples are not complete!):

/// Run the closure `cb` with interrupts disabled (possibly temporarily) on the local CPU.
///
/// This creates an [`OwnedIrqDisabled`] token and passes it to the closure by-value. Generally, this
/// function should only be used in situations where it is possible that interrupts may be
/// temporarily re-enabled within the closure, as it requires significantly more care than
/// [`with_irqs_disabled_always`] to be used properly.
///
/// Generally, situations in which using this function is the right thing to do will usually involve
/// doing some work with interrupts disabled, re-enabling interrupts and then sleeping until a
/// condition is fulfilled, and then re-disabling interrupts afterwards.
///
/// # Panics
///
/// In debug builds, this function will assert that `irq` matches the current state of interrupts at
/// the beginning of function. This means if interrupts were disabled when the function was called,
/// `irq` should be `Some(OwnedIrqDisabled)`, otherwise it should be `None`. This function will restore
/// the interrupt state to how it was at the start of the function call, and return a corresponding
/// `Option<OwnedIrqDisabled>` token that may be used for further operations that require interrupts
/// disabled (if there are any).
///
/// # Safety
///
/// * The caller must ensure that if interrupts are re-enabled, the [`OwnedIrqDisabled`] token goes out
///   of scope for the duration in which interrupts are enabled.
/// * Any function which calls this function *must* have an argument of the type
///   `Option<OwnedIrqDisabled>` in its signature. This argument must reflect the state of interrupts at
///   the start of the function call.
/// * The `irq` argument of this function must always accurately reflect the state of interrupts
///   being enabled at the time this function is called. If interrupts were enabled, `irq` should be
///   `None`. Otherwise, it should be `Some(OwnedIrqDisabled)`. The argument required by the previous
///   safety requirement or the returned result from this function should be used for this.
/// * The interrupt state at the end of the closure must match the interrupt state at the beginning
///   of the function call.
/// * Additionally, a token accurately representing the state of interrupts at the end of the
///   closure *must* be returned from any function calling this as a `Option<OwnedIrqDisabled>`.
/// * Finally, because the safety requirements of this function must be upheld by the caller and
///   cannot yet be enforced by the compiler - any function calling this function must be `unsafe`
///   and have a safety contract that requires the caller ensure that the `irq` input argument
///   reflects the state of interrupts being enabled at the beginning of the function call.
///
/// # Examples
///
/// Using [`with_irqs_disabled`] to call a function that disables interrupts, temporarily re-enables
/// interrupts, and then returns with interrupts disabled.
///
/// ```
/// use bindings;
/// use kernel::irq::{OwnedIrqDisabled, with_irqs_disabled_always};
///
/// /// Does some work with interrupts explicitly enabled, and returns with them in their original
/// /// state
/// unsafe fn enable_interrupts_work(irq: Option<OwnedIrqDisabled>) -> Option<IrqDisabled> {
/// }
///
/// /// Does something incredible with interrupts disabled, enables them, then disables them again
/// ///
/// /// A new [`OwnedIrqDisabled`] token will be returned from this function if interrupts were disabled
/// /// when this function was called.
/// ///
/// /// # Safety
/// ///
/// /// * The caller must ensure that `irq` reflects the current state of interrupts at the time
/// ///   this function is called.
/// unsafe fn interrupt_me_sometimes(irq: Option<OwnedIrqDisabled>) -> Option<IrqDisabled> {
///   
/// }
/// ```

Currently this isn't part of the main series for the irq module, as I don't think we really have any users that need this quite yet.

@y86-dev
Copy link
Member

y86-dev commented Sep 13, 2024

An orthogonal thing that makes sense is to make the Backend::lock function take the Context type. Then the SpinlockIrq would be able to use IrqDisabled<'a> as the GuardState (it would also need a lifetime). And both lock and unlock would have proof that IRQs are disabled.
There is a slight problem with this idea: Guard::do_unlocked. It uses Backend::relock, which just calls Backend::lock, but it obviously doesn't have access to Context. So we need to find a way around that.
This also shows that do_unlocked probably shouldn't be used with SpinlockIrq, since the supplied closure isn't allowed to eg sleep (like how Condvar currently does), since IRQs are still disabled.

@ojeda ojeda added the • lib Related to the `rust/` library. label Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
• lib Related to the `rust/` library.
Development

No branches or pull requests

3 participants