Message-ID: <afa6d33a-c933-4996-8cdf-e1677772d63e@gmail.com>
Date: Mon, 26 Aug 2024 16:59:45 +0200
From: Dirk Behme <dirk.behme@...il.com>
To: Boqun Feng <boqun.feng@...il.com>, Dirk Behme <dirk.behme@...bosch.com>
Cc: Lyude Paul <lyude@...hat.com>, rust-for-linux@...r.kernel.org,
 linux-kernel@...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>, 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

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@...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?
> 
> 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.


>> 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 :)


> 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
> 
>>
> [...]
> 

Powered by blists - more mailing lists