[PATCH 0/3] genirq: oneshot-safe threaded EOIs

[PATCH 0/3] genirq: oneshot-safe threaded EOIs

[Samuel Holland]

This is the optimization that I promised back in
https://lore.kernel.org/lkml/20220701202440.59059-1-samuel@sholland.org/
and got back around to cleaning up. To quote that cover letter:

"A further optimization is to take advantage of the fact that multiple
IRQs can be claimed at once. This allows removing the mask operations
for oneshot IRQs -- i.e. the combination of IRQCHIP_ONESHOT_SAFE and
IRQCHIP_EOI_THREADED, which is not currently supported."

Or in other words, we reuse the oneshot infrastructure for EOIs instead
of masking.

There a a few functions/variables that probably should be renamed now
that I have muddied what "oneshot" means. I elected not to do that in
this version to avoid distracting from the functional changes.


Samuel Holland (3):
  genirq: Simplify cond_unmask_eoi_irq()
  genirq: Add support for oneshot-safe threaded EOIs
  irqchip/sifive-plic: Enable oneshot-safe threaded EOIs

 drivers/irqchip/irq-sifive-plic.c |  2 ++
 kernel/irq/chip.c                 | 21 ++++++++++++---------
 kernel/irq/manage.c               | 15 ++++++++++-----
 3 files changed, 24 insertions(+), 14 deletions(-)

-- 
2.37.4


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 1/3] genirq: Simplify cond_unmask_eoi_irq()

[Samuel Holland]

This function calls .irq_eoi in three places, making the logic hard to
follow. Rearrange the function so that .irq_eoi is called only once.

The only time .irq_eoi is not called is when all three if checks fail,
so return early in that case. threads_oneshot can only be nonzero if
IRQS_ONESHOT is set, so the IRQS_ONESHOT check can be omitted there.

The IRQS_ONESHOT condition from the first if statement must then be
copied to the unmask_irq() condition.

Furthermore, if IRQS_ONESHOT is set, mask_irq() must have been called
in the parent function, so the irqd_irq_masked() check is redundant.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 kernel/irq/chip.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 8ac37e8e738a..672bad021a1f 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -658,10 +658,15 @@ EXPORT_SYMBOL_GPL(handle_level_irq);
 
 static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip)
 {
-	if (!(desc->istate & IRQS_ONESHOT)) {
-		chip->irq_eoi(&desc->irq_data);
+	/*
+	 * Do not send an EOI if the thread will do it later in
+	 * unmask_threaded_irq().
+	 */
+	if ((chip->flags & IRQCHIP_EOI_THREADED) && desc->threads_oneshot)
 		return;
-	}
+
+	chip->irq_eoi(&desc->irq_data);
+
 	/*
 	 * We need to unmask in the following cases:
 	 * - Oneshot irq which did not wake the thread (caused by a
@@ -669,12 +674,8 @@ static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip)
 	 *   completely).
 	 */
 	if (!irqd_irq_disabled(&desc->irq_data) &&
-	    irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) {
-		chip->irq_eoi(&desc->irq_data);
+	    (desc->istate & IRQS_ONESHOT) && !desc->threads_oneshot)
 		unmask_irq(desc);
-	} else if (!(chip->flags & IRQCHIP_EOI_THREADED)) {
-		chip->irq_eoi(&desc->irq_data);
-	}
 }
 
 /**
-- 
2.37.4


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 2/3] genirq: Add support for oneshot-safe threaded EOIs

[Samuel Holland]

For irqchips such as the SiFive PLIC with a claim/EOI flow, each IRQ is
implicitly masked during the claim operation and unmasked after the EOI.
By delaying the EOI until after the thread runs, we can support threaded
IRQs without any explicit mask/unmask operations.

irqchips can declare this capability using the combination of flags
IRQCHIP_ONESHOT_SAFE | IRQCHIP_EOI_THREADED.

In this case, we still set IRQF_ONESHOT and thus action->thread_mask, so
we know based on desc->threads_oneshot when to send the EOI. However, we
do not set IRQS_ONESHOT, so we skip the actual mask/unmask operations.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 kernel/irq/chip.c   |  4 +++-
 kernel/irq/manage.c | 15 ++++++++++-----
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 672bad021a1f..7a4b3fa85da0 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -446,7 +446,9 @@ void unmask_threaded_irq(struct irq_desc *desc)
 	if (chip->flags & IRQCHIP_EOI_THREADED)
 		chip->irq_eoi(&desc->irq_data);
 
-	unmask_irq(desc);
+	if (!irqd_irq_disabled(&desc->irq_data) &&
+	    (desc->istate & IRQS_ONESHOT))
+		unmask_irq(desc);
 }
 
 /*
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 40fe7806cc8c..b9edb66428cd 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1074,9 +1074,10 @@ static int irq_wait_for_interrupt(struct irqaction *action)
 static void irq_finalize_oneshot(struct irq_desc *desc,
 				 struct irqaction *action)
 {
-	if (!(desc->istate & IRQS_ONESHOT) ||
+	if (!(action->flags & IRQF_ONESHOT) ||
 	    action->handler == irq_forced_secondary_handler)
 		return;
+
 again:
 	chip_bus_lock(desc);
 	raw_spin_lock_irq(&desc->lock);
@@ -1112,8 +1113,7 @@ static void irq_finalize_oneshot(struct irq_desc *desc,
 
 	desc->threads_oneshot &= ~action->thread_mask;
 
-	if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&
-	    irqd_irq_masked(&desc->irq_data))
+	if (!desc->threads_oneshot)
 		unmask_threaded_irq(desc);
 
 out_unlock:
@@ -1565,8 +1565,12 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	 * MSI based interrupts are per se one shot safe. Check the
 	 * chip flags, so we can avoid the unmask dance at the end of
 	 * the threaded handler for those.
+	 *
+	 * If IRQCHIP_EOI_THREADED is also set, we do an EOI dance
+	 * instead of a mask/unmask dance.
 	 */
-	if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)
+	if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE &&
+	    !(desc->irq_data.chip->flags & IRQCHIP_EOI_THREADED))
 		new->flags &= ~IRQF_ONESHOT;
 
 	/*
@@ -1755,7 +1759,8 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		if (noirqdebug)
 			irq_settings_set_no_debug(desc);
 
-		if (new->flags & IRQF_ONESHOT)
+		if (new->flags & IRQF_ONESHOT &&
+		    !(desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE))
 			desc->istate |= IRQS_ONESHOT;
 
 		/* Exclude IRQ from balancing if requested */
-- 
2.37.4


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 3/3] irqchip/sifive-plic: Enable oneshot-safe threaded EOIs

[Samuel Holland]

This defers the EOI until the IRQ thread has finished, avoiding the need
to mask the IRQ while the thread is pending.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/irqchip/irq-sifive-plic.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index ff47bd0dec45..d8fc3354b38c 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -203,6 +203,8 @@ static struct irq_chip plic_chip = {
 #endif
 	.irq_set_type	= plic_irq_set_type,
 	.flags		= IRQCHIP_SKIP_SET_WAKE |
+			  IRQCHIP_ONESHOT_SAFE |
+			  IRQCHIP_EOI_THREADED |
 			  IRQCHIP_AFFINITY_PRE_STARTUP,
 };
 
-- 
2.37.4


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/3] genirq: Simplify cond_unmask_eoi_irq()

[Thomas Gleixner]

Samuel!

On Sat, Nov 26 2022 at 17:41, Samuel Holland wrote:

> This function calls .irq_eoi in three places, making the logic hard to
> follow. Rearrange the function so that .irq_eoi is called only once.
>
> The only time .irq_eoi is not called is when all three if checks fail,
> so return early in that case. threads_oneshot can only be nonzero if
> IRQS_ONESHOT is set, so the IRQS_ONESHOT check can be omitted there.
>
> The IRQS_ONESHOT condition from the first if statement must then be
> copied to the unmask_irq() condition.
>
> Furthermore, if IRQS_ONESHOT is set, mask_irq() must have been called
> in the parent function, so the irqd_irq_masked() check is redundant.

Not really convinced that all this is functionaly equivalent.

>  static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip)
>  {
> -	if (!(desc->istate & IRQS_ONESHOT)) {
> -		chip->irq_eoi(&desc->irq_data);
> +	/*
> +	 * Do not send an EOI if the thread will do it later in
> +	 * unmask_threaded_irq().
> +	 */
> +	if ((chip->flags & IRQCHIP_EOI_THREADED) && desc->threads_oneshot)
>  		return;
> -	}
> +
> +	chip->irq_eoi(&desc->irq_data);

This now issues EOI when the interrupt is in disabled state, which was
not done before. That's probably a non-issue, but clearly a undocumented
change.

> +
>  	/*
>  	 * We need to unmask in the following cases:
>  	 * - Oneshot irq which did not wake the thread (caused by a
> @@ -669,12 +674,8 @@ static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip)
>  	 *   completely).
>  	 */
>  	if (!irqd_irq_disabled(&desc->irq_data) &&
> -	    irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) {
> -		chip->irq_eoi(&desc->irq_data);
> +	    (desc->istate & IRQS_ONESHOT) && !desc->threads_oneshot)
>  		unmask_irq(desc);

This breaks the mask logic of handle_fasteoi_mask_irq() for an interrupt
which does not have IRQS_ONESHOT set.

So no, it's not the same and it even breaks stuff.

Thanks,

        tglx

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/3] genirq: Simplify cond_unmask_eoi_irq()

[Thomas Gleixner]

On Wed, Nov 30 2022 at 15:48, Thomas Gleixner wrote:
> On Sat, Nov 26 2022 at 17:41, Samuel Holland wrote:
>> +	    (desc->istate & IRQS_ONESHOT) && !desc->threads_oneshot)
>>  		unmask_irq(desc);
>
> This breaks the mask logic of handle_fasteoi_mask_irq() for an interrupt
> which does not have IRQS_ONESHOT set.

Ignore this one. That handler is broken by design.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/3] genirq: Add support for oneshot-safe threaded EOIs

[Thomas Gleixner]

Samuel!

On Sat, Nov 26 2022 at 17:41, Samuel Holland wrote:
> For irqchips such as the SiFive PLIC with a claim/EOI flow, each IRQ is
> implicitly masked during the claim operation and unmasked after the EOI.
> By delaying the EOI until after the thread runs, we can support threaded
> IRQs without any explicit mask/unmask operations.
>
> irqchips can declare this capability using the combination of flags
> IRQCHIP_ONESHOT_SAFE | IRQCHIP_EOI_THREADED.
>
> In this case, we still set IRQF_ONESHOT and thus action->thread_mask, so
> we know based on desc->threads_oneshot when to send the EOI. However, we
> do not set IRQS_ONESHOT, so we skip the actual mask/unmask operations.

Lots of 'we' here.

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog

> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 672bad021a1f..7a4b3fa85da0 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -446,7 +446,9 @@ void unmask_threaded_irq(struct irq_desc *desc)
>  	if (chip->flags & IRQCHIP_EOI_THREADED)
>  		chip->irq_eoi(&desc->irq_data);
>  
> -	unmask_irq(desc);
> +	if (!irqd_irq_disabled(&desc->irq_data) &&
> +	    (desc->istate & IRQS_ONESHOT))
> +		unmask_irq(desc);
>
>  /*
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 40fe7806cc8c..b9edb66428cd 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1074,9 +1074,10 @@ static int irq_wait_for_interrupt(struct irqaction *action)
>  static void irq_finalize_oneshot(struct irq_desc *desc,
>  				 struct irqaction *action)
>  {
> -	if (!(desc->istate & IRQS_ONESHOT) ||
> +	if (!(action->flags & IRQF_ONESHOT) ||
>  	    action->handler == irq_forced_secondary_handler)
>  		return;
> +
>  again:
>  	chip_bus_lock(desc);
>  	raw_spin_lock_irq(&desc->lock);
> @@ -1112,8 +1113,7 @@ static void irq_finalize_oneshot(struct irq_desc *desc,
>  
>  	desc->threads_oneshot &= ~action->thread_mask;
>  
> -	if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&

Same undocumented change vs. disabled and EOI. It might not matter, but
if something like this is changed, then it has to be done in a separate
commit with a very good explanation why this is correct and not silently
burried into a hodgepodge of other changes.

> -	    irqd_irq_masked(&desc->irq_data))
> +	if (!desc->threads_oneshot)
>  		unmask_threaded_irq(desc);
>  
>  out_unlock:
> @@ -1565,8 +1565,12 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>  	 * MSI based interrupts are per se one shot safe. Check the
>  	 * chip flags, so we can avoid the unmask dance at the end of
>  	 * the threaded handler for those.
> +	 *
> +	 * If IRQCHIP_EOI_THREADED is also set, we do an EOI dance
> +	 * instead of a mask/unmask dance.
>  	 */
> -	if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)
> +	if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE &&
> +	    !(desc->irq_data.chip->flags & IRQCHIP_EOI_THREADED))

This flag combination makes my head spin. 

>  		new->flags &= ~IRQF_ONESHOT;
>  
>  	/*
> @@ -1755,7 +1759,8 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>  		if (noirqdebug)
>  			irq_settings_set_no_debug(desc);
>  
> -		if (new->flags & IRQF_ONESHOT)
> +		if (new->flags & IRQF_ONESHOT &&
> +		    !(desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE))
>  			desc->istate |= IRQS_ONESHOT;

And I really detest the resulting action to desc inconsistency:

> -	if (!(desc->istate & IRQS_ONESHOT) ||
> +	if (!(action->flags & IRQF_ONESHOT) ||

It's clever, but completely non-obvious.

So let's take a step back. You want to defer the EOI to the point where
the threaded handler finishes, but you dont want the mask/unmask
mechanism which is attached to IRQS_ONESHOT.

So instead of playing games with IRQS_ONESHOT and IRQF_ONESHOT and
creating something which cannot even be remotely followed, why not
having explicit and understandable flags for theses cases.

Something like the incomplete, uncompiled below. You get the idea,
right?

Thanks,

        tglx
---
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -656,23 +656,27 @@ void handle_level_irq(struct irq_desc *d
 }
 EXPORT_SYMBOL_GPL(handle_level_irq);
 
+static inline cond_mask_eoi_irq(struct irq_desc *desc)
+{
+	if (desc->istate & IRQS_UNMASK_EOI_THREAD)
+		mask_irq(desc);
+}
+
 static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip)
 {
-	if (!(desc->istate & IRQS_ONESHOT)) {
-		chip->irq_eoi(&desc->irq_data);
-		return;
-	}
-	/*
-	 * We need to unmask in the following cases:
-	 * - Oneshot irq which did not wake the thread (caused by a
-	 *   spurious interrupt or a primary handler handling it
-	 *   completely).
-	 */
-	if (!irqd_irq_disabled(&desc->irq_data) &&
-	    irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) {
-		chip->irq_eoi(&desc->irq_data);
-		unmask_irq(desc);
-	} else if (!(chip->flags & IRQCHIP_EOI_THREADED)) {
+	if (desc->istate & IRQS_UNMASK_EOI_THREAD) {
+		/*
+		 * We need to EOI and unmask when the primary handler
+		 * did not wake the thread due to a spurious interrupt or
+		 * the primary handler handling the interrupt completely.
+		 */
+		if (!irqd_irq_disabled(&desc->irq_data) &&
+		    irqd_irq_masked(&desc->irq_data) &&
+		    !desc->threads_oneshot) {
+			chip->irq_eoi(&desc->irq_data);
+			unmask_irq(desc);
+		}
+	} else if (!(desc->istate & IRQS_EOI_THREAD) || !desc->threads_oneshot) {
 		chip->irq_eoi(&desc->irq_data);
 	}
 }
@@ -708,8 +712,7 @@ void handle_fasteoi_irq(struct irq_desc
 	}
 
 	kstat_incr_irqs_this_cpu(desc);
-	if (desc->istate & IRQS_ONESHOT)
-		mask_irq(desc);
+	cond_mask_eoi_irq(desc);
 
 	handle_irq_event(desc);
 
@@ -1219,8 +1222,7 @@ void handle_fasteoi_ack_irq(struct irq_d
 	}
 
 	kstat_incr_irqs_this_cpu(desc);
-	if (desc->istate & IRQS_ONESHOT)
-		mask_irq(desc);
+	cond_mask_eoi_irq(desc);
 
 	/* Start handling the irq */
 	desc->irq_data.chip->irq_ack(&desc->irq_data);
@@ -1266,16 +1268,14 @@ void handle_fasteoi_mask_irq(struct irq_
 	 */
 	if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) {
 		desc->istate |= IRQS_PENDING;
-		mask_irq(desc);
 		goto out;
 	}
 
 	kstat_incr_irqs_this_cpu(desc);
-	if (desc->istate & IRQS_ONESHOT)
-		mask_irq(desc);
 
 	handle_irq_event(desc);
 
+	/* Needs more fixes ... */
 	cond_unmask_eoi_irq(desc, chip);
 
 	raw_spin_unlock(&desc->lock);
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -53,6 +53,8 @@ enum {
  * IRQS_SUSPENDED		- irq is suspended
  * IRQS_NMI			- irq line is used to deliver NMIs
  * IRQS_SYSFS			- descriptor has been added to sysfs
+ * IRQS_UNMASK_EOI_THREAD	- EOI and unmask is deferred to the threaded handler
+ * IRQS_EOI_THREAD		- EOI is deferred to the threaded handler
  */
 enum {
 	IRQS_AUTODETECT		= 0x00000001,
@@ -66,6 +68,8 @@ enum {
 	IRQS_TIMINGS		= 0x00001000,
 	IRQS_NMI		= 0x00002000,
 	IRQS_SYSFS		= 0x00004000,
+	IRQS_UNMASK_EOI_THREAD	= 0x00008000,
+	IRQS_EOI_THREAD		= 0x00010000,
 };
 
 #include "debug.h"



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv