Re: [PATCH v18 5/5] rust: sync: Introduce SpinLockIrq::lock_with() and friends

[lyude@redhat.com]

On Thu, 2026-02-05 at 23:23 +0000, Gary Guo wrote:
> On Thu Feb 5, 2026 at 8:44 PM GMT, Lyude Paul wrote:
> > `SpinLockIrq` and `SpinLock` use the exact same underlying C
> > structure,
> > with the only real difference being that the former uses the
> > irq_disable()
> > and irq_enable() variants for locking/unlocking. These variants can
> > introduce some minor overhead in contexts where we already know
> > that
> > local processor interrupts are disabled, and as such we want a way
> > to be
> > able to skip modifying processor interrupt state in said contexts
> > in order
> > to avoid some overhead - just like the current C API allows us to
> > do.
> > 
> > In order to do this, we add some special functions for SpinLockIrq:
> > lock_with() and try_lock_with(), which allow acquiring the lock
> > without
> > changing the interrupt state - as long as the caller can provide a
> > LocalInterruptDisabled reference to prove that local processor
> > interrupts
> > have been disabled.
> > 
> > In some hacked-together benchmarks we ran, most of the time this
> > did
> > actually seem to lead to a noticeable difference in overhead:
> > 
> >   From an aarch64 VM running on a MacBook M4:
> >     lock() when irq is disabled, 100 times cost Delta { nanos: 500
> > }
> >     lock_with() when irq is disabled, 100 times cost Delta { nanos:
> > 292 }
> >     lock() when irq is enabled, 100 times cost Delta { nanos: 834 }
> > 
> >     lock() when irq is disabled, 100 times cost Delta { nanos: 459
> > }
> >     lock_with() when irq is disabled, 100 times cost Delta { nanos:
> > 291 }
> >     lock() when irq is enabled, 100 times cost Delta { nanos: 709 }
> > 
> >   From an x86_64 VM (qemu/kvm) running on a i7-13700H
> >     lock() when irq is disabled, 100 times cost Delta { nanos: 1002
> > }
> >     lock_with() when irq is disabled, 100 times cost Delta { nanos:
> > 729 }
> >     lock() when irq is enabled, 100 times cost Delta { nanos: 1516
> > }
> > 
> >     lock() when irq is disabled, 100 times cost Delta { nanos: 754
> > }
> >     lock_with() when irq is disabled, 100 times cost Delta { nanos:
> > 966 }
> >     lock() when irq is enabled, 100 times cost Delta { nanos: 1227
> > }
> > 
> >     (note that there were some runs on x86_64 where lock() on irq
> > disabled
> >     vs. lock_with() on irq disabled had equivalent benchmarks, but
> > it very
> >     much appeared to be a minority of test runs.
> > 
> > While it's not clear how this affects real-world workloads yet,
> > let's add
> > this for the time being so we can find out.
> > 
> > This makes it so that a `SpinLockIrq` will work like a `SpinLock`
> > if
> > interrupts are disabled. So a function:
> > 
> >         (&'a SpinLockIrq, &'a InterruptDisabled) -> Guard<'a, ..,
> > SpinLockBackend>
> > 
> > makes sense. Note that due to `Guard` and `InterruptDisabled`
> > having the
> > same lifetime, interrupts cannot be enabled while the Guard exists.
> > 
> > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > 
> > ---
> > This was originally two patches, but keeping them split didn't make
> > sense
> > after going from BackendInContext to BackendWithContext.
> > 
> > V10:
> > * Fix typos - Dirk/Lyude
> > * Since we're adding support for context locks to GlobalLock as
> > well, let's
> >   also make sure to cover try_lock while we're at it and add
> > try_lock_with
> > * Add a private function as_lock_in_context() for handling casting
> > from a
> >   Lock<T, B> to Lock<T, B::ContextualBackend> so we don't have to
> > duplicate
> >   safety comments
> > V11:
> > * Fix clippy::ref_as_ptr error in Lock::as_lock_in_context()
> > V14:
> > * Add benchmark results, rewrite commit message
> > V17:
> > * Introduce `BackendWithContext`, move context-related bits into
> > there and
> >   out of `Backend`.
> > * Add missing #[must_use = …] for try_lock_with()
> > * Remove all unsafe code from lock_with() and try_lock_with():
> >   Somehow I never noticed that literally none of the unsafe code in
> > these
> >   two functions is needed with as_lock_in_context()...
> > V18:
> > * Get rid of BackendWithContext
> > * Just use transmute in as_lock_in_context()
> > * Now that we're only supporting IRQ spinlocks and not using
> > traits, use
> >   the type aliases for SpinLock and SpinLockGuard
> > * Improve the docs now that we're not using traits.
> > 
> >  rust/kernel/sync/lock/spinlock.rs | 78
> > ++++++++++++++++++++++++++++++-
> >  1 file changed, 77 insertions(+), 1 deletion(-)
> > 
> > diff --git a/rust/kernel/sync/lock/spinlock.rs
> > b/rust/kernel/sync/lock/spinlock.rs
> > index f11a84505ba0e..de736cb777e93 100644
> > --- a/rust/kernel/sync/lock/spinlock.rs
> > +++ b/rust/kernel/sync/lock/spinlock.rs
> > @@ -4,7 +4,10 @@
> >  //!
> >  //! This module allows Rust code to use the kernel's `spinlock_t`.
> >  use super::*;
> > -use crate::prelude::*;
> > +use crate::{
> > +    interrupt::LocalInterruptDisabled,
> > +    prelude::*, //
> > +};
> >  
> >  /// Creates a [`SpinLock`] initialiser with the given name and a
> > newly-created lock class.
> >  ///
> > @@ -224,6 +227,45 @@ macro_rules! new_spinlock_irq {
> >  /// # Ok::<(), Error>(())
> >  /// ```
> >  ///
> > +/// The next example demonstrates locking a [`SpinLockIrq`] using
> > [`lock_with()`] in a function
> > +/// which can only be called when local processor interrupts are
> > already disabled.
> > +///
> > +/// ```
> > +/// use kernel::sync::{new_spinlock_irq, SpinLockIrq};
> > +/// use kernel::interrupt::*;
> > +///
> > +/// struct Inner {
> > +///     a: u32,
> > +/// }
> > +///
> > +/// #[pin_data]
> > +/// struct Example {
> > +///     #[pin]
> > +///     inner: SpinLockIrq<Inner>,
> > +/// }
> > +///
> > +/// impl Example {
> > +///     fn new() -> impl PinInit<Self> {
> > +///         pin_init!(Self {
> > +///             inner <- new_spinlock_irq!(Inner { a: 20 }),
> > +///         })
> > +///     }
> > +/// }
> > +///
> > +/// // Accessing an `Example` from a function that can only be
> > called in no-interrupt contexts.
> > +/// fn noirq_work(e: &Example, interrupt_disabled:
> > &LocalInterruptDisabled) {
> > +///     // Because we know interrupts are disabled from
> > interrupt_disable, we can skip toggling
> > +///     // interrupt state using lock_with() and the provided
> > token
> > +///     assert_eq!(e.inner.lock_with(interrupt_disabled).a, 20);
> > +/// }
> > +///
> > +/// # let e = KBox::pin_init(Example::new(), GFP_KERNEL)?;
> > +/// # let interrupt_guard = local_interrupt_disable();
> > +/// # noirq_work(&e, &interrupt_guard);
> > +/// #
> > +/// # Ok::<(), Error>(())
> > +/// ```
> > +///
> >  /// [`lock()`]: SpinLockIrq::lock
> >  /// [`lock_with()`]: SpinLockIrq::lock_with
> >  pub type SpinLockIrq<T> = super::Lock<T, SpinLockIrqBackend>;
> > @@ -286,6 +328,40 @@ unsafe fn assert_is_held(ptr: *mut
> > Self::State) {
> >      }
> >  }
> >  
> 
> This is much cleaner now compared the previous series. Thanks!
> 
> With functions marked as `#[inline]` as appropriate:
> 
>     Reviewed-by: Gary Guo <gary@garyguo.net>
> 
> BTW: do we want to have a
> 
>     impl<'a> lock::Guard<'a, T, SpinLockIrqBackend> {
>         fn as_local_irq_disabled(&self) -> &LocalInterruptDisabled;
>     }
> 

Yes imo - since it lets us get to the level of skipping assertions that
C is currently able to hit. Though, I wonder if this would be better
suited for AsRef?

I will save this for a follow-up patch though, since we're so close to
getting this in right now.

> ?
> 
> Best,
> Gary
> 
> > +impl<T: ?Sized> Lock<T, SpinLockIrqBackend> {
> > +    /// Casts the lock as a `Lock<T, SpinLockBackend>`.
> > +    fn as_lock_in_interrupt<'a>(&'a self, _context: &'a
> > LocalInterruptDisabled) -> &'a SpinLock<T> {
> > +        // SAFETY:
> > +        // - `Lock<T, SpinLockBackend>` and `Lock<T,
> > SpinLockIrqBackend>` both have identical data
> > +        //   layouts.
> > +        // - As long as local interrupts are disabled (which is
> > proven to be true by _context), it
> > +        //   is safe to treat a lock with SpinLockIrqBackend as a
> > SpinLockBackend lock.
> > +        unsafe { core::mem::transmute(self) }
> > +    }
> > +
> > +    /// Acquires the lock without modifying local interrupt state.
> > +    ///
> > +    /// This function should be used in place of the more
> > expensive [`Lock::lock()`] function when
> > +    /// possible for [`SpinLockIrq`] locks.
> > +    pub fn lock_with<'a>(&'a self, context: &'a
> > LocalInterruptDisabled) -> SpinLockGuard<'a, T> {
> > +        self.as_lock_in_interrupt(context).lock()
> > +    }
> > +
> > +    /// Tries to acquire the lock without modifying local
> > interrupt state.
> > +    ///
> > +    /// This function should be used in place of the more
> > expensive [`Lock::try_lock()`] function
> > +    /// when possible for [`SpinLockIrq`] locks.
> > +    ///
> > +    /// Returns a guard that can be used to access the data
> > protected by the lock if successful.
> > +    #[must_use = "if unused, the lock will be immediately
> > unlocked"]
> > +    pub fn try_lock_with<'a>(
> > +        &'a self,
> > +        context: &'a LocalInterruptDisabled,
> > +    ) -> Option<SpinLockGuard<'a, T>> {
> > +        self.as_lock_in_interrupt(context).try_lock()
> > +    }
> > +}
> > +
> >  #[kunit_tests(rust_spinlock_irq_condvar)]
> >  mod tests {
> >      use super::*;