Message-ID: <CALNs47tra0+zdjiBZJgqWBm9v0Q-Osdhj_T_Sh8o00ASzjqWRg@mail.gmail.com>
Date: Fri, 26 Jul 2024 06:13:07 -0400
From: Trevor Gross <tmgross@...ch.edu>
To: Lyude Paul <lyude@...hat.com>
Cc: rust-for-linux@...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>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...sung.com>, 
	Alice Ryhl <aliceryhl@...gle.com>, Martin Rodriguez Reboredo <yakoyoku@...il.com>, 
	FUJITA Tomonori <fujita.tomonori@...il.com>, Aakash Sen Sharma <aakashsensharma@...il.com>, 
	Valentin Obst <kernel@...entinobst.de>, open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] rust: Introduce irq module

On Thu, Jul 25, 2024 at 6:29 PM Lyude Paul <lyude@...hat.com> wrote:
>
> This introduces a module for dealing with interrupt-disabled contexts,
> including the ability to enable and disable interrupts
> (with_irqs_disabled()) - along with the ability to annotate functions as
> expecting that IRQs are already disabled on the local CPU.
>
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> ---
>  rust/helpers.c     | 14 +++++++++
>  rust/kernel/irq.rs | 74 ++++++++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs |  1 +
>  3 files changed, 89 insertions(+)
>  create mode 100644 rust/kernel/irq.rs
>
> [...]
> diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
> new file mode 100644
> index 0000000000000..8a540bd6123f7
> --- /dev/null
> +++ b/rust/kernel/irq.rs
> [...]
> +/// A guarantee that IRQs are disabled on this CPU

Nit: `.` after summary

> +///
> +/// An [`IrqDisabled`] represents a guarantee that interrupts will remain disabled on the current CPU
> +/// until the lifetime of the object ends. However, it does not disable or enable interrupts on its
> +/// own - see [`with_irqs_disabled()`] for that.
> +///

I don't think lifetime necessarily needs to be discussed here, since
this doesn't have any  action on drop. A functional description may be
better, possibly:

        A token that is only available in contexts where IRQs are disabled.

        [`IrqDisabled`] is marker made available when interrupts are
        not active. Certain functions (such as [`SOMETHING`]) take an
        `IrqDisabled` in order to indicate that they may only be run in
        IRQ-free contexts.

        This is a marker type; it has no size, and is simply used as a
        compile-time guarantee that interrupts are disabled where required.

        This token can be created by [`with_irqs_disabled`]. See
        [`with_irqs_disabled`] for examples and further information.


> +/// This object has no cost at runtime (TODO: …except if whatever kernel compile-time option that
> +/// would assert IRQs are enabled or not is enabled - in which case we should actually verify that
> +/// they're enabled).
> +///
> +/// # Examples
> +///
> +/// If you want to ensure that a function may only be invoked within contexts where interrupts are
> +/// disabled, you can do so by requiring that a reference to this type be passed. You can also
> +/// create this type using unsafe code in order to indicate that it's known that interrupts are
> +/// already disabled on this CPU
> +///
> +/// ```
> +/// use kernel::irq::{IrqDisabled, disable_irqs};
> +///
> +/// // Requiring interrupts be disabled to call a function
> +/// fn dont_interrupt_me(_irq: &IrqDisabled<'_>) { }
> +///
> +/// // Disabling interrupts. They'll be re-enabled once this closure completes.
> +/// disable_irqs(|irq| dont_interrupt_me(&irq));
> +/// ```

I think it would be okay to only have examples in one place, possible
`with_irqs_disabled` since that seems like it will get more direct use.
If you would like one here, this one and its docs may need an update
(takes by reference rather than by value).

> +pub struct IrqDisabled<'a>(PhantomData<&'a ()>);

#[derive(Clone, Copy, Debug)]

Since this needs to be duplicatable.

> +impl<'a> IrqDisabled<'a> {
> +    /// Create a new [`IrqDisabled`] without disabling interrupts

Nit: `.` after summary

> +    ///
> +    /// If debug assertions are enabled, this function will check that interrupts are disabled.
> +    /// Otherwise, it has no cost at runtime.
> +    ///
> +    /// # Safety
> +    ///
> +    /// This function must only be called in contexts where it is already known that interrupts have
> +    /// been disabled for the current CPU, as the user is making a promise that they will remain
> +    /// disabled at least until this [`IrqDisabled`] is dropped.
> +    pub unsafe fn new() -> Self {
> +        Self(PhantomData)
> +    }
> +}
> +
> +/// Run the closure `cb` with interrupts disabled on the local CPU.
> +///
> +/// Interrupts will be re-enabled once the closure returns. If interrupts were already disabled on
> +/// this CPU, this is a no-op.

The wording makes it sound like the entire function is a no-op if IRQs
are disabled, rather than the act of disabling IRQs. Could you clarify?

Suggested docs addition:

        This creates an [`IrqDisabled`] token, which can be passed to
        functions that must be run  without interrupts.

        ```
        fn some_sync_action(_irq: IrqDisabled<'_>) {
                /* When the token is available, IRQs are known to be disabled.
                   Actions that rely on this can be safely performed. */
        }

        with_irqs_disabled(|irq_dis| {
                some_sync_action(irq_dis);
        })
        ```

> +#[inline]
> +pub fn with_irqs_disabled<T, F>(cb: F) -> T
> +where
> +    F: FnOnce(IrqDisabled<'_>) -> T,
> +{
> +    // SAFETY: FFI call with no special requirements
> +    let flags = unsafe { bindings::local_irq_save() };
> +
> +    let ret = cb(IrqDisabled(PhantomData));
> +
> +    // SAFETY: `flags` comes from our previous call to local_irq_save
> +    unsafe { bindings::local_irq_restore(flags) };
> +
> +    ret
> +}

Maybe it would be better to put some more extensive module-level docs
with a couple examples, then for the function / type-level docs just
link there?

- Trevor

Powered by blists - more mailing lists