Re: [PATCH] genirq: reduce irqdebug bouncing cachelines

2021-04-14 Thread Cédric Le Goater 
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.

Re: [PATCH] genirq: reduce irqdebug bouncing cachelines

2021-04-13 Thread Thomas Gleixner 
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

Re: [PATCH] genirq: reduce irqdebug bouncing cachelines

2021-04-13 Thread Cédric Le Goater 
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.

Re: [PATCH] genirq: reduce irqdebug bouncing cachelines

2021-04-12 Thread Thomas Gleixner 
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(>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;
 }

Re: [PATCH] genirq: reduce irqdebug bouncing cachelines

2021-04-12 Thread 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  0x0080
>  #define IRQF_PROBE_SHARED0x0100
> @@ -78,6 +80,7 @@
>  #define IRQF_EARLY_RESUME0x0002
>  #define IRQF_COND_SUSPEND0x0004
>  #define IRQF_NO_AUTOEN   0x0008
> +#define IRQF_NO_DEBUG0x0010
>  
>  #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   = 0x,
> @@ -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(>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_POLLEDGOT_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.

Re: [PATCH] genirq: reduce irqdebug bouncing cachelines

2021-04-10 Thread Thomas Gleixner 
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_SHARED0x0080
 #define IRQF_PROBE_SHARED  0x0100
@@ -78,6 +80,7 @@
 #define IRQF_EARLY_RESUME  0x0002
 #define IRQF_COND_SUSPEND  0x0004
 #define IRQF_NO_AUTOEN 0x0008
+#define IRQF_NO_DEBUG  0x0010
 
 #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   = 0x,
@@ -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(>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)) {