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

[Thomas Gleixner]

On Thu, Aug 27 2026 at 12:41, Boqun Feng wrote:
> On Thu, Aug 27, 2026 at 08:15:44PM +0200, Thomas Gleixner wrote:
>> > 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.

The point is that the counter is only valid when used within the limits
of the current coverage. Other than that it is not:

        spin_lock_irq() // or any other non-covered mechanism
            // observes 0
            cnt = preempt_count() & HARDIRQ_DISABLE_MASK;

That's inconsistent and therefore it is a random number, no?

You have no way to prevent that this happens and if it does it becomes a
nightmare to debug for everyone. Guess who got the bug reports about
preemption counter issues and local softirq pending messages in his
inbox and dealt with them.

There is a world outside of your safe rust zone and that needs to be
safe too. This half finished attempt to make Rust work is absolutely
not and I have zero interrest to deal with the fallout.

It's not safe and no extra hacks will make it safe. Which means it is
not ready. So the only sensible thing is to revert everything which
touches that section of preempt_count() and provides interfaces.

As this annoyed me, I rumaged through my poison cabinet and found the
old patches again. They obviously don't apply anymore but I found the
hints which corners need some care. With the generic entry code that
also got way simpler.

So I sat down and reverted

  1b0866874833 ("locking: Switch to _irq_{disable,enable}() variants in cleanup guards")
  e901c1510e24 ("irq,spin_lock: Add counted interrupt disabling/enabling")

and then hacked it up just to see how far I get before vanishing to bed.

Three hours later it surprisingly booted right away into a full distro
kernel and survived kernel builds and a few test cases. :)

Obviously I did not do any serious testing on it, but I wanted to share
it as a starting point and food for thoughts.

Yes, it needs to be enabled per architecture as the preempt counter
initialization is architecture specific and it requires generic entry
code. But those are not uncommon prerequisites and an incentive for
architecture people to get their act together.

But it is fully consistent and the fully refcounted thing can be
built on top of it. If you look carefuly you'll notice that
__raw_local_irq_disable/enable() are just optimized versions of
__raw_local_irq_save/restore() as they don't have the conditionals, so
they can be unified completely at least for debug builds or in general
when it turns out that the overhead is neglible.

There is a wide range of optimizations possible with that especially by
combining preempt/interrupt modifications into one operation and
rescheduling without changing the preemption counter in the first
place. Which is what I hinted to in the mail you linked earlier. I'm so
tempted to hack that up tomorrow once my brain is less fried than now
and after I exposed it to some serious testing.

Thanks,

        tglx
---
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -318,6 +318,7 @@ config X86
 	select PCI_DOMAINS			if PCI
 	select PCI_LOCKLESS_CONFIG		if PCI
 	select PERF_EVENTS
+	select PREEMPT_COUNT_IRQFLAGS
 	select RTC_LIB
 	select RTC_MC146818_LIB
 	select SPARSE_IRQ
--- a/arch/x86/include/asm/preempt.h
+++ b/arch/x86/include/asm/preempt.h
@@ -61,8 +61,8 @@ static __always_inline void preempt_coun
  */
 #define init_task_preempt_count(p) do { } while (0)
 
-#define init_idle_preempt_count(p, cpu) do { \
-	per_cpu(__preempt_count, (cpu)) = PREEMPT_DISABLED; \
+#define init_idle_preempt_count(p, cpu) do {						\
+	per_cpu(__preempt_count, (cpu)) = PREEMPT_DISABLED | HARDIRQ_DISABLE_OFFSET;	\
 } while (0)
 
 /*
--- a/include/linux/irq-entry-common.h
+++ b/include/linux/irq-entry-common.h
@@ -97,6 +97,7 @@ static __always_inline bool arch_in_rcu_
  */
 static __always_inline void enter_from_user_mode(struct pt_regs *regs)
 {
+	__preempt_count_inc_hardirqs_disable();
 	arch_enter_from_user_mode(regs);
 	lockdep_hardirqs_off(CALLER_ADDR0);
 
@@ -275,6 +276,7 @@ static __always_inline void exit_to_user
 	user_enter_irqoff();
 	arch_exit_to_user_mode();
 	lockdep_hardirqs_on(CALLER_ADDR0);
+	__preempt_count_dec_hardirqs_disable();
 }
 
 /**
@@ -385,6 +387,8 @@ static __always_inline irqentry_state_t
 		.exit_rcu = false,
 	};
 
+	__preempt_count_inc_hardirqs_disable();
+
 	/*
 	 * If this entry hit the idle task invoke ct_irq_enter() whether
 	 * RCU is watching or not.
@@ -498,6 +502,7 @@ irqentry_exit_to_kernel_mode_after_preem
 			instrumentation_end();
 			ct_irq_exit();
 			lockdep_hardirqs_on(CALLER_ADDR0);
+			__preempt_count_dec_hardirqs_disable();
 			return;
 		}
 
@@ -514,6 +519,7 @@ irqentry_exit_to_kernel_mode_after_preem
 		if (state.exit_rcu)
 			ct_irq_exit();
 	}
+	__preempt_count_dec_hardirqs_disable();
 }
 
 /**
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -13,6 +13,7 @@
 #define _LINUX_TRACE_IRQFLAGS_H
 
 #include <linux/irqflags_types.h>
+#include <linux/preempt.h>
 #include <linux/typecheck.h>
 #include <linux/cleanup.h>
 #include <asm/irqflags.h>
@@ -165,31 +166,124 @@ extern void warn_bogus_irq_restore(void)
 /*
  * Wrap the arch provided IRQ routines to provide appropriate checks.
  */
-#define raw_local_irq_disable()		arch_local_irq_disable()
-#define raw_local_irq_enable()		arch_local_irq_enable()
+#ifdef CONFIG_PREEMPT_COUNT_IRQFLAGS
+static __always_inline void raw_local_irq_disable(void)
+{
+	arch_local_irq_disable();
+	preempt_count_add(HARDIRQ_DISABLE_OFFSET);
+}
+
+static __always_inline void raw_local_irq_enable(void)
+{
+	preempt_count_sub(HARDIRQ_DISABLE_OFFSET);
+	arch_local_irq_enable();
+}
+
+static __always_inline unsigned long __raw_local_irq_save(void)
+{
+	unsigned long cnt = preempt_count();
+
+	if (!(cnt & HARDIRQ_DISABLE_MASK))
+		arch_local_irq_disable();
+	preempt_count_add(HARDIRQ_DISABLE_OFFSET);
+
+	// Probably not even needed unless something feeds 'flags' into
+	// irqs_disabled_flags()
+	return cnt;
+}
+
+static __always_inline void __raw_local_irq_restore(unsigned long cnt)
+{
+	if (!(__preempt_count_sub_return(HARDIRQ_DISABLE_OFFSET) & HARDIRQ_DISABLE_MASK))
+		arch_local_irq_enable();
+}
+
+static __always_inline unsigned long __raw_local_save_flags(void)
+{
+	return preempt_count() & HARDIRQ_DISABLE_MASK;
+}
+
+static __always_inline bool __raw_irqs_disabled_flags(unsigned long cnt)
+{
+	return !!cnt;
+}
+
+static __always_inline bool raw_irqs_disabled(void)
+{
+	return preempt_count() & HARDIRQ_DISABLE_MASK;
+}
+
+static __always_inline void raw_safe_halt(void)
+{
+	preempt_count_sub(HARDIRQ_DISABLE_OFFSET);
+	arch_safe_halt();
+}
+
+#else
+
+static __always_inline void raw_local_irq_disable(void)
+{
+	arch_local_irq_disable();
+}
+
+static __always_inline void raw_local_irq_enable(void)
+{
+	arch_local_irq_enable();
+}
+
+static __always_inline unsigned long __raw_local_irq_save(void)
+{
+	return arch_local_irq_save();
+}
+
+static __always_inline void __raw_local_irq_restore(unsigned long flags)
+{
+	arch_local_irq_restore(flags);
+}
+
+static __always_inline unsigned long __raw_local_save_flags(void)
+{
+	return arch_local_save_flags();
+}
+
+static __always_inline bool __raw_irqs_disabled_flags(unsigned long flags)
+{
+	return arch_irqs_disabled_flags(flags);
+}
+
+static __always_inline bool raw_irqs_disabled(void)
+{
+	return arch_irqs_disabled();
+}
+
+static __always_inline void raw_safe_halt(void)
+{
+	arch_safe_halt();
+}
+
+#endif
+
 #define raw_local_irq_save(flags)			\
 	do {						\
 		typecheck(unsigned long, flags);	\
-		flags = arch_local_irq_save();		\
+		flags = __raw_local_irq_save();		\
 	} while (0)
 #define raw_local_irq_restore(flags)			\
 	do {						\
 		typecheck(unsigned long, flags);	\
 		raw_check_bogus_irq_restore();		\
-		arch_local_irq_restore(flags);		\
+		__raw_local_irq_restore(flags);	\
 	} while (0)
 #define raw_local_save_flags(flags)			\
 	do {						\
 		typecheck(unsigned long, flags);	\
-		flags = arch_local_save_flags();	\
+		flags = __raw_local_save_flags();	\
 	} while (0)
 #define raw_irqs_disabled_flags(flags)			\
 	({						\
 		typecheck(unsigned long, flags);	\
-		arch_irqs_disabled_flags(flags);	\
+		__raw_irqs_disabled_flags(flags);	\
 	})
-#define raw_irqs_disabled()		(arch_irqs_disabled())
-#define raw_safe_halt()			arch_safe_halt()
 
 /*
  * The local_irq_*() APIs are equal to the raw_local_irq*()
--- a/include/linux/preempt.h
+++ b/include/linux/preempt.h
@@ -54,31 +54,31 @@
  *             NMI_MASK:	0xf0000000
  * (PREEMPT_NEED_RESCHED is in a different word)
  */
-#define PREEMPT_BITS	8
-#define SOFTIRQ_BITS	8
+#define PREEMPT_BITS		8
+#define SOFTIRQ_BITS		8
 #define HARDIRQ_DISABLE_BITS	8
-#define HARDIRQ_BITS	4
-#define NMI_BITS	(1 + 3*IS_ENABLED(CONFIG_HAS_SEPARATE_PREEMPT_RESCHED_BITS))
+#define HARDIRQ_BITS		4
+#define NMI_BITS		(1 + 3*IS_ENABLED(CONFIG_HAS_SEPARATE_PREEMPT_RESCHED_BITS))
 
-#define PREEMPT_SHIFT	0
-#define SOFTIRQ_SHIFT	(PREEMPT_SHIFT + PREEMPT_BITS)
+#define PREEMPT_SHIFT		0
+#define SOFTIRQ_SHIFT		(PREEMPT_SHIFT + PREEMPT_BITS)
 #define HARDIRQ_DISABLE_SHIFT	(SOFTIRQ_SHIFT + SOFTIRQ_BITS)
-#define HARDIRQ_SHIFT	(HARDIRQ_DISABLE_SHIFT + HARDIRQ_DISABLE_BITS)
-#define NMI_SHIFT	(HARDIRQ_SHIFT + HARDIRQ_BITS)
+#define HARDIRQ_SHIFT		(HARDIRQ_DISABLE_SHIFT + HARDIRQ_DISABLE_BITS)
+#define NMI_SHIFT		(HARDIRQ_SHIFT + HARDIRQ_BITS)
 
-#define __IRQ_MASK(x)	((1UL << (x))-1)
+#define __IRQ_MASK(x)		((1UL << (x))-1)
 
-#define PREEMPT_MASK	(__IRQ_MASK(PREEMPT_BITS) << PREEMPT_SHIFT)
-#define SOFTIRQ_MASK	(__IRQ_MASK(SOFTIRQ_BITS) << SOFTIRQ_SHIFT)
+#define PREEMPT_MASK		(__IRQ_MASK(PREEMPT_BITS) << PREEMPT_SHIFT)
+#define SOFTIRQ_MASK		(__IRQ_MASK(SOFTIRQ_BITS) << SOFTIRQ_SHIFT)
 #define HARDIRQ_DISABLE_MASK	(__IRQ_MASK(HARDIRQ_DISABLE_BITS) << HARDIRQ_DISABLE_SHIFT)
-#define HARDIRQ_MASK	(__IRQ_MASK(HARDIRQ_BITS) << HARDIRQ_SHIFT)
-#define NMI_MASK	(__IRQ_MASK(NMI_BITS)     << NMI_SHIFT)
+#define HARDIRQ_MASK		(__IRQ_MASK(HARDIRQ_BITS) << HARDIRQ_SHIFT)
+#define NMI_MASK		(__IRQ_MASK(NMI_BITS)     << NMI_SHIFT)
 
-#define PREEMPT_OFFSET	(1UL << PREEMPT_SHIFT)
-#define SOFTIRQ_OFFSET	(1UL << SOFTIRQ_SHIFT)
+#define PREEMPT_OFFSET		(1UL << PREEMPT_SHIFT)
+#define SOFTIRQ_OFFSET		(1UL << SOFTIRQ_SHIFT)
 #define HARDIRQ_DISABLE_OFFSET	(1UL << HARDIRQ_DISABLE_SHIFT)
-#define HARDIRQ_OFFSET	(1UL << HARDIRQ_SHIFT)
-#define NMI_OFFSET	(1UL << NMI_SHIFT)
+#define HARDIRQ_OFFSET		(1UL << HARDIRQ_SHIFT)
+#define NMI_OFFSET		(1UL << NMI_SHIFT)
 
 #define SOFTIRQ_DISABLE_OFFSET	(2 * SOFTIRQ_OFFSET)
 
@@ -90,7 +90,11 @@
  *
  * Reset by start_kernel()->sched_init()->init_idle()->init_idle_preempt_count().
  */
+#ifdef CONFIG_PREEMPT_COUNT_IRQFLAGS
+#define INIT_PREEMPT_COUNT	(PREEMPT_OFFSET + HARDIRQ_DISABLE_OFFSET)
+#else
 #define INIT_PREEMPT_COUNT	PREEMPT_OFFSET
+#endif
 
 /*
  * Initial preempt_count value; reflects the preempt_count schedule invariant
@@ -322,6 +326,21 @@ do { \
 
 #endif /* CONFIG_PREEMPT_COUNT */
 
+#ifdef CONFIG_PREEMPT_COUNT_IRQFLAGS
+static __always_inline void __preempt_count_inc_hardirqs_disable(void)
+{
+	__preempt_count_add(HARDIRQ_DISABLE_OFFSET);
+}
+
+static __always_inline void __preempt_count_dec_hardirqs_disable(void)
+{
+	__preempt_count_sub(HARDIRQ_DISABLE_OFFSET);
+}
+#else
+static __always_inline void __preempt_count_inc_hardirqs_disable(void) { }
+static __always_inline void __preempt_count_dec_hardirqs_disable(void) { }
+#endif
+
 #ifdef MODULE
 /*
  * Modules have no business playing preemption tricks.
--- a/kernel/Kconfig.preempt
+++ b/kernel/Kconfig.preempt
@@ -152,6 +152,9 @@ config PREEMPT_DYNAMIC
 	  Interesting if you want the same pre-built kernel should be used for
 	  both Server and Desktop workloads.
 
+config PREEMPT_COUNT_IRQFLAGS
+	bool
+
 config SCHED_CORE
 	bool "Core Scheduling for SMT"
 	depends on SCHED_SMT
--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -171,6 +171,7 @@ irqentry_state_t noinstr irqentry_nmi_en
 {
 	irqentry_state_t irq_state;
 
+	__preempt_count_inc_hardirqs_disable();
 	irq_state.lockdep = lockdep_hardirqs_enabled();
 
 	__nmi_enter();
@@ -202,4 +203,5 @@ void noinstr irqentry_nmi_exit(struct pt
 	if (irq_state.lockdep)
 		lockdep_hardirqs_on(CALLER_ADDR0);
 	__nmi_exit();
+	__preempt_count_dec_hardirqs_disable();
 }