Re: [PATCH 05/24] irq & spin_lock: Add counted interrupt disabling/enabling

On Mon, Aug 03, 2026 at 12:21:55PM +0200, Peter Zijlstra wrote:
> On Fri, Jul 31, 2026 at 01:30:06PM -0700, Boqun Feng wrote:
> > +static inline void local_interrupt_disable(void)
> > +{
> > +	unsigned long flags;
> > +	int new_count;
> > +
> > +	WARN_ON_ONCE(in_nmi());
> > +
> > +	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_irq_save(flags);
> > +		raw_cpu_write(local_interrupt_disable_state.flags, flags);
> > +	}
> > +}
> > +
> > +static inline void local_interrupt_enable(void)
> > +{
> > +	int new_count;
> > +
> > +	new_count = hardirq_disable_exit();
> > +
> > +	if ((new_count & HARDIRQ_DISABLE_MASK) == 0) {
> > +		unsigned long flags;
> > +
> > +		flags = raw_cpu_read(local_interrupt_disable_state.flags);
> > +		local_irq_restore(flags);
> > +		/*
> > +		 * TODO: re-read preempt count can be avoided, but it needs
> > +		 * should_resched() taking another parameter as the current
> > +		 * preempt count
> > +		 */
> > +#ifdef CONFIG_PREEMPTION
> > +		if (should_resched(0))
> > +			__preempt_schedule();
> 
> I'm confused by this one. This should not be needed. If there was a
> preemption while IRQs were disabled, there should be a pending
> IPI.
> 
> Notably local_irq_restore() also doesn't do this, since otherwise you
> would not have needed to add this here, since you already call that.
> 

I should probably have made a comment on this instead of keeping it in
my head ;-)

Note that since we use preempt_count to record irq disabling, then in
the spinlock version i.e. spin_{,un}lock_irq_{disable,enable}(), we
don't need (and don't have) preempt_disable/enable() any more. So we
need to rely on this for the following case:

	spin_lock_irq_disable();
	<set NEED_RESCHED for the current task>
	spin_unlock_irq_enable():
	  do_raw_spin_unlock(lock);
	  local_interrupt_enable(); // <- need this to trigger a
	                            // reschedule, because no
				    // preempt_enable here.

Make sense?

Regards,
Boqun

> > +#endif
> > +	}
> > +}