Re: [PATCH] interrupt: Disable interrupt before modifying hardirq_disable counter

[Bradley Morgan]

On 27 August 2026 19:10:48 BST, Boqun Feng <boqun@xxxxxxxxxx> wrote:
>Currently a softirq may be pending longer then expected if the
>triggering interrupt happens in-between hardirq_disable_enter() and
>_local_interrupt_disable() in local_interrupt_disable():
>
>    local_interrupt_disable():
>      hardirq_disable_enter();
>      <interrupt>
>      ...
>      __irq_exit_rcu():
>        // false because hardirq_disable_count() is not 0
>        if (.. && !hardirq_disable_count() && ..) {
>	  invoke_softirq();
>	}
>      _local_interrupt_disable();
>
>, it'll defer the softirq to the next interrupt which can be forever.
>
>The order between hardirq_disable_enter() and _local_interrupt_disable()
>is to optimize re-disabling interrupts if they are already disabled, but
>as 1) local_interrupt_disable() is not widely used yet and 2) the proper
>way to achieve this optimization may need fixing up the counter at
>entry/exit time [1], so reverse the order for now to avoid the softirq
>pending issue.
>
>Since we are doing this, the part of saving the current state is
>separated from irq disabling, and we basically do the following in
>local_interrupt_disable():
>
>    local_irq_save(flags);
>    if (counter++ == 0) {
>      this_cpu(local_interrupt_disable_state) = flags;
>    }
>
>Change _local_interrupt_disable() to _local_interrupt_save_state().
>
>Link: https://lore.kernel.org/lkml/87v78wezid.ffs@fw13/ [1]
>Reported-by: Thomas Gleixner <tglx@xxxxxxxxxx>
>Fixes: e901c1510e24 ("irq,spin_lock: Add counted interrupt disabling/enabling")

LGTM, thanks 

Reviewed-by: Bradley Morgan <brads@xxxxxxxxxxxxxx>


>Signed-off-by: Boqun Feng <boqun@xxxxxxxxxx>
>---
> include/linux/interrupt_rc.h | 19 ++++++++-----------
> kernel/softirq.c             | 17 ++++-------------
> 2 files changed, 12 insertions(+), 24 deletions(-)
>
>diff --git a/include/linux/interrupt_rc.h b/include/linux/interrupt_rc.h
>index b9a7f05ecf42..e68e1bedba66 100644
>--- a/include/linux/interrupt_rc.h
>+++ b/include/linux/interrupt_rc.h
>@@ -20,11 +20,8 @@
> /* 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)
>+static __always_inline void __local_interrupt_save_state(unsigned long flags)
> {
>-	unsigned long flags;
>-
>-	local_irq_save(flags);
> 	raw_cpu_write(local_interrupt_disable_state, flags);
> }
> 
>@@ -36,9 +33,9 @@ static __always_inline void __local_interrupt_enable(void)
> }
> 
> #ifndef INSTANTIATE_EXPORTED_INTERRUPT_DISABLE
>-static __always_inline void _local_interrupt_disable(void)
>+static __always_inline void _local_interrupt_save_state(unsigned long flags)
> {
>-	__local_interrupt_disable();
>+	__local_interrupt_save_state(flags);
> }
> 
> static __always_inline void _local_interrupt_enable(void)
>@@ -46,27 +43,27 @@ static __always_inline void _local_interrupt_enable(void)
> 	__local_interrupt_enable();
> }
> #else
>-extern void _local_interrupt_disable(void);
>+extern void _local_interrupt_save_state(unsigned long flags);
> extern void _local_interrupt_enable(void);
> #endif
> 
> #else /* !MODULE */
>-extern void _local_interrupt_disable(void);
>+extern void _local_interrupt_save_state(unsigned long flags);
> extern void _local_interrupt_enable(void);
> #endif /* !MODULE */
> 
> static inline void local_interrupt_disable(void)
> {
> 	int new_count;
>+	unsigned long flags;
> 
> 	WARN_ON_ONCE(in_nmi());
> 
>+	local_irq_save(flags);
> 	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();
>+		_local_interrupt_save_state(flags);
> }
> 
> static inline void local_interrupt_enable(void)
>diff --git a/kernel/softirq.c b/kernel/softirq.c
>index 7980a4a232f9..5d02c36c40e3 100644
>--- a/kernel/softirq.c
>+++ b/kernel/softirq.c
>@@ -91,11 +91,11 @@ EXPORT_PER_CPU_SYMBOL_GPL(hardirq_context);
> 
> DEFINE_PER_CPU(unsigned long, local_interrupt_disable_state);
> 
>-void _local_interrupt_disable(void)
>+void _local_interrupt_save_state(unsigned long flags)
> {
>-	__local_interrupt_disable();
>+	__local_interrupt_save_state(flags);
> }
>-EXPORT_SYMBOL(_local_interrupt_disable);
>+EXPORT_SYMBOL(_local_interrupt_save_state);
> 
> void _local_interrupt_enable(void)
> {
>@@ -749,16 +749,7 @@ static inline void __irq_exit_rcu(void)
> #endif
> 	account_hardirq_exit(current);
> 	preempt_count_sub(HARDIRQ_OFFSET);
>-	/*
>-	 * Interrupts may happen between hardirq_disable_enter() and
>-	 * local_irq_save() in local_interrupt_disable(), if irq_exit() invokes
>-	 * softirq here, we may have a softirq handler calling
>-	 * local_interrupt_disable() but it won't disable the IRQ because
>-	 * hardirq disabling count is already 1, hence we need to prevent
>-	 * invoking softirq when a local_interrupt_disable() is ongoing.
>-	 */
>-	if (!in_interrupt() && !hardirq_disable_count() &&
>-	    local_softirq_pending()) {
>+	if (!in_interrupt() && local_softirq_pending()) {
> 		/*
> 		 * If we left hrtimers unarmed, make sure to arm them now,
> 		 * before enabling interrupts to run softirq.
>

--- Thanks!
https://lore.kernel.org/all/EE579805-42F2-4C58-B752-F28779EEB717@xxxxxxxxx/