Re: [PATCH] genirq: reduce irqdebug bouncing cachelines

[Cédric Le Goater]

Hello,

On 4/10/21 1:58 PM, Thomas Gleixner wrote:
> Nicholas,
> 
> On Fri, Apr 02 2021 at 23:20, Nicholas Piggin wrote:
>> note_interrupt increments desc->irq_count for each interrupt even for
>> percpu interrupt handlers, even when they are handled successfully. This
>> causes cacheline bouncing and limits scalability.
>>
>> Instead of incrementing irq_count every time, only start incrementing it
>> after seeing an unhandled irq, which should avoid the cache line
>> bouncing in the common path.
>>
>> This actually should give better consistency in handling misbehaving
>> irqs too, because instead of the first unhandled irq arriving at an
>> arbitrary point in the irq_count cycle, its arrival will begin the
>> irq_count cycle.
> 
> I've applied that because it makes sense in general, but I think the whole
> call to note_interrupt() can be avoided or return early when interrupts
> would be marked accordingly. For IPI handlers which always return
> HANDLED the whole procedure is pretty pointless to begin with.
> 
> Something like the completely untested below.
> 
> Thanks,
> 
>         tglx
> ---
>  include/linux/interrupt.h |    3 +++
>  include/linux/irq.h       |    2 ++
>  kernel/irq/manage.c       |    2 ++
>  kernel/irq/settings.h     |   12 ++++++++++++
>  kernel/irq/spurious.c     |    2 +-
>  5 files changed, 20 insertions(+), 1 deletion(-)
> 
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -64,6 +64,8 @@
>   * IRQF_NO_AUTOEN - Don't enable IRQ or NMI automatically when users request it.
>   *                Users will enable it explicitly by enable_irq() or enable_nmi()
>   *                later.
> + * IRQF_NO_DEBUG - Exclude from runnaway detection for IPI and similar handlers,
> + *		   depends on IRQF_PERCPU.
>   */
>  #define IRQF_SHARED		0x00000080
>  #define IRQF_PROBE_SHARED	0x00000100
> @@ -78,6 +80,7 @@
>  #define IRQF_EARLY_RESUME	0x00020000
>  #define IRQF_COND_SUSPEND	0x00040000
>  #define IRQF_NO_AUTOEN		0x00080000
> +#define IRQF_NO_DEBUG		0x00100000
>  
>  #define IRQF_TIMER		(__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD)
>  
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -72,6 +72,7 @@ enum irqchip_irq_state;
>   *				  mechanism and from core side polling.
>   * IRQ_DISABLE_UNLAZY		- Disable lazy irq disable
>   * IRQ_HIDDEN			- Don't show up in /proc/interrupts
> + * IRQ_NO_DEBUG			- Exclude from note_interrupt() debugging
>   */
>  enum {
>  	IRQ_TYPE_NONE		= 0x00000000,
> @@ -99,6 +100,7 @@ enum {
>  	IRQ_IS_POLLED		= (1 << 18),
>  	IRQ_DISABLE_UNLAZY	= (1 << 19),
>  	IRQ_HIDDEN		= (1 << 20),
> +	IRQ_NO_DEBUG		= (1 << 21),
>  };
>  
>  #define IRQF_MODIFY_MASK	\
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1682,6 +1682,8 @@ static int
>  		if (new->flags & IRQF_PERCPU) {
>  			irqd_set(&desc->irq_data, IRQD_PER_CPU);
>  			irq_settings_set_per_cpu(desc);
> +			if (new->flags & IRQF_NO_DEBUG)
> +				irq_settings_set_no_debug(desc);
>  		}
>  
>  		if (new->flags & IRQF_ONESHOT)
> --- a/kernel/irq/settings.h
> +++ b/kernel/irq/settings.h
> @@ -18,6 +18,7 @@ enum {
>  	_IRQ_IS_POLLED		= IRQ_IS_POLLED,
>  	_IRQ_DISABLE_UNLAZY	= IRQ_DISABLE_UNLAZY,
>  	_IRQ_HIDDEN		= IRQ_HIDDEN,
> +	_IRQ_NO_DEBUG		= IRQ_NO_DEBUG,
>  	_IRQF_MODIFY_MASK	= IRQF_MODIFY_MASK,
>  };
>  
> @@ -33,6 +34,7 @@ enum {
>  #define IRQ_IS_POLLED		GOT_YOU_MORON
>  #define IRQ_DISABLE_UNLAZY	GOT_YOU_MORON
>  #define IRQ_HIDDEN		GOT_YOU_MORON
> +#define IRQ_NO_DEBUG		GOT_YOU_MORON
>  #undef IRQF_MODIFY_MASK
>  #define IRQF_MODIFY_MASK	GOT_YOU_MORON
>  
> @@ -174,3 +176,13 @@ static inline bool irq_settings_is_hidde
>  {
>  	return desc->status_use_accessors & _IRQ_HIDDEN;
>  }
> +
> +static inline void irq_settings_set_no_debug(struct irq_desc *desc)
> +{
> +	desc->status_use_accessors |= _IRQ_NO_DEBUG;
> +}
> +
> +static inline bool irq_settings_no_debug(struct irq_desc *desc)
> +{
> +	return desc->status_use_accessors & _IRQ_NO_DEBUG;
> +}
> --- a/kernel/irq/spurious.c
> +++ b/kernel/irq/spurious.c
> @@ -274,7 +274,7 @@ void note_interrupt(struct irq_desc *des
>  	unsigned int irq;
>  
>  	if (desc->istate & IRQS_POLL_INPROGRESS ||
> -	    irq_settings_is_polled(desc))
> +	    irq_settings_is_polled(desc) | irq_settings_no_debug(desc))

Shouldn't it be '||' instead  ? 

>  		return;
>  
>  	if (bad_action_ret(action_ret)) {
> 

We could test irq_settings_no_debug() directly under handle_nested_irq() 
and handle_irq_event_percpu() to avoid calling note_interrupt(), just 
like we do for noirqdebug.

Thanks,

C.