Re: [PATCH] locking: Revert switching guards to _irq_{disable,enable}()

[Boqun Feng <boqun@kernel.org>]

On Thu, Aug 27, 2026 at 05:43:26PM +0200, Thomas Gleixner wrote:
> On Thu, Aug 27 2026 at 06:14, Boqun Feng wrote:
> > On Thu, Aug 27, 2026 at 10:30:50AM +0200, Thomas Gleixner wrote:
> >> The main problem is that you cover only half of it and there are
> >> completely correct cases where this simply blows up in your face:
> >> 
> >>   local_irq_disable();                          // does not affect CNT
> >>   ....
> >>   guard(raw_spinlock)(&l1);                     // does not affect CNT
> >>      foo()
> >>        guard(raw_spinlock_irqsave)(&l2);        // observes CNT = 0
> >> 
> >> so the unlocking of &l2 will enable interrupts prematurely.
> >> 
> >
> > If we are talking the switch in this patch, then no, the unlocking
> > of &l2 will NOT enable interrupts prematurely. The above code expands as
> > the following (using pseudo code to describe how
> > raw_spin_lock_irq_disable(), raw_spin_lock_irq_enable(),
> > local_interrupt_disable(), and local_interrupt_enable() work)
> >
> >    local_irq_disable();                          // does not affect CNT
> >    ....
> >    guard(raw_spinlock)(&l1);                     // does not affect CNT
> >       foo()
> >         guard(raw_spinlock_irqsave)(&l2): 
> >           raw_spin_lock_irq_disable():
> >             local_interrupt_disable():
> > 	      CNT++;
> >               this_cpu(state) = local_irq_save(); // record the current state
> >             raw_spin_lock(&l2);
> > 	  ...
> >           raw_spin_lock_irq_enable():
> >             raw_spin_unlock(&l2);
> >             local_interrupt_enable():
> >               CNT--;
> >               if (CNT == 0)
> >                 local_irq_restore(this_cpu(state)); // recover the previous state
> >
> > So local_interrupt_disable() and local_interrupt_enable() only recover
> > to the previous state, as a result it'll not enable interrupt
> > prematurely here. In other words, the following code works:
> >
> >     local_irq_disable();
> >     local_interrupt_disable();
> >     local_interrupt_enable();  // interrupt is not re-enabled here
> >                                // similar to how preempt_disable() does
> > 			       // in a nested preemption disable
> > 			       // critical section.
> >     local_irq_enable();
> 
> Fair enough. I misread that part.
> 
> But my main observation that the counter is inconsistent still stands
> and I think that's a fundamental flaw because there is no way that code
> can rely on that counter until everything has been converted over and
> the interrupt/exception/nmi/syscall entry/exit code has been fixed up.
> 
> Just let me look at local_interrupt_disable() and __irq_exit_rcu()
> again.
> 
> local_interrupt_disable()
>    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();
> 
> This is absolutely not ok. Why?
> 
> The counter is incremented _before_ interrupts are actually
> disabled. Now in __irq_exit_rcu():
> 
>         if (!in_interrupt() && !hardirq_disable_count() &&
>             local_softirq_pending()) {
> 
> which prevents soft interrupt handling in a completely legitimate
> situation. As a consequence _nothing_ will handle the pending soft
> interrupt until:
> 
>      - an interrupt coming in which observes consistent state
> 
>      - a local_bh_enable() processes them
> 
> That explains that recently quite a few more spurious 'local soft irq
> pending' printk's have been observed by people as there is no guarantee
> that either one of those events happens _before_ a CPU reaches
> idle. Even in the non-idle case deferring this to the next 'by chance'
> handling is fundamentally broken. This needs to be removed ASAP.
> 
> But coming back to the problem underneath. The ordering in
> local_interrupt_disable() is simply wrong. You need to disable first and
> then update the counter. Reverse order for enable() obviously update
> counter and enable, which you got right.
> 

Noted, the reason that I used the current order is to optimize
local_interrupt_disable() from re-disabling interrupt every time:

	https://lore.kernel.org/rust-for-linux/87a5eu7gvw.ffs@tglx/

but looks like we cannot do it without the fixups you mention below.
For now I will reverse the order and remove the additional checking in
softirq to fix the softirq pending issue.

Regards,
Boqun

> And to make stuff work correctly you need the fixups I pointed out in my
> previous reply to the various entry/exit functions. It's exactly the
> same problem as we handle in interrupt/exception/nmi entry/exit code
> vs. RCU, lockdep, tracing etc.
> 
> So if disable() does:
> 
>    if (!count)
>    	arch_local_irq_disable();
>    count++;
> 
> then an interrupt hitting before arch_local_irq_disable() will always
> observe the correct state. After that it can't be delivered.
> 
[...]