Message-ID: <3f6a5c86-8dc4-4a62-8308-5ca25f9e4aec@de.bosch.com>
Date: Mon, 26 Aug 2024 13:21:17 +0200
From: Dirk Behme <dirk.behme@...bosch.com>
To: Lyude Paul <lyude@...hat.com>, <rust-for-linux@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
CC: 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>,
	"FUJITA Tomonori" <fujita.tomonori@...il.com>, Aakash Sen Sharma
	<aakashsensharma@...il.com>, Valentin Obst <kernel@...entinobst.de>
Subject: Re: [PATCH v3 1/3] rust: Introduce irq module

Hi Lyude,

On 02.08.2024 02:10, Lyude Paul 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>
...
> diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
> new file mode 100644
> index 0000000000000..b52f8073e5cd0
> --- /dev/null
> +++ b/rust/kernel/irq.rs
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Interrupt controls
> +//!
> +//! This module allows Rust code to control processor interrupts. [`with_irqs_disabled()`] may be
> +//! used for nested disables of interrupts, whereas [`IrqDisabled`] can be used for annotating code
> +//! that requires interrupts to be disabled.
> +
> +use bindings;
> +use core::marker::*;
> +
> +/// A token that is only available in contexts where IRQs are disabled.
> +///
> +/// [`IrqDisabled`] is marker made available when interrupts are not active. Certain functions 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.
> +#[derive(Copy, Clone, Debug, Ord, Eq, PartialOrd, PartialEq, Hash)]
> +pub struct IrqDisabled<'a>(PhantomData<(&'a (), *mut ())>);
> +
> +impl IrqDisabled<'_> {
> +    /// Create a new [`IrqDisabled`] without disabling interrupts.
> +    ///
> +    /// This creates an [`IrqDisabled`] token, which can be passed to functions that must be run
> +    /// without interrupts. If debug assertions are enabled, this function will assert that
> +    /// interrupts are disabled upon creation. Otherwise, it has no size or cost at runtime.
> +    ///
> +    /// # Panics
> +    ///
> +    /// If debug assertions are enabled, this function will panic if interrupts are not disabled
> +    /// upon creation.
> +    ///
> +    /// # 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 {
> +        // SAFETY: FFI call with no special requirements
> +        debug_assert!(unsafe { bindings::irqs_disabled() });
> +
> +        Self(PhantomData)
> +    }
> +}

I have some (understanding) questions for this IrqDisabled::new():

1. It looks to me that both examples, below in this file irq.rs nor the 
with_irqs_disabled() example in spinlock.rs in the 3rd patch seem to use 
IrqDisabled::new()? At least some debug pr_info() added here doesn't 
print anything.

What's the intended use case of IrqDisabled::new()? Do we have any example?

I 'simulated' an interrupt handler where we know the interrupts are 
disabled:

let flags = unsafe { bindings::local_irq_save() }; // Simulate IRQ 
disable of an interrupt handler
let mut g = foo.lock_with(unsafe {IrqDisabled::new() });
g.val = 42;
unsafe { bindings::local_irq_restore(flags) }; // Simulate IRQ re-enable 
of an interrupt handler

Is this the intended use case?


2. If the example above is what is intended here, is it intended that 
this has to be called unsafe{}?

foo.lock_with(unsafe {IrqDisabled::new() });


3. I somehow feel slightly uncomfortable with the debug_assert!().

I understood that this is intended to be not in production code and only 
enabled with RUST_DEBUG_ASSERTIONS for performance reasons? But I have 
some doubts how many people have RUST_DEBUG_ASSERTIONS enabled? And 
disable it for the production build?

Wouldn't it be better to be on the safe side and have this check, 
always? Wouldn't a permanent if statement checking this be safer for all 
cases? Compare e.g. BUG_ON() or WARN_ONCE() or similar in the kernel's C 
code?

Or could we invent anything more clever?


> +/// 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));
> +/// ```

Best regards

Dirk

Powered by blists - more mailing lists