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

[Shrikanth Hegde]

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 <lyude@xxxxxxxxxx>
> [boqun: Apply Peter's feedback and fix spell errors reported by Ingo]
> Signed-off-by: Boqun Feng <boqun@xxxxxxxxxx>
> ---
>   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?
Should there be a warn_on?

> +
> +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 */