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

[Boqun Feng <boqun@kernel.org>]

On Thu, Aug 27, 2026 at 08:15:44PM +0200, Thomas Gleixner wrote:
> On Thu, Aug 27 2026 at 09:52, Boqun Feng wrote:
> > On Thu, Aug 27, 2026 at 05:43:26PM +0200, Thomas Gleixner wrote:
> >> 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/
> 
> Yes. I gave you the wrong order, but I expected you to actually think it
> through and not blindly copy it. :)
> 

No, not blaming you :) I was just providing a bit more context.

I did think through a few parts to make it work, but TBH I lack of the
sensitivity for the impact that no interrupt happen on one CPU for a
while, so I didn't think this part very seriously. And I just liked the
idea we could skip disabling IRQ if possible.

> > but looks like we cannot do it without the fixups you mention below.
> 
> But that does not mean it can't be done. Checking for 0 first and
> incrementing after the actual disable is still achieving the same result
> of touching the CPU only once, no?
> 

Yeah, that should work. But I need to think a bit hard on this.

> > For now I will reverse the order and remove the additional checking in
> > softirq to fix the softirq pending issue.
> 
> That "fixes" another nasty bug which was latent for weeks and people
> could not get a handle on it because it was absolutely not
> reproducible. Given all that I'm absolutely not convinced that there
> isn't another pile of latent surprises lurking.
> 
> Aside of that I'm worried about having this new counter exposed in the
> current state of affairs. Nothing prevents arbitrary code from using
> hardirq_disable_count(), which is definitely faster than
> irqs_disabled(), but returns a random value depending on context. That's

Random how? Are you saying in the current (wrong) order? Because after
reversing the order, hardirq_disable_count() != 0 means the interrupt
has been disabled, no?

But I checked, actually with the reverse order, we don't need
hardirq_disable_count(), so we can remove it entirely. Will send a
follow up patch on this.

> just another recipe for latent and hard to debug disasters to happen as
> you already demonstrated in __irq_exit_rcu().
> 
> It's not the end of the world to bite the bullet and undo the whole
> pile, except for the then unused expansion of preempt count, go back to
> the drawing board and come up with a consistent and better overall
> solution.
> 
> I know that hurts, I've been there myself more than once. But at the end
> I was always happy that we decided to rip it out instead of trying to
> debug and duct tape it to death.
> 
> A inconsistent and fragile facility is worse than having none.
> 

To be honest, it doesn't hurt myself if we have to redo the work, I
would always like to do it correct. So I don't mind doing that. But it
might hurt others who want to develop real drivers with Rust because no
SpinLockIrq for them until the redo finishes. That's the major reason
that I would like to keep local_interrupt_disable() and
spin_lock_irq_disable().

(I also feel like with the order fix and hardirq_disable_count() remove,
the design is robust enough to exist and evolve, but I may miss
something subtle?)

Alternatively, we can move the current API to be Rust use only (we can
make the implementation in Rust even, if we maintain the state and
counter in Rust) in this way, there is only a limit set interactions
from the new things with the existing kernel, and Rust can always make
the guard work properly.

But honestly, it'll be just duplicating what we already have here to the
Rust side. So it's not my own desire that I want to keep the current
things in tree, it's more that I also look at this from a different
angle, and it make some sense engineer-wise: the semantics of
local_interrupt_disable() is so easy and straightforward that I feel
it's unfair to block the potential user especially when the users can
guarantee the correct usages with the type system. 

Anyway, that's just my two cents.

Regards,
Boqun

> Thanks,
> 
>         tglx
> 
> 
> 
> 
> 
> 
>