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

[Boqun Feng <boqun@kernel.org>]

On Fri, Aug 28, 2026 at 12:52:26AM +0200, Thomas Gleixner wrote:
> 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.
> 

My biggest concern is how you are going to handle the oddballs I
mentioned in another thread, especially when you need to fix the users.
Because this would lead to a all-or-nothing solution: unless we resolve
all the oddballs, we cannot enable this.

> 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.
> 

If you did, looking forwards to it, I can help enable that for other
architectures if needed.

Regards,
Boqun

> 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();
>  }