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

[Ingo Molnar <mingo@kernel.org>]

* Boqun Feng <boqun@kernel.org> 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.

> +/* 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?

> +		/*
> +		 * 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 ...

  	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.

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

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