Message-ID: <ZxMRNFouhf30ibVy@Boquns-Mac-mini.local>
Date: Fri, 18 Oct 2024 18:53:56 -0700
From: Boqun Feng <boqun.feng@...il.com>
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>,
	Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...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@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Wedson Almeida Filho <wedsonaf@...il.com>,
	Danilo Krummrich <dakr@...nel.org>,
	FUJITA Tomonori <fujita.tomonori@...il.com>,
	Valentin Obst <kernel@...entinobst.de>
Subject: Re: [PATCH v8 1/3] rust: Introduce local_irq module

On Fri, Oct 18, 2024 at 07:22:25PM -0400, Lyude Paul wrote:
> This introduces a module for adding marker types to indicate the type of
> interrupt context a function is called in. Note that nothing here enables
> or disables interrupts on its own, this is simply for verifying correctness
> at compile-time.
> 
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> 
> ---
> 
> V2:
> * Actually make it so that we check whether or not we have interrupts
>   disabled with debug assertions
> * Fix issues in the documentation (added suggestions, missing periods, made
>   sure that all rustdoc examples compile properly)
> * Pass IrqDisabled by value, not reference
> * Ensure that IrqDisabled is !Send and !Sync using
>   PhantomData<(&'a (), *mut ())>
> * Add all of the suggested derives from Benno Lossin
> 
> V3:
> * Use `impl` for FnOnce bounds in with_irqs_disabled()
> * Use higher-ranked trait bounds for the lifetime of with_irqs_disabled()
> * Wording changes in the documentation for the module itself
> 
> V4:
> * Use the actual unsafe constructor for IrqDisabled in
>   with_irqs_disabled()
> * Fix comment style in with_irqs_disabled example
> * Check before calling local_irq_restore() in with_irqs_disabled that
>   interrupts are still disabled. It would have been nice to do this from a
>   Drop implementation like I hoped, but I realized rust doesn't allow that
>   for types that implement Copy.
> * Document that interrupts can't be re-enabled within the `cb` provided to
>   `with_irqs_disabled`, and link to the github issue I just filed about
>   this that describes the solution for this.
> 
> V5:
> * Rebase against rust-next for the helpers split
> * Fix typo (enabled -> disabled) - Dirk
> 
> V6:
> * s/indicate/require/ in IrqDisabled docs
> * Reword comment above with_irqs_disabled in code example requested by
>   Benno
> * Add paragraph to with_irqs_disabled docs requested by Benno
> * Apply the comments from Boqun's review for V4 that I missed
> * Document type invariants of `IrqDisabled`
> 
> V7:
> * Change name of module to local_irq.rs
> * Remove with_interrupts_disabled()
> * Update documentation wording a bit to make mention of PREEMPT_RT
> 

So with the current approach in your patch, there is no safe way to
enable interrrupts (or restore the interrupt state). Therefore I don't
think there is a safe version of CondVar::wait(), at least it won't be
easy to use at the current API. Right?

Whereas the POC [1] approach does support CondVar::wait() and a guard
interface, and has a relatively simple (and more Rusty) design. To
Thomas' point [2], I think we should look at it (interrupt disabling and
nested interrupt disabling) from Rust perspective and provide a cleaner
API in Rust.

And I think a guard interface (and we can even have a guard interface
for interrupt disabling itself) is what we *want* to provide to the
users, the fact that we cannot provide it currently is something we want
to workaround. Maybe we should focus on providing these better APIs with
acceptable small workarounds if necessary first.

Anyway, just make some of my points. Looking forward to hearing your
feedbacks!

Regards,
Boqun

[1]: https://lore.kernel.org/rust-for-linux/ce05b149-63ca-46c9-8eb3-67ff3dc5b2c5@gmail.com/
[2]: https://lore.kernel.org/rust-for-linux/87iktrahld.ffs@tglx/
[3]: https://lore.kernel.org/rust-for-linux/20241018055125.2784186-2-boqun.feng@gmail.com/

> This patch depends on
> https://lore.kernel.org/rust-for-linux/20240808-alice-file-v9-1-2cb7b934e0e1@google.com/
> 
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> ---
>  rust/helpers/helpers.c   |  1 +
>  rust/helpers/irq.c       |  8 ++++++
>  rust/kernel/lib.rs       |  1 +
>  rust/kernel/local_irq.rs | 56 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 66 insertions(+)
>  create mode 100644 rust/helpers/irq.c
>  create mode 100644 rust/kernel/local_irq.rs
> 
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 20a0c69d5cc7b..fd70afe5069ca 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -12,6 +12,7 @@
>  #include "build_assert.c"
>  #include "build_bug.c"
>  #include "err.c"
> +#include "irq.c"
>  #include "kunit.c"
>  #include "mutex.c"
>  #include "page.c"
> diff --git a/rust/helpers/irq.c b/rust/helpers/irq.c
> new file mode 100644
> index 0000000000000..d129094cc1940
> --- /dev/null
> +++ b/rust/helpers/irq.c
> @@ -0,0 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/irqflags.h>
> +
> +bool rust_helper_irqs_disabled(void)
> +{
> +	return irqs_disabled();
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 620de74d128f9..b7e604bc968ce 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -40,6 +40,7 @@
>  #[cfg(CONFIG_KUNIT)]
>  pub mod kunit;
>  pub mod list;
> +pub mod local_irq;
>  #[cfg(CONFIG_NET)]
>  pub mod net;
>  pub mod page;
> diff --git a/rust/kernel/local_irq.rs b/rust/kernel/local_irq.rs
> new file mode 100644
> index 0000000000000..e9e82347392c7
> --- /dev/null
> +++ b/rust/kernel/local_irq.rs
> @@ -0,0 +1,56 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Interrupt controls
> +//!
> +//! This module allows Rust code to annotate functions which can only be called in contexts where
> +//! local interrupts on the CPU may be disabled.
> +
> +use crate::types::NotThreadSafe;
> +use bindings;
> +use core::marker::*;
> +
> +/// A token that is only available in contexts where interrupts are disabled on non-PREEMPT_RT
> +/// kernels.
> +///
> +/// [`IrqDisabled`] is marker made available when local processor interrupts are disabled on
> +/// non-PREEMPT_RT kernels. A function may require a [`IrqDisabled`] to ensure that functions may
> +/// only be run with processor interrupts disabled on such kernels.
> +///
> +/// This is a marker type; it has no size, and is simply used as a compile-time guarantee that
> +/// interrupts are disabled where required.
> +///
> +/// # Invariants
> +///
> +/// On kernels where `CONFIG_PREEMPT_RT=n` is set in the kernel config, local processor interrupts
> +/// are disabled whenever an object of this type exists.
> +#[derive(Copy, Clone, Debug, Ord, Eq, PartialOrd, PartialEq, Hash)]
> +pub struct IrqDisabled<'a>(PhantomData<(&'a (), NotThreadSafe)>);
> +
> +impl IrqDisabled<'_> {
> +    /// Create a new [`IrqDisabled`] token in an interrupt disabled context.
> +    ///
> +    /// This creates a [`IrqDisabled`] token, which can be passed to functions that must
> +    /// be run without interrupts on kernels where `CONFIG_PREEMPT_RT=n`. If debug assertions are
> +    /// enabled, this function will assert that interrupts match the expected state. Otherwise, it
> +    /// has no size or cost at runtime.
> +    ///
> +    /// # Panics
> +    ///
> +    /// If debug assertions are enabled, then this function will assert on non-PREEMPT_RT kernels
> +    /// that local processor interrupts are disabled upon creation.
> +    ///
> +    /// # Safety
> +    ///
> +    /// This function must only be called in contexts where it is known that on a non-PREEMPT_RT
> +    /// kernel, local interrupts have been disabled for the current CPU. 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() });
> +
> +        // INVARIANT: The safety requirements of this function ensure that IRQs are disabled on
> +        // non-PREEMPT_RT kernels.
> +        Self(PhantomData)
> +    }
> +}
> -- 
> 2.47.0
> 

Powered by blists - more mailing lists