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

On Mon, Aug 03, 2026 at 04:21:06PM +0200, Peter Zijlstra wrote:
> On Mon, Aug 03, 2026 at 07:06:54AM -0700, Boqun Feng wrote:
> > 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?
> 
> No; for one, you cannot elide the preempt_disable/enable() from this
> thing.
> 
> It has always been valid to do:
> 
> 	raw_spin_lock_irq(lock);
> 	...
> 	raw_spin_unlock(lock);
> 	...
> 	local_irq_enable();
> 
> And since raw_spin_unlock() does have the preemption thing,

Good point!

However, IIRC, the elision was trying to optimize two preempt_count
accesses (one for irq disable, one for preempt disable) into one in
raw_spin_lock_irq_disable() or raw_spin_unlock_irq_enable(). We may
still want to do it in the future, if that's the case, we will still
have should_resched() check in raw_spin_unlock_irq_enable()?

> raw_spin_lock_irq() must too. And the very same argument is still valid
> if you now write:
> 
> 	raw_spin_lock_irq_disable(lock);
> 	...
> 	raw_spin_unlock(lock);
> 	...
> 	local_interrupt_enable();
> 
> Secondly, nothing should 'set' NEED_RESCHED without also immediately
> causing a preemption.

Thanks for the information.

Regards,
Boqun