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

On Mon, Aug 03, 2026 at 09:09:41PM +0200, Ingo Molnar wrote:
> 
> * Boqun Feng <[email protected]> wrote:
> 
> > diff --git a/include/linux/interrupt_rc.h b/include/linux/interrupt_rc.h
> > new file mode 100644
> > index 000000000000..dd4444c61330
> > --- /dev/null
> > +++ b/include/linux/interrupt_rc.h
> > @@ -0,0 +1,67 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * include/linux/interrupt_rc.h - refcounted local processor interrupt
> > + * management.
> > + *
> > + * Since the implementation of this API currently depends on
> > + * local_irq_save()/local_irq_restore(), we split this into it's own header to
> > + * make it easier to include without hitting circular header dependencies.
> > + */
> > +
> > +#ifndef __LINUX_INTERRUPT_RC_H
> > +#define __LINUX_INTERRUPT_RC_H
> 
> That's not the customary placement for header guards, please try to
> follow existing patterns as much as possible.
> 

Fixed locally, I follow what spinlock.h does.

> > +/* Per-cpu interrupt disabling state for local_interrupt_{disable,enable}() */
> 
> That's not the standard spelling of per-CPU.
> 
> > +struct interrupt_disable_state {
> > +	unsigned long flags;
> > +};
> 
> Unless this structure is extended in the same series - and it isn't
> AFICT, why this layer of obfuscation of type and purpose?
> 

Will remove this layer in the next version.

> > +		/*
> > +		 * 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();
> > +#endif
> 
> Let's not add TODOs and call it an upstream-ready patch ...
> 

Per discussion with Peter [1], this part is not needed, so the TODO will
be removed.

>   	preempt_count_sub(HARDIRQ_OFFSET);
> > -	if (!in_interrupt() && local_softirq_pending()) {
> > +	/*
> > +	 * Interrupts may happen between hardirq_disable_enter() and
> > +	 * local_irq_save() in local_interrupt_disable(), if irq_exit() invokes
> > +	 * softirq here, we may have a softirq handler calling
> > +	 * local_interrupt_disable() but it won't disable the irq because
> > +	 * hardirq disabling count is already 1, hence we need to prevent
> > +	 * invoking softirq when a local_interrupt_disable() is ongoing.
> 
> Non-standard spelling of 'IRQ'.
> 
> > +	 */
> > +	if (!in_interrupt() && !hardirq_disable_count() &&
> > +	    local_softirq_pending()) {
> >  		/*
> >  		 * If we left hrtimers unarmed, make sure to arm them now,
> >  		 * before enabling interrupts to run SoftIRQ.
> 
> Non-standard spelling of 'softirq' here.
> 

Fixed.

> There's also new spelling errors in some of the comments introduced
> by this patch.
> 

Will try to find and fix these as well.

[1]: https://lore.kernel.org/rust-for-linux/[email protected]/

Regards,
Boqun

> This is a very low quality patch, it should never have been included
> and declared 'ready' for upstream. It's not even close!
> 
> Thanks,
> 
> 	Ingo