Rust-for-Linux / linux

Adding support for the Rust language to the Linux kernel.
https://rust-for-linux.com
Other
3.99k stars 431 forks source link

IrqDisabled variant which allows for re-enabling interrupts #1115

Open Lyude opened 2 months ago

Lyude commented 2 months ago

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:

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 commented 2 months ago

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.