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

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