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

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

On Mon, Aug 26, 2024 at 04:59:45PM +0200, Dirk Behme wrote:
> Am 26.08.24 um 16:21 schrieb Boqun Feng:
> > 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 knon 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.
> 
> I think I'm thinking the other way around ;)
> 
> Make sure I get a warning if I'm (as the developer) have done anything wrong
> in my assumption about the interrupt state my code is running with.
> 
> So cover the human failure case.
> 

Again, if a developer wants to find whether the code is correct or now,
that falls into the debugging catagory. If you want to know that, you
just enable the debug_assert!().

> 
> > > 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.
> 
> Yes, exactly, this is what I'm looking for. Isn't this what the C's
> WARN_ONCE() & friends are about? Let the machine tell us that the programmer
> has done something wrong :)
> 

Not really, sometimes they are used to tell users that there are
hardware issues. But moreover, they are used to catch unexpected cases,
which as I mention above, not all the IRQ-disabled usages need this.

Regards,
Boqun

> 
> > 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
> 
> No, this is not what I'm looking for. I'm just about noticing the
> programming error case.
> 
> Best regards
> 
> Dirk
> 
> 
> > 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
> > 
> > > 
> > [...]
> > 
>