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

On Mon, Aug 03, 2026 at 12:43:21PM +0200, Peter Zijlstra wrote:
> On Mon, Aug 03, 2026 at 11:29:10AM +0200, Peter Zijlstra wrote:
> > On Fri, Jul 31, 2026 at 01:30:06PM -0700, Boqun Feng wrote:
> > > +++ b/include/linux/interrupt_rc.h
> > > @@ -0,0 +1,67 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * 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 it's own header to
> > > + * make it easier to include without hitting circular header dependencies.
> > > + */
> > > +
> > > +#ifndef __LINUX_INTERRUPT_RC_H
> > > +#define __LINUX_INTERRUPT_RC_H
> > > +
> > > +#include <linux/irqflags.h>
> > > +#include <asm/processor.h>
> > > +#ifdef CONFIG_SMP
> > > +#include <asm/smp.h>
> > > +#endif
> > > +
> > > +/* Per-cpu interrupt disabling state for local_interrupt_{disable,enable}() */
> > > +struct interrupt_disable_state {
> > > +	unsigned long flags;
> > > +};
> > > +
> > > +DECLARE_PER_CPU(struct interrupt_disable_state, local_interrupt_disable_state);
> > 
> > I'm a bit confused by the need for this struct; afaict there aren't any
> > additional members in the rest of the series. So at this point this is
> > pointless wrappery, no?
> > 
> > Having this variable exported is unfortunate.
> 
> One option to get rid of that EXPORT would be something like so I
> suppose.
> 
> ---
> --- a/include/linux/interrupt_rc.h
> +++ b/include/linux/interrupt_rc.h
> @@ -12,21 +12,52 @@
>  #define __LINUX_INTERRUPT_RC_H
>  
>  #include <linux/irqflags.h>
> +#include <linux/preempt.h>
>  #include <asm/processor.h>
>  #ifdef CONFIG_SMP
>  #include <asm/smp.h>
>  #endif
>  
> +#ifndef MODULE
>  /* Per-cpu interrupt disabling state for local_interrupt_{disable,enable}() */
> -struct interrupt_disable_state {
> +DECLARE_PER_CPU(unsigned long, local_interrupt_disable_state);
> +
> +static __always_inline void __local_interrupt_disable(void)
> +{
>  	unsigned long flags;
> -};
>  
> -DECLARE_PER_CPU(struct interrupt_disable_state, local_interrupt_disable_state);
> +	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);

I think the "extern" keywords are not necessary and ..

> +#endif /* !MODULE */
>  
>  static inline void local_interrupt_disable(void)
>  {
> -	unsigned long flags;
>  	unsigned long new_count;
>  
>  	WARN_ON_ONCE(in_nmi());
> @@ -35,10 +66,8 @@ static inline void local_interrupt_disab
>  
>  	/* Interrupts can happen here, but it's OK, see __irq_exit_rcu(). */
>  
> -	if ((new_count & HARDIRQ_DISABLE_MASK) == HARDIRQ_DISABLE_OFFSET) {
> -		local_irq_save(flags);
> -		raw_cpu_write(local_interrupt_disable_state.flags, flags);
> -	}
> +	if ((new_count & HARDIRQ_DISABLE_MASK) == HARDIRQ_DISABLE_OFFSET)
> +		_local_interrupt_disable();
>  }
>  
>  #ifdef CONFIG_PREEMPTION
> @@ -63,14 +92,8 @@ static inline void local_interrupt_enabl
>  
>  	new_count = hardirq_disable_exit();
>  
> -	if ((new_count & HARDIRQ_DISABLE_MASK) == 0) {
> -		unsigned long flags;
> -
> -		flags = raw_cpu_read(local_interrupt_disable_state.flags);
> -		local_irq_restore(flags);
> -
> -		local_interrupt_enable_reched(new_count);
> -	}
> +	if ((new_count & HARDIRQ_DISABLE_MASK) == 0)
> +		_local_interrupt_enable();
>  }
>  
>  #endif /* !__LINUX_INTERRUPT_RC_H */
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -9,6 +9,7 @@
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
> +#define INSTANTIATE_EXPORTED_INTERRUPT_DISABLE
>  #include <linux/export.h>
>  #include <linux/kernel_stat.h>
>  #include <linux/interrupt.h>
> @@ -88,8 +89,17 @@ EXPORT_PER_CPU_SYMBOL_GPL(hardirqs_enabl
>  EXPORT_PER_CPU_SYMBOL_GPL(hardirq_context);
>  #endif
>  
> -DEFINE_PER_CPU(struct interrupt_disable_state, local_interrupt_disable_state);
> -EXPORT_PER_CPU_SYMBOL_GPL(local_interrupt_disable_state);
> +DEFINE_PER_CPU(unsigned long, local_interrupt_disable_state);
> +
> +void _local_interrupt_disable(void)
> +{
> +	__local_interrupt_disable();
> +}
> +
> +void _local_interrupt_enable(void)
> +{
> +	__local_interrupt_enable();
> +}
>  

We need to EXPORT_SYMBOL_GPL() for these two functions.

Right?

Regards,
Boqun

>  #ifndef CONFIG_HAS_SEPARATE_PREEMPT_RESCHED_BITS
>  /*