linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] genirq: reduce irqdebug bouncing cachelines
@ 2021-04-02 13:20 Nicholas Piggin
  2021-04-10 11:38 ` [tip: irq/core] genirq: Reduce irqdebug cacheline bouncing tip-bot2 for Nicholas Piggin
  2021-04-10 11:58 ` [PATCH] genirq: reduce irqdebug bouncing cachelines Thomas Gleixner
  0 siblings, 2 replies; 10+ messages in thread
From: Nicholas Piggin @ 2021-04-02 13:20 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: Nicholas Piggin, Cédric Le Goater, linux-kernel

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.

Cédric reports the result of his IPI throughput test:

               Millions of IPIs/s
 -----------   --------------------------------------
               upstream   upstream   patched
 chips  cpus   default    noirqdebug default (irqdebug)
 -----------   -----------------------------------------
 1      0-15     4.061      4.153      4.084
        0-31     7.937      8.186      8.158
        0-47    11.018     11.392     11.233
        0-63    11.460     13.907     14.022
 2      0-79     8.376     18.105     18.084
        0-95     7.338     22.101     22.266
        0-111    6.716     25.306     25.473
        0-127    6.223     27.814     28.029

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 kernel/irq/spurious.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/irq/spurious.c b/kernel/irq/spurious.c
index f865e5f4d382..c481d8458325 100644
--- a/kernel/irq/spurious.c
+++ b/kernel/irq/spurious.c
@@ -403,6 +403,10 @@ void note_interrupt(struct irq_desc *desc, irqreturn_t action_ret)
 			desc->irqs_unhandled -= ok;
 	}
 
+	if (likely(!desc->irqs_unhandled))
+		return;
+
+	/* Now getting into unhandled irq detection */
 	desc->irq_count++;
 	if (likely(desc->irq_count < 100000))
 		return;
-- 
2.23.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [tip: irq/core] genirq: Reduce irqdebug cacheline bouncing
  2021-04-02 13:20 [PATCH] genirq: reduce irqdebug bouncing cachelines Nicholas Piggin
@ 2021-04-10 11:38 ` tip-bot2 for Nicholas Piggin
  2021-04-10 11:58 ` [PATCH] genirq: reduce irqdebug bouncing cachelines Thomas Gleixner
  1 sibling, 0 replies; 10+ messages in thread
From: tip-bot2 for Nicholas Piggin @ 2021-04-10 11:38 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Nicholas Piggin, Thomas Gleixner, x86, linux-kernel, maz

The following commit has been merged into the irq/core branch of tip:

Commit-ID:     7c07012eb1be8b4a95d3502fd30795849007a40e
Gitweb:        https://git.kernel.org/tip/7c07012eb1be8b4a95d3502fd30795849007a40e
Author:        Nicholas Piggin <npiggin@gmail.com>
AuthorDate:    Fri, 02 Apr 2021 23:20:37 +10:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Sat, 10 Apr 2021 13:35:54 +02:00

genirq: Reduce irqdebug cacheline bouncing

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.

Cédric reports the result of his IPI throughput test:

               Millions of IPIs/s
 -----------   --------------------------------------
               upstream   upstream   patched
 chips  cpus   default    noirqdebug default (irqdebug)
 -----------   -----------------------------------------
 1      0-15     4.061      4.153      4.084
        0-31     7.937      8.186      8.158
        0-47    11.018     11.392     11.233
        0-63    11.460     13.907     14.022
 2      0-79     8.376     18.105     18.084
        0-95     7.338     22.101     22.266
        0-111    6.716     25.306     25.473
        0-127    6.223     27.814     28.029

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/r/20210402132037.574661-1-npiggin@gmail.com

---
 kernel/irq/spurious.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/irq/spurious.c b/kernel/irq/spurious.c
index f865e5f..c481d84 100644
--- a/kernel/irq/spurious.c
+++ b/kernel/irq/spurious.c
@@ -403,6 +403,10 @@ void note_interrupt(struct irq_desc *desc, irqreturn_t action_ret)
 			desc->irqs_unhandled -= ok;
 	}
 
+	if (likely(!desc->irqs_unhandled))
+		return;
+
+	/* Now getting into unhandled irq detection */
 	desc->irq_count++;
 	if (likely(desc->irq_count < 100000))
 		return;

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] genirq: reduce irqdebug bouncing cachelines
  2021-04-02 13:20 [PATCH] genirq: reduce irqdebug bouncing cachelines Nicholas Piggin
  2021-04-10 11:38 ` [tip: irq/core] genirq: Reduce irqdebug cacheline bouncing tip-bot2 for Nicholas Piggin
@ 2021-04-10 11:58 ` Thomas Gleixner
  2021-04-12  9:06   ` Cédric Le Goater
  1 sibling, 1 reply; 10+ messages in thread
From: Thomas Gleixner @ 2021-04-10 11:58 UTC (permalink / raw)
  To: Nicholas Piggin; +Cc: Nicholas Piggin, Cédric Le Goater, linux-kernel

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))
 		return;
 
 	if (bad_action_ret(action_ret)) {

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] genirq: reduce irqdebug bouncing cachelines
  2021-04-10 11:58 ` [PATCH] genirq: reduce irqdebug bouncing cachelines Thomas Gleixner
@ 2021-04-12  9:06   ` Cédric Le Goater
  2021-04-12 12:43     ` Thomas Gleixner
  0 siblings, 1 reply; 10+ messages in thread
From: Cédric Le Goater @ 2021-04-12  9:06 UTC (permalink / raw)
  To: Thomas Gleixner, Nicholas Piggin; +Cc: linux-kernel

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] genirq: reduce irqdebug bouncing cachelines
  2021-04-12  9:06   ` Cédric Le Goater
@ 2021-04-12 12:43     ` Thomas Gleixner
  2021-04-13 12:16       ` Cédric Le Goater
  0 siblings, 1 reply; 10+ messages in thread
From: Thomas Gleixner @ 2021-04-12 12:43 UTC (permalink / raw)
  To: Cédric Le Goater, Nicholas Piggin; +Cc: linux-kernel

Cédric,

On Mon, Apr 12 2021 at 11:06, Cédric Le Goater wrote:
> On 4/10/21 1:58 PM, Thomas Gleixner wrote:
>> --- 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  ?

It could. But that's intentionally '|'. Why?

Because that lets the compiler merge the bit checks into one and
therefore spares one conditional branch.

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

We can do that, but then we should not just make it:

   if (!irqnodebug && !irq_settings_no_debug(desc))
   	note_interrupt(...);

Instead have only one condition:

   if (!irq_settings_no_debug(desc))
   	note_interrupt(...);

See the uncompiled delta patch below.

Thanks,

        tglx
---
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1690,6 +1690,9 @@ static int
 				irq_settings_set_no_debug(desc);
 		}
 
+		if (noirqdebug)
+			irq_settings_set_no_debug(desc);
+
 		if (new->flags & IRQF_ONESHOT)
 			desc->istate |= IRQS_ONESHOT;
 
--- 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_no_debug(desc))
+	    irq_settings_is_polled(desc))
 		return;
 
 	if (bad_action_ret(action_ret)) {
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -481,7 +481,7 @@ void handle_nested_irq(unsigned int irq)
 	for_each_action_of_desc(desc, action)
 		action_ret |= action->thread_fn(action->irq, action->dev_id);
 
-	if (!noirqdebug)
+	if (!irq_settings_no_debug(desc))
 		note_interrupt(desc, action_ret);
 
 	raw_spin_lock_irq(&desc->lock);
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -197,7 +197,7 @@ irqreturn_t handle_irq_event_percpu(stru
 
 	add_interrupt_randomness(desc->irq_data.irq, flags);
 
-	if (!noirqdebug)
+	if (!irq_settings_no_debug(desc))
 		note_interrupt(desc, retval);
 	return retval;
 }

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] genirq: reduce irqdebug bouncing cachelines
  2021-04-12 12:43     ` Thomas Gleixner
@ 2021-04-13 12:16       ` Cédric Le Goater
  2021-04-13 20:24         ` Thomas Gleixner
  0 siblings, 1 reply; 10+ messages in thread
From: Cédric Le Goater @ 2021-04-13 12:16 UTC (permalink / raw)
  To: Thomas Gleixner, Nicholas Piggin; +Cc: linux-kernel

Thomas,

>> 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.
> 
> We can do that, but then we should not just make it:
> 
>    if (!irqnodebug && !irq_settings_no_debug(desc))
>    	note_interrupt(...);
> 
> Instead have only one condition:
> 
>    if (!irq_settings_no_debug(desc))
>    	note_interrupt(...);
> 
> See the uncompiled delta patch below.

I merged this second part with the first and gave IRQF_NO_DEBUG a try 
on P8 and P9 systems and all looked fine. I should send both patches 
after IRQF_NO_AUTOEN is merged in mainline.     

Thanks,

C.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] genirq: reduce irqdebug bouncing cachelines
  2021-04-13 12:16       ` Cédric Le Goater
@ 2021-04-13 20:24         ` Thomas Gleixner
  2021-04-14 13:13           ` Cédric Le Goater
  2021-05-15 17:01           ` Cédric Le Goater
  0 siblings, 2 replies; 10+ messages in thread
From: Thomas Gleixner @ 2021-04-13 20:24 UTC (permalink / raw)
  To: Cédric Le Goater, Nicholas Piggin; +Cc: linux-kernel

On Tue, Apr 13 2021 at 14:16, Cédric Le Goater wrote:
>>> 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.
>> 
>> We can do that, but then we should not just make it:
>> 
>>    if (!irqnodebug && !irq_settings_no_debug(desc))
>>    	note_interrupt(...);
>> 
>> Instead have only one condition:
>> 
>>    if (!irq_settings_no_debug(desc))
>>    	note_interrupt(...);
>> 
>> See the uncompiled delta patch below.
>
> I merged this second part with the first and gave IRQF_NO_DEBUG a try 
> on P8 and P9 systems and all looked fine. I should send both patches 
> after IRQF_NO_AUTOEN is merged in mainline.     

Does having that NODEBUG flag set on the IPI irqs make a measurable
difference or is it just too small to matter?

Thanks,

        tglx





^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] genirq: reduce irqdebug bouncing cachelines
  2021-04-13 20:24         ` Thomas Gleixner
@ 2021-04-14 13:13           ` Cédric Le Goater
  2021-05-15 17:01           ` Cédric Le Goater
  1 sibling, 0 replies; 10+ messages in thread
From: Cédric Le Goater @ 2021-04-14 13:13 UTC (permalink / raw)
  To: Thomas Gleixner, Nicholas Piggin; +Cc: linux-kernel

On 4/13/21 10:24 PM, Thomas Gleixner wrote:
> On Tue, Apr 13 2021 at 14:16, Cédric Le Goater wrote:
>>>> 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.
>>>
>>> We can do that, but then we should not just make it:
>>>
>>>    if (!irqnodebug && !irq_settings_no_debug(desc))
>>>    	note_interrupt(...);
>>>
>>> Instead have only one condition:
>>>
>>>    if (!irq_settings_no_debug(desc))
>>>    	note_interrupt(...);
>>>
>>> See the uncompiled delta patch below.
>>
>> I merged this second part with the first and gave IRQF_NO_DEBUG a try 
>> on P8 and P9 systems and all looked fine. I should send both patches 
>> after IRQF_NO_AUTOEN is merged in mainline.     
> 
> Does having that NODEBUG flag set on the IPI irqs make a measurable
> difference or is it just too small to matter?

It does not add much benefits on the 2s and 4s systems but I will give it 
a try on bigger ones when I can grab them. Then we can decide if it is 
worth merging. 

Thanks,

C.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] genirq: reduce irqdebug bouncing cachelines
  2021-04-13 20:24         ` Thomas Gleixner
  2021-04-14 13:13           ` Cédric Le Goater
@ 2021-05-15 17:01           ` Cédric Le Goater
  2021-05-17 18:04             ` [tip: irq/core] genirq: Add a IRQF_NO_DEBUG flag tip-bot2 for Thomas Gleixner
  1 sibling, 1 reply; 10+ messages in thread
From: Cédric Le Goater @ 2021-05-15 17:01 UTC (permalink / raw)
  To: Thomas Gleixner, Nicholas Piggin; +Cc: linux-kernel

On 4/13/21 10:24 PM, Thomas Gleixner wrote:
> On Tue, Apr 13 2021 at 14:16, Cédric Le Goater wrote:
>>>> 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.
>>>
>>> We can do that, but then we should not just make it:
>>>
>>>    if (!irqnodebug && !irq_settings_no_debug(desc))
>>>    	note_interrupt(...);
>>>
>>> Instead have only one condition:
>>>
>>>    if (!irq_settings_no_debug(desc))
>>>    	note_interrupt(...);
>>>
>>> See the uncompiled delta patch below.
>>
>> I merged this second part with the first and gave IRQF_NO_DEBUG a try 
>> on P8 and P9 systems and all looked fine. I should send both patches 
>> after IRQF_NO_AUTOEN is merged in mainline.     
> 
> Does having that NODEBUG flag set on the IPI irqs make a measurable
> difference or is it just too small to matter?

Here are some ipistorm results on a P10 system with 12 activated chips, 
each with ~120 HW threads. The kernel is a 5.12 plus extras : 

  - XIVE IPI IRQ domain patchset (merged in 5.13-rc1)
  - MSI IRQ domain pachset (should not have any impact)
  - your IRQF_NO_DEBUG patch.

There is some slight improvement on the first 4 chips but it gets
better when we reach the fifth which requires an extra hop on the
bus. On 12 chips, we are close to an extra 9%. 

Given these results, I think it would good to have the IRQF_NO_DEBUG 
flag. 

Thanks,

C.

[*] https://github.com/antonblanchard/ipistorm


                         Mint/s

chips   cpus              IRQF_NO_DEBUG
---------------------------------------
0         15       4.32         4.32   
          31       8.63         8.65   
          47      12.86        12.89  
          63      17.11        17.16  
          79      21.29        21.40  
          95      25.45        25.67  
         111      29.61        29.80  
1        127      33.53        33.64  
         143      37.55        37.77  
         159      41.50        41.78  
         175      45.42        45.63  
         191      49.39        49.51  
         207      53.27        53.61  
         223      57.17        57.73  
         239      61.05        61.59  
2        255      64.94        65.69  
         271      65.20        62.62  
         287      62.20        65.18  
         303      63.96        67.69  
         319      65.74        69.95  
         335      67.45        71.84  
         351      69.63        72.75  
3        367      70.96        74.38  
         383      72.34        75.62  
         399      74.26        77.69  
         415      76.49        79.78  
         431      78.46        81.67  
         447      79.87        82.50  
         463      82.07        85.13  
         479      83.95        87.03  
4        495      87.67        92.94  
         511      93.70        98.74  
         527      95.43       101.45 
         543      97.39       103.39 
         559      98.95       104.76 
         575     100.08       105.70 
         591     101.26       107.06 
         607     102.17       108.16 
5        623     103.27       109.44 
         639     104.95       111.14 
         655     106.59       112.36 
         671     107.92       113.72 
         687     108.75       115.25 
         703     109.84       117.06 
         719     110.99       118.45 
6        735     112.30       119.07 
         751     114.19       120.72 
         767     115.59       122.70 
         783     116.69       124.80 
         799     118.62       126.38 
         815     120.38       128.36 
         831     120.78       129.15 
         847     122.69       130.44 
7        863     123.65       132.44 
         879     124.78       133.83 
         895     126.58       135.43 
         911     128.02       136.85 
         927     130.48       138.98 
         943     130.98       140.58 
         959     132.17       141.74 
8        975     134.55       142.62 
         991     134.89       143.82 
        1007     135.64       145.51
        1023     137.23       147.22
        1039     139.37       149.85
        1055     141.80       150.27
        1071     141.37       152.33
        1087     143.08       153.20
9       1103     144.83       154.23
        1119     144.55       155.96
        1135     147.32       157.91
        1151     148.31       159.04
        1167     150.67       160.41
        1183     149.44       161.82
        1199     152.53       164.69
        1215     153.51       165.31
10      1231     154.88       166.67
        1247     156.06       168.40
        1263     157.59       170.01
        1279     158.74       172.10
        1295     159.55       172.79
        1311     160.74       173.62
        1327     161.86       175.02
11      1343     163.75       175.95
        1359     163.67       177.96
        1375     164.95       178.94
        1391     166.53       180.09
        1407     167.17       181.25
        1423     168.84       183.94
        1439     170.20       183.96
        1455     170.47       185.47
        

From 130eb33a02eb9d98fd8d8148e420b7b7b243bb93 Mon Sep 17 00:00:00 2001
From: Thomas Gleixner <tglx@linutronix.de>
Date: Fri, 2 Apr 2021 08:23:25 +0200
Subject: [PATCH] genirq: Add a IRQF_NO_DEBUG flag
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.

Add a IRQF_NO_DEBUG flag.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/linux/interrupt.h |  3 +++
 include/linux/irq.h       |  2 ++
 kernel/irq/settings.h     | 12 ++++++++++++
 kernel/irq/chip.c         |  2 +-
 kernel/irq/handle.c       |  2 +-
 kernel/irq/manage.c       |  5 +++++
 6 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 4777850a6dc7..a52109c3f3a4 100644
--- 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)
 
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 31b347c9f8dd..8e9a9ae471a6 100644
--- 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	\
diff --git a/kernel/irq/settings.h b/kernel/irq/settings.h
index 403378b9947b..7b7efb1a114b 100644
--- 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_hidden(struct irq_desc *desc)
 {
 	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;
+}
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 8cc8e5713287..7f04c7d8296e 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -481,7 +481,7 @@ void handle_nested_irq(unsigned int irq)
 	for_each_action_of_desc(desc, action)
 		action_ret |= action->thread_fn(action->irq, action->dev_id);
 
-	if (!noirqdebug)
+	if (!irq_settings_no_debug(desc))
 		note_interrupt(desc, action_ret);
 
 	raw_spin_lock_irq(&desc->lock);
diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 762a928e18f9..221d80c31e94 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -197,7 +197,7 @@ irqreturn_t handle_irq_event_percpu(struct irq_desc *desc)
 
 	add_interrupt_randomness(desc->irq_data.irq, flags);
 
-	if (!noirqdebug)
+	if (!irq_settings_no_debug(desc))
 		note_interrupt(desc, retval);
 	return retval;
 }
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 4c14356543d9..7bdd09e7d5f0 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1686,8 +1686,13 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		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 (noirqdebug)
+			irq_settings_set_no_debug(desc);
+
 		if (new->flags & IRQF_ONESHOT)
 			desc->istate |= IRQS_ONESHOT;
 
-- 
2.31.1

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [tip: irq/core] genirq: Add a IRQF_NO_DEBUG flag
  2021-05-15 17:01           ` Cédric Le Goater
@ 2021-05-17 18:04             ` tip-bot2 for Thomas Gleixner
  0 siblings, 0 replies; 10+ messages in thread
From: tip-bot2 for Thomas Gleixner @ 2021-05-17 18:04 UTC (permalink / raw)
  To: linux-tip-commits; +Cc: Thomas Gleixner, clg, x86, linux-kernel, maz

The following commit has been merged into the irq/core branch of tip:

Commit-ID:     c2b1063e8feb2115537addce10f36c0c82d11d9b
Gitweb:        https://git.kernel.org/tip/c2b1063e8feb2115537addce10f36c0c82d11d9b
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Fri, 02 Apr 2021 08:23:25 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Mon, 17 May 2021 20:01:35 +02:00

genirq: Add a IRQF_NO_DEBUG flag

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.

Add a IRQF_NO_DEBUG flag and mark the interrupt accordingly if supplied
when the interrupt is requested.

When noirqdebug is set on the kernel commandline, then the interrupt is
marked unconditionally so that there is only one condition in the hotpath
to evaluate.

 [ clg: Add changelog ]

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/r/7a8ad02f-63a8-c1aa-fdd1-39d973593d02@kaod.org
---
 include/linux/interrupt.h |  3 +++
 include/linux/irq.h       |  2 ++
 kernel/irq/chip.c         |  2 +-
 kernel/irq/handle.c       |  2 +-
 kernel/irq/manage.c       |  5 +++++
 kernel/irq/settings.h     | 12 ++++++++++++
 6 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 4777850..a52109c 100644
--- 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)
 
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 31b347c..8e9a9ae 100644
--- 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	\
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 8cc8e57..7f04c7d 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -481,7 +481,7 @@ void handle_nested_irq(unsigned int irq)
 	for_each_action_of_desc(desc, action)
 		action_ret |= action->thread_fn(action->irq, action->dev_id);
 
-	if (!noirqdebug)
+	if (!irq_settings_no_debug(desc))
 		note_interrupt(desc, action_ret);
 
 	raw_spin_lock_irq(&desc->lock);
diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 762a928..221d80c 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -197,7 +197,7 @@ irqreturn_t handle_irq_event_percpu(struct irq_desc *desc)
 
 	add_interrupt_randomness(desc->irq_data.irq, flags);
 
-	if (!noirqdebug)
+	if (!irq_settings_no_debug(desc))
 		note_interrupt(desc, retval);
 	return retval;
 }
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 4c14356..7bdd09e 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1686,8 +1686,13 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		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 (noirqdebug)
+			irq_settings_set_no_debug(desc);
+
 		if (new->flags & IRQF_ONESHOT)
 			desc->istate |= IRQS_ONESHOT;
 
diff --git a/kernel/irq/settings.h b/kernel/irq/settings.h
index 403378b..7b7efb1 100644
--- 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_hidden(struct irq_desc *desc)
 {
 	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;
+}

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-05-17 18:04 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-02 13:20 [PATCH] genirq: reduce irqdebug bouncing cachelines Nicholas Piggin
2021-04-10 11:38 ` [tip: irq/core] genirq: Reduce irqdebug cacheline bouncing tip-bot2 for Nicholas Piggin
2021-04-10 11:58 ` [PATCH] genirq: reduce irqdebug bouncing cachelines Thomas Gleixner
2021-04-12  9:06   ` Cédric Le Goater
2021-04-12 12:43     ` Thomas Gleixner
2021-04-13 12:16       ` Cédric Le Goater
2021-04-13 20:24         ` Thomas Gleixner
2021-04-14 13:13           ` Cédric Le Goater
2021-05-15 17:01           ` Cédric Le Goater
2021-05-17 18:04             ` [tip: irq/core] genirq: Add a IRQF_NO_DEBUG flag tip-bot2 for Thomas Gleixner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).