Re: [PATCH v3 1/3] rust: Introduce irq module

[Boqun Feng <boqun.feng@gmail.com>]

On Mon, Aug 26, 2024 at 01:21:17PM +0200, Dirk Behme wrote:
> 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@redhat.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?

No, for example in your code example above, the IRQ is known being
disabled, so actually there's no point to check. The checking of course
makes sense in a function where there is no IRQ	disable code, and you
want to make sure it's only called when IRQ disabled. But that's
something you want to make sure at development time rather than in the
production.

> Wouldn't a permanent if statement checking this be safer for all cases?

I don't think so, it's just a checking, even if we enable this in the
production, the best it could do is just telling us the code is
incorrect. If one realy wants to make sure a function works in both IRQ
disabled and enabled cases, he/she should check the irq status and
handle it accordingly e.g.

	if (irqs_disabled()) {
		// irq is disabled, we can call it directly
		do_sth();
	} else {
		// Disable IRQ on our own.
		local_irq_disable();
		do_sth();
		local_irq_enabled();
	}

> Compare e.g. BUG_ON() or WARN_ONCE() or similar in the kernel's C code?
> 
> Or could we invent anything more clever?
> 

I'm open to any new idea, but for the time being, I think the
debug_assert!() makes more sense.

Regards,
Boqun

> 
[...]