[PATCH] pipeline: irq: ack the irq when it is postponed inband irq from outside of the pipeline entry path.

[PATCH] pipeline: irq: ack the irq when it is postponed inband irq from outside of the pipeline entry path.

[Hongzhan Chen]

1. To avoid double ack for the interrupt that already acked in
   pipeline entry path, we add new flag to note such status and
   differentiate.
2. For postponed stacked edge irq that is handling outside of the
   pipeline entry path, as discussed in [1], ack the irq directly
   because it would never be acked in the pipeline entry path.

[1]: https://lore.kernel.org/xenomai/BY5PR11MB4276FEFA2C6D88C4B6CFFEFCF2E9A@BY5PR11MB4276.namprd11.prod.outlook.com/T/#t

Signed-off-by: Hongzhan Chen <hongzhan.chen@intel.com>

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 6aca6c036ada..7280a338bd3e 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -863,14 +863,23 @@ void handle_edge_irq(struct irq_desc *desc)
 		chip->irq_ack(&desc->irq_data);
 		desc->istate |= IRQS_EDGE;
 		handle_oob_irq(desc);
+
+		/* For inband irq, we tag acked status
+		 * to avoid double ack
+		 */
+		if (!irq_settings_is_oob(desc))
+			desc->istate |= IRQS_ACK;
+
 		goto out_unlock;
 	}
 
 	kstat_incr_irqs_this_cpu(desc);
 
 	/* Start handling the irq */
-	if (!irqs_pipelined())
+	if (!(desc->istate & IRQS_ACK))
 		chip->irq_ack(&desc->irq_data);
+	else
+		desc->istate &= ~IRQS_ACK;
 
 	do {
 		if (unlikely(!desc->action)) {
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index a9121f6e6f1c..1b43467b5407 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -54,6 +54,7 @@ enum {
  * IRQS_NMI			- irq line is used to deliver NMIs
  * IRQS_SYSFS			- descriptor has been added to sysfs
  * IRQS_EDGE			- irq line received an edge event
+ * IRQS_ACK			- acked irq line
  */
 enum {
 	IRQS_AUTODETECT		= 0x00000001,
@@ -68,6 +69,7 @@ enum {
 	IRQS_NMI		= 0x00002000,
 	IRQS_SYSFS		= 0x00004000,
 	IRQS_EDGE		= 0x00008000,
+	IRQS_ACK		= 0x00010000,
 };
 
 #include "debug.h"
-- 
2.17.1

RE: [PATCH] pipeline: irq: ack the irq when it is postponed inband irq from outside of the pipeline entry path.

[Chen, Hongzhan]

>-----Original Message-----
>From: Hongzhan Chen <hongzhan.chen@intel.com>
>Sent: Wednesday, September 6, 2023 9:41 AM
>To: xenomai@lists.linux.dev
>Subject: [PATCH] pipeline: irq: ack the irq when it is postponed inband irq
>from outside of the pipeline entry path.
>
>1. To avoid double ack for the interrupt that already acked in
>   pipeline entry path, we add new flag to note such status and
>   differentiate.
>2. For postponed stacked edge irq that is handling outside of the
>   pipeline entry path, as discussed in [1], ack the irq directly
>   because it would never be acked in the pipeline entry path.
>
>[1]:
>https://lore.kernel.org/xenomai/BY5PR11MB4276FEFA2C6D88C4B6CFFEFCF2E
>9A@BY5PR11MB4276.namprd11.prod.outlook.com/T/#t
>
>Signed-off-by: Hongzhan Chen <hongzhan.chen@intel.com>
>
>diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>index 6aca6c036ada..7280a338bd3e 100644
>--- a/kernel/irq/chip.c
>+++ b/kernel/irq/chip.c
>@@ -863,14 +863,23 @@ void handle_edge_irq(struct irq_desc *desc)
> 		chip->irq_ack(&desc->irq_data);
> 		desc->istate |= IRQS_EDGE;
> 		handle_oob_irq(desc);
>+
>+		/* For inband irq, we tag acked status
>+		 * to avoid double ack
>+		 */
>+		if (!irq_settings_is_oob(desc))
>+			desc->istate |= IRQS_ACK;

Please ignore this patch. need to be optimized here.

Regards

Hongzhan Chen
>+
> 		goto out_unlock;
> 	}
>
> 	kstat_incr_irqs_this_cpu(desc);
>
> 	/* Start handling the irq */
>-	if (!irqs_pipelined())
>+	if (!(desc->istate & IRQS_ACK))
> 		chip->irq_ack(&desc->irq_data);
>+	else
>+		desc->istate &= ~IRQS_ACK;
>
> 	do {
> 		if (unlikely(!desc->action)) {
>diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
>index a9121f6e6f1c..1b43467b5407 100644
>--- a/kernel/irq/internals.h
>+++ b/kernel/irq/internals.h
>@@ -54,6 +54,7 @@ enum {
>  * IRQS_NMI			- irq line is used to deliver NMIs
>  * IRQS_SYSFS			- descriptor has been added to sysfs
>  * IRQS_EDGE			- irq line received an edge event
>+ * IRQS_ACK			- acked irq line
>  */
> enum {
> 	IRQS_AUTODETECT		= 0x00000001,
>@@ -68,6 +69,7 @@ enum {
> 	IRQS_NMI		= 0x00002000,
> 	IRQS_SYSFS		= 0x00004000,
> 	IRQS_EDGE		= 0x00008000,
>+	IRQS_ACK		= 0x00010000,
> };
>
> #include "debug.h"
>--
>2.17.1
>

Re: [PATCH] pipeline: irq: ack the irq when it is postponed inband irq from outside of the pipeline entry path.

[Philippe Gerum]

Hongzhan Chen <hongzhan.chen@intel.com> writes:

> 1. To avoid double ack for the interrupt that already acked in
>    pipeline entry path, we add new flag to note such status and
>    differentiate.
> 2. For postponed stacked edge irq that is handling outside of the
>    pipeline entry path, as discussed in [1], ack the irq directly
>    because it would never be acked in the pipeline entry path.
>
> [1]: https://lore.kernel.org/xenomai/BY5PR11MB4276FEFA2C6D88C4B6CFFEFCF2E9A@BY5PR11MB4276.namprd11.prod.outlook.com/T/#t
>
> Signed-off-by: Hongzhan Chen <hongzhan.chen@intel.com>
>
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 6aca6c036ada..b8d8aba38e17 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -862,15 +862,25 @@ void handle_edge_irq(struct irq_desc *desc)
>  	if (on_pipeline_entry()) {
>  		chip->irq_ack(&desc->irq_data);
>  		desc->istate |= IRQS_EDGE;
> -		handle_oob_irq(desc);
> +		if (!handle_oob_irq(desc)) {
> +
> +			/* For inband irq, we tag acked status
> +			 * to avoid double ack
> +			 */
> +			if (!oob_stage_present() || !irq_settings_is_oob(desc))
> +				desc->istate |= IRQS_ACK;

This code could move to handle_oob_irq(), provided that we account for
this generic behavior in the x86-specific APIC flow handler.

> +		}
> +
>  		goto out_unlock;
>  	}
>  
>  	kstat_incr_irqs_this_cpu(desc);
>  
>  	/* Start handling the irq */
> -	if (!irqs_pipelined())
> +	if (!(desc->istate & IRQS_ACK))

Disabling the IRQ pipeline should compile out any Dovetail-specific
code. In addition to ensuring zero overhead in this case, this is
important when chasing bugs as well, so that we can reliably compare
pipelined vs non-pipelined behavior with the very same kernel tree.

>  		chip->irq_ack(&desc->irq_data);
> +	else
> +		desc->istate &= ~IRQS_ACK;

Ok, this is going somewhere. I believe we need to generalize this
approach for all kinds of flow handlers, so that cascading any of them
would work, not only the edge one. There is also yet another tricky case
to care about when dealing with unmasked acked/eoi events, i.e.:

[IRQ-A] (acked, cascading, deferred handling to in-band stage)
  |
  +--> handler(A): demux IRQ-B (in-band)
       ...
       synchronize_pipeline_on_irq()
               hard irqs ON
               [IRQ-A]
                  |
                  +--> handler(A): demux IRQ-B
                       ...
                       synchronize_pipeline_on_irq()
                       hard irqs ON
                       flow_handler(B) (replay)
                       ...
                flow_handler(B) (replay)
                ...

and so on (a few intermediate steps were omitted). So we might have a
parent IRQ piling up prior to (re)playing the earlier one in-band with
hard irqs on, since we acked it early in the pipelining injection
path. IOW, the code which tracks the ack/eoi status for a cascaded event
from a stacked irqchip should be robust wrt this case too.

To achieve this, we could track the deferral status of irq events,
i.e. whether we expect them to be replayed in-band, instead of tracking
their acknowledge status on pipeline entry - this would disambiguate
some logic. We'd also need to track the state of the interrupt flow in a
more elaborate way than Dovetail currently implements. I'll follow up
with a patch illustrating this.

-- 
Philippe.

[PATCH] pipeline: irq: ack the irq when it is postponed inband irq from outside of the pipeline entry path.

[Hongzhan Chen]

1. To avoid double ack for the interrupt that already acked in
   pipeline entry path, we add new flag to note such status and
   differentiate.
2. For postponed stacked edge irq that is handling outside of the
   pipeline entry path, as discussed in [1], ack the irq directly
   because it would never be acked in the pipeline entry path.

[1]: https://lore.kernel.org/xenomai/BY5PR11MB4276FEFA2C6D88C4B6CFFEFCF2E9A@BY5PR11MB4276.namprd11.prod.outlook.com/T/#t

Signed-off-by: Hongzhan Chen <hongzhan.chen@intel.com>

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 6aca6c036ada..b8d8aba38e17 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -862,15 +862,25 @@ void handle_edge_irq(struct irq_desc *desc)
 	if (on_pipeline_entry()) {
 		chip->irq_ack(&desc->irq_data);
 		desc->istate |= IRQS_EDGE;
-		handle_oob_irq(desc);
+		if (!handle_oob_irq(desc)) {
+
+			/* For inband irq, we tag acked status
+			 * to avoid double ack
+			 */
+			if (!oob_stage_present() || !irq_settings_is_oob(desc))
+				desc->istate |= IRQS_ACK;
+		}
+
 		goto out_unlock;
 	}
 
 	kstat_incr_irqs_this_cpu(desc);
 
 	/* Start handling the irq */
-	if (!irqs_pipelined())
+	if (!(desc->istate & IRQS_ACK))
 		chip->irq_ack(&desc->irq_data);
+	else
+		desc->istate &= ~IRQS_ACK;
 
 	do {
 		if (unlikely(!desc->action)) {
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index a9121f6e6f1c..1b43467b5407 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -54,6 +54,7 @@ enum {
  * IRQS_NMI			- irq line is used to deliver NMIs
  * IRQS_SYSFS			- descriptor has been added to sysfs
  * IRQS_EDGE			- irq line received an edge event
+ * IRQS_ACK			- acked irq line
  */
 enum {
 	IRQS_AUTODETECT		= 0x00000001,
@@ -68,6 +69,7 @@ enum {
 	IRQS_NMI		= 0x00002000,
 	IRQS_SYSFS		= 0x00004000,
 	IRQS_EDGE		= 0x00008000,
+	IRQS_ACK		= 0x00010000,
 };
 
 #include "debug.h"
-- 
2.17.1