Re: [PATCH v4 05/17] irq & spin_lock: Add counted interrupt disabling/enabling

On Wed, Aug 05, 2026 at 02:21:49AM +0530, Shrikanth Hegde wrote:
> 
> 
> On 8/4/26 9:44 PM, Boqun Feng wrote:
> > Currently the nested interrupt disabling and enabling is represented by
> > _irqsave() and _irqrestore() APIs, which are relatively unsafe, for
> > example:
> > 
> > 	<interrupts are enabled as beginning>
> > 	spin_lock_irqsave(l1, flag1);
> > 	spin_lock_irqsave(l2, flag2);
> > 	spin_unlock_irqrestore(l1, flags1);
> > 	<l2 is still held but interrupts are enabled>
> > 	// accesses to interrupt-disable protected data will cause races
> > 
> > This is even easier to trigger with guard facilities:
> > 
> > 	unsigned long flag2;
> > 
> > 	scoped_guard(spin_lock_irqsave, l1) {
> > 		spin_lock_irqsave(l2, flag2);
> > 	}
> > 	// l2 locked but interrupts are enabled.
> > 	spin_unlock_irqrestore(l2, flag2);
> > 
> > (Hand-to-hand locking critical sections are not uncommon for a
> > fine-grained lock design)
> > 
> > And because of this unsafety, Rust cannot easily wrap the
> > interrupt-disabling locks in a safe API, which complicates the design.
> > 
> > To resolve this, introduce a new set of interrupt disabling APIs:
> > 
> > *	local_interrupt_disable();
> > *	local_interrupt_enable();
> > 
> > They work like local_irq_save() and local_irq_restore() except that 1)
> > the outermost local_interrupt_disable() call saves the interrupt state
> > into a per-CPU variable, so that the outermost local_interrupt_enable()
> > can restore the state, and 2) a per-CPU counter is added to record the
> > nest level of these calls, so that interrupts are not accidentally
> > enabled inside the outermost critical section.
> > 
> > Also add the corresponding spin_lock primitives: spin_lock_irq_disable()
> > and spin_unlock_irq_enable(), as a result, code as follows:
> > 
> > 	spin_lock_irq_disable(l1);
> > 	spin_lock_irq_disable(l2);
> > 	spin_unlock_irq_enable(l1);
> > 	// Interrupts are still disabled.
> > 	spin_unlock_irq_enable(l2);
> > 
> > doesn't have the issue that interrupts are accidentally enabled.
> > 
> > This also makes the wrapper of interrupt-disabling locks on Rust easier
> > to design.
> > 
> > Signed-off-by: Lyude Paul <[email protected]>
> > [boqun: Apply Peter's feedback and fix spell errors reported by Ingo]
> > Signed-off-by: Boqun Feng <[email protected]>
> > ---
> >   include/linux/interrupt_rc.h     | 82 ++++++++++++++++++++++++++++++++
> >   include/linux/preempt.h          |  4 ++
> >   include/linux/spinlock.h         | 23 +++++++++
> >   include/linux/spinlock_api_smp.h | 43 +++++++++++++++++
> >   include/linux/spinlock_api_up.h  | 15 ++++++
> >   include/linux/spinlock_rt.h      | 18 +++++++
> >   kernel/locking/spinlock.c        | 31 ++++++++++++
> >   kernel/softirq.c                 | 28 ++++++++++-
> >   8 files changed, 242 insertions(+), 2 deletions(-)
> >   create mode 100644 include/linux/interrupt_rc.h
> > 
> > diff --git a/include/linux/interrupt_rc.h b/include/linux/interrupt_rc.h
> > new file mode 100644
> > index 000000000000..b9a7f05ecf42
> > --- /dev/null
> > +++ b/include/linux/interrupt_rc.h
> > @@ -0,0 +1,82 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __LINUX_INTERRUPT_RC_H
> > +#define __LINUX_INTERRUPT_RC_H
> > +
> > +/*
> > + * include/linux/interrupt_rc.h - refcounted local processor interrupt
> > + * management.
> > + *
> > + * Since the implementation of this API currently depends on
> > + * local_irq_save()/local_irq_restore(), we split this into its own header to
> > + * make it easier to include without hitting circular header dependencies.
> > + */
> > +
> > +#include <linux/irqflags.h>
> > +#include <linux/preempt.h>
> > +#include <linux/processor.h>
> > +#include <linux/smp.h>
> > +
> > +#ifndef MODULE
> > +/* Per-CPU interrupt disabling state for local_interrupt_{disable,enable}(). */
> > +DECLARE_PER_CPU(unsigned long, local_interrupt_disable_state);
> > +
> > +static __always_inline void __local_interrupt_disable(void)
> > +{
> > +	unsigned long flags;
> > +
> > +	local_irq_save(flags);
> > +	raw_cpu_write(local_interrupt_disable_state, flags);
> > +}
> > +
> > +static __always_inline void __local_interrupt_enable(void)
> > +{
> > +	unsigned long flags = raw_cpu_read(local_interrupt_disable_state);
> > +
> > +	local_irq_restore(flags);
> > +}
> > +
> > +#ifndef INSTANTIATE_EXPORTED_INTERRUPT_DISABLE
> > +static __always_inline void _local_interrupt_disable(void)
> > +{
> > +	__local_interrupt_disable();
> > +}
> > +
> > +static __always_inline void _local_interrupt_enable(void)
> > +{
> > +	__local_interrupt_enable();
> > +}
> > +#else
> > +extern void _local_interrupt_disable(void);
> > +extern void _local_interrupt_enable(void);
> > +#endif
> > +
> > +#else /* !MODULE */
> > +extern void _local_interrupt_disable(void);
> > +extern void _local_interrupt_enable(void);
> > +#endif /* !MODULE */
> > +
> > +static inline void local_interrupt_disable(void)
> > +{
> > +	int new_count;
> > +
> > +	WARN_ON_ONCE(in_nmi());
> > +
> > +	new_count = hardirq_disable_enter();
> > +
> > +	/* Interrupts can happen here, but it's OK, see __irq_exit_rcu(). */
> > +
> > +	if ((new_count & HARDIRQ_DISABLE_MASK) == HARDIRQ_DISABLE_OFFSET)
> > +		_local_interrupt_disable();
> > +}
> 
> Maximum nesting possible is 256 right? Whats is stopping here to do more than that?

Yes. Currently similar as softirq, we don't detect the overflow.

> Should there be a warn_on?

A simple warn_on could be problematic because warn_on() itself may take
an irq-disabling lock, and that may trigger another overflow on top of
the existing overflow. It's a bit tricky to do a proper detection. But
I'm open to ideas.

Regards,
Boqun

> 
> > +
> > +static inline void local_interrupt_enable(void)
> > +{
> > +	int new_count;
> > +
> > +	new_count = hardirq_disable_exit();
> > +
> > +	if ((new_count & HARDIRQ_DISABLE_MASK) == 0)
> > +		_local_interrupt_enable();
> > +}
> > +
> > +#endif /* !__LINUX_INTERRUPT_RC_H */