Message-Id: <DG7EVKBS76KU.1XRUJ92G16BIH@garyguo.net>
Date: Thu, 05 Feb 2026 23:19:33 +0000
From: "Gary Guo" <gary@...yguo.net>
To: "Lyude Paul" <lyude@...hat.com>, <rust-for-linux@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, "Thomas Gleixner" <tglx@...utronix.de>
Cc: "Boqun Feng" <boqun.feng@...il.com>, "Daniel Almeida"
 <daniel.almeida@...labora.com>, "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"
 <lossin@...nel.org>, "Andreas Hindborg" <a.hindborg@...nel.org>, "Alice
 Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>, "Danilo
 Krummrich" <dakr@...nel.org>, "Andrew Morton" <akpm@...ux-foundation.org>,
 "Peter Zijlstra" <peterz@...radead.org>, "Ingo Molnar" <mingo@...hat.com>,
 "Will Deacon" <will@...nel.org>, "Waiman Long" <longman@...hat.com>
Subject: Re: [PATCH v18 4/5] rust: sync: Add SpinLockIrq

On Thu Feb 5, 2026 at 8:44 PM GMT, Lyude Paul wrote:
> A variant of `SpinLock` that ensures interrupts are disabled in the
> critical section. `lock()` will ensure that either interrupts are already
> disabled or disable them. `unlock()` will reverse the respective operation.
>
> [Boqun: Port to use spin_lock_irq_disable() and
> spin_unlock_irq_enable()]
>
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> Co-developed-by: Boqun Feng <boqun.feng@...il.com>
> Signed-off-by: Boqun Feng <boqun.feng@...il.com>
>
> ---
> V10:
> * Also add support to GlobalLock
> * Documentation fixes from Dirk
> V11:
> * Add unit test requested by Daniel Almeida
> V14:
> * Improve rustdoc for SpinLockIrqBackend
> V17:
> * Update Git summary according to Benno's review
> V18:
> * Add missing comment change requested by Benno in V16
>
>  rust/kernel/sync.rs               |   4 +-
>  rust/kernel/sync/lock/global.rs   |   3 +
>  rust/kernel/sync/lock/spinlock.rs | 228 ++++++++++++++++++++++++++++++
>  3 files changed, 234 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> index 993dbf2caa0e3..58af785d62b99 100644
> --- a/rust/kernel/sync.rs
> +++ b/rust/kernel/sync.rs
> @@ -27,7 +27,9 @@
>  pub use condvar::{new_condvar, CondVar, CondVarTimeoutResult};
>  pub use lock::global::{global_lock, GlobalGuard, GlobalLock, GlobalLockBackend, GlobalLockedBy};
>  pub use lock::mutex::{new_mutex, Mutex, MutexGuard};
> -pub use lock::spinlock::{new_spinlock, SpinLock, SpinLockGuard};
> +pub use lock::spinlock::{
> +    new_spinlock, new_spinlock_irq, SpinLock, SpinLockGuard, SpinLockIrq, SpinLockIrqGuard,
> +};

Please convert to kernel import style

>  pub use locked_by::LockedBy;
>  pub use refcount::Refcount;
>  pub use set_once::SetOnce;
> diff --git a/rust/kernel/sync/lock/global.rs b/rust/kernel/sync/lock/global.rs
> index aecbdc34738fb..32efcbc870500 100644
> --- a/rust/kernel/sync/lock/global.rs
> +++ b/rust/kernel/sync/lock/global.rs
> @@ -304,4 +304,7 @@ macro_rules! global_lock_inner {
>      (backend SpinLock) => {
>          $crate::sync::lock::spinlock::SpinLockBackend
>      };
> +    (backend SpinLockIrq) => {
> +        $crate::sync::lock::spinlock::SpinLockIrqBackend
> +    };
>  }
> diff --git a/rust/kernel/sync/lock/spinlock.rs b/rust/kernel/sync/lock/spinlock.rs
> index d75af32218bae..f11a84505ba0e 100644
> --- a/rust/kernel/sync/lock/spinlock.rs
> +++ b/rust/kernel/sync/lock/spinlock.rs
> @@ -4,6 +4,7 @@
>  //!
>  //! This module allows Rust code to use the kernel's `spinlock_t`.
>  use super::*;
> +use crate::prelude::*;
>  
>  /// Creates a [`SpinLock`] initialiser with the given name and a newly-created lock class.
>  ///
> @@ -143,3 +144,230 @@ unsafe fn assert_is_held(ptr: *mut Self::State) {
>          unsafe { bindings::spin_assert_is_held(ptr) }
>      }
>  }
> +
> +/// Creates a [`SpinLockIrq`] initialiser with the given name and a newly-created lock class.
> +///
> +/// It uses the name if one is given, otherwise it generates one based on the file name and line
> +/// number.
> +#[macro_export]
> +macro_rules! new_spinlock_irq {
> +    ($inner:expr $(, $name:literal)? $(,)?) => {
> +        $crate::sync::SpinLockIrq::new(
> +            $inner, $crate::optional_name!($($name)?), $crate::static_lock_class!())
> +    };
> +}
> +pub use new_spinlock_irq;
> +
> +/// A variant of `SpinLock` that ensures interrupts are disabled in the critical section.
> +///
> +/// This is a version of [`SpinLock`] that can only be used in contexts where interrupts for the
> +/// local CPU are disabled. It can be acquired in two ways:
> +///
> +/// - Using [`lock()`] like any other type of lock, in which case the bindings will modify the
> +///   interrupt state to ensure that local processor interrupts remain disabled for at least as long
> +///   as the [`SpinLockIrqGuard`] exists.
> +/// - Using [`lock_with()`] in contexts where a [`LocalInterruptDisabled`] token is present and
> +///   local processor interrupts are already known to be disabled, in which case the local interrupt
> +///   state will not be touched. This method should be preferred if a [`LocalInterruptDisabled`]
> +///   token is present in the scope.
> +///
> +/// For more info on spinlocks, see [`SpinLock`]. For more information on interrupts,
> +/// [see the interrupt module](kernel::interrupt).
> +///
> +/// # Examples
> +///
> +/// The following example shows how to declare, allocate initialise and access a struct (`Example`)
> +/// that contains an inner struct (`Inner`) that is protected by a spinlock that requires local
> +/// processor interrupts to be disabled.
> +///
> +/// ```
> +/// use kernel::sync::{new_spinlock_irq, SpinLockIrq};
> +///
> +/// struct Inner {
> +///     a: u32,
> +///     b: u32,
> +/// }
> +///
> +/// #[pin_data]
> +/// struct Example {
> +///     #[pin]
> +///     c: SpinLockIrq<Inner>,
> +///     #[pin]
> +///     d: SpinLockIrq<Inner>,
> +/// }
> +///
> +/// impl Example {
> +///     fn new() -> impl PinInit<Self> {
> +///         pin_init!(Self {
> +///             c <- new_spinlock_irq!(Inner { a: 0, b: 10 }),
> +///             d <- new_spinlock_irq!(Inner { a: 20, b: 30 }),
> +///         })
> +///     }
> +/// }
> +///
> +/// // Allocate a boxed `Example`
> +/// let e = KBox::pin_init(Example::new(), GFP_KERNEL)?;
> +///
> +/// // Accessing an `Example` from a context where interrupts may not be disabled already.
> +/// let c_guard = e.c.lock(); // interrupts are disabled now, +1 interrupt disable refcount
> +/// let d_guard = e.d.lock(); // no interrupt state change, +1 interrupt disable refcount
> +///
> +/// assert_eq!(c_guard.a, 0);
> +/// assert_eq!(c_guard.b, 10);
> +/// assert_eq!(d_guard.a, 20);
> +/// assert_eq!(d_guard.b, 30);
> +///
> +/// drop(c_guard); // Dropping c_guard will not re-enable interrupts just yet, since d_guard is
> +///                // still in scope.
> +/// drop(d_guard); // Last interrupt disable reference dropped here, so interrupts are re-enabled
> +///                // now
> +/// # Ok::<(), Error>(())
> +/// ```
> +///
> +/// [`lock()`]: SpinLockIrq::lock
> +/// [`lock_with()`]: SpinLockIrq::lock_with
> +pub type SpinLockIrq<T> = super::Lock<T, SpinLockIrqBackend>;
> +
> +/// A kernel `spinlock_t` lock backend that can only be acquired in interrupt disabled contexts.
> +pub struct SpinLockIrqBackend;
> +
> +/// A [`Guard`] acquired from locking a [`SpinLockIrq`] using [`lock()`].
> +///
> +/// This is simply a type alias for a [`Guard`] returned from locking a [`SpinLockIrq`] using
> +/// [`lock_with()`]. It will unlock the [`SpinLockIrq`] and decrement the local processor's
> +/// interrupt disablement refcount upon being dropped.
> +///
> +/// [`lock()`]: SpinLockIrq::lock
> +/// [`lock_with()`]: SpinLockIrq::lock_with
> +pub type SpinLockIrqGuard<'a, T> = Guard<'a, T, SpinLockIrqBackend>;
> +
> +// SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion. `relock` uses the
> +// default implementation that always calls the same locking method.
> +unsafe impl Backend for SpinLockIrqBackend {
> +    type State = bindings::spinlock_t;
> +    type GuardState = ();
> +

#[inline] hints for all of these functions.

> +    unsafe fn init(
> +        ptr: *mut Self::State,
> +        name: *const crate::ffi::c_char,
> +        key: *mut bindings::lock_class_key,
> +    ) {
> +        // SAFETY: The safety requirements ensure that `ptr` is valid for writes, and `name` and
> +        // `key` are valid for read indefinitely.
> +        unsafe { bindings::__spin_lock_init(ptr, name, key) }
> +    }
> +
> +    unsafe fn lock(ptr: *mut Self::State) -> Self::GuardState {
> +        // SAFETY: The safety requirements of this function ensure that `ptr` points to valid
> +        // memory, and that it has been initialised before.
> +        unsafe { bindings::spin_lock_irq_disable(ptr) }
> +    }
> +
> +    unsafe fn unlock(ptr: *mut Self::State, _guard_state: &Self::GuardState) {
> +        // SAFETY: The safety requirements of this function ensure that `ptr` is valid and that the
> +        // caller is the owner of the spinlock.
> +        unsafe { bindings::spin_unlock_irq_enable(ptr) }
> +    }
> +
> +    unsafe fn try_lock(ptr: *mut Self::State) -> Option<Self::GuardState> {
> +        // SAFETY: The `ptr` pointer is guaranteed to be valid and initialized before use.
> +        let result = unsafe { bindings::spin_trylock_irq_disable(ptr) };
> +
> +        if result != 0 {
> +            Some(())
> +        } else {
> +            None
> +        }
> +    }
> +
> +    unsafe fn assert_is_held(ptr: *mut Self::State) {
> +        // SAFETY: The `ptr` pointer is guaranteed to be valid and initialized before use.
> +        unsafe { bindings::spin_assert_is_held(ptr) }
> +    }
> +}
> +
> +#[kunit_tests(rust_spinlock_irq_condvar)]
> +mod tests {
> +    use super::*;
> +    use crate::{
> +        sync::*,
> +        workqueue::{self, impl_has_work, new_work, Work, WorkItem},
> +    };
> +
> +    struct TestState {
> +        value: u32,
> +        waiter_ready: bool,
> +    }
> +
> +    #[pin_data]
> +    struct Test {
> +        #[pin]
> +        state: SpinLockIrq<TestState>,
> +
> +        #[pin]
> +        state_changed: CondVar,
> +
> +        #[pin]
> +        waiter_state_changed: CondVar,
> +
> +        #[pin]
> +        wait_work: Work<Self>,
> +    }
> +
> +    impl_has_work! {
> +        impl HasWork<Self> for Test { self.wait_work }
> +    }
> +
> +    impl Test {
> +        pub(crate) fn new() -> Result<Arc<Self>> {
> +            Arc::try_pin_init(
> +                try_pin_init!(
> +                    Self {
> +                        state <- new_spinlock_irq!(TestState {
> +                            value: 1,
> +                            waiter_ready: false
> +                        }),
> +                        state_changed <- new_condvar!(),
> +                        waiter_state_changed <- new_condvar!(),
> +                        wait_work <- new_work!("IrqCondvarTest::wait_work")
> +                    }
> +                ),
> +                GFP_KERNEL,
> +            )
> +        }
> +    }
> +
> +    impl WorkItem for Test {
> +        type Pointer = Arc<Self>;
> +
> +        fn run(this: Arc<Self>) {
> +            // Wait for the test to be ready to wait for us
> +            let mut state = this.state.lock();

Perhaps throw in an irq actually disabled assertion from the module in patch 1
here?

With these fixed:

Reviewed-by: Gary Guo <gary@...yguo.net>

Best,
Gary

> +
> +            while !state.waiter_ready {
> +                this.waiter_state_changed.wait(&mut state);
> +            }
> +
> +            // Deliver the exciting value update our test has been waiting for
> +            state.value += 1;
> +            this.state_changed.notify_sync();
> +        }
> +    }
> +
> +    #[test]
> +    fn spinlock_irq_condvar() -> Result {
> +        let testdata = Test::new()?;
> +
> +        let _ = workqueue::system().enqueue(testdata.clone());
> +
> +        // Let the updater know when we're ready to wait
> +        let mut state = testdata.state.lock();
> +        state.waiter_ready = true;
> +        testdata.waiter_state_changed.notify_sync();
> +
> +        // Wait for the exciting value update
> +        testdata.state_changed.wait(&mut state);
> +        assert_eq!(state.value, 2);
> +        Ok(())
> +    }
> +}

Powered by blists - more mailing lists