[PATCH] PCI: dra7xx: mark dra7xx_pcie_msi irq as IRQF_NO_THREAD

[PATCH] PCI: dra7xx: mark dra7xx_pcie_msi irq as IRQF_NO_THREAD

[Grygorii Strashko]

On -RT, TI DRA7 PCIe driver always produces below backtrace when the
first PCI interrupt is triggered:

------------[ cut here ]------------
WARNING: CPU: 1 PID: 82 at kernel/irq/handle.c:150 handle_irq_event_percpu+0x14c/0x174()
irq 460 handler irq_default_primary_handler+0x0/0x14 enabled interrupts
Modules linked in: ahci_platform(+) ti_vip(+) libahci_platform xhci_pci(+) c_can_platform(+)
libahci xhci_hcd ti_vpe c_can libata v4l2_mem2mem can_dev gpio_keys leds_gpio
snd_soc_simple_card usbcore spi_ti_qspi videobuf2_core extcon_usb_gpio omap_wdt
ti_vpdma videobuf2_dma_contig ti_soc_thermal videobuf2_memops dwc3_omap extcon
rtc_omap ov1063x v4l2_common videodev media snd_soc_tlv320aic3x omap_rng rng_core omap_remoteproc
CPU: 1 PID: 82 Comm: irq/26-dra7-pci Not tainted 4.1.10-rt10-02638-g6486d7e-dirty #1
Hardware name: Generic DRA74X (Flattened Device Tree)
Backtrace:
[<c0013060>] (dump_backtrace) from [<c0013280>] (show_stack+0x18/0x1c)
 r7:c07acca0 r6:00000000 r5:c09034e4 r4:00000000
[<c0013268>] (show_stack) from [<c061ef14>] (dump_stack+0x90/0xac)
[<c061ee84>] (dump_stack) from [<c00436e4>] (warn_slowpath_common+0x7c/0xb8)
 r7:c07acca0 r6:00000096 r5:00000009 r4:ee4d3e38
[<c0043668>] (warn_slowpath_common) from [<c0043758>] (warn_slowpath_fmt+0x38/0x40)
 r8:ee3fcc00 r7:000001cc r6:00000000 r5:00000002 r4:c07acc78
[<c0043724>] (warn_slowpath_fmt) from [<c0085fd0>] (handle_irq_event_percpu+0x14c/0x174)
 r3:000001cc r2:c07acc78
 r4:ecfcdd80
[<c0085e84>] (handle_irq_event_percpu) from [<c008607c>] (handle_irq_event+0x84/0xb8)
 r10:00000001 r9:ee4b59c0 r8:ee1d0900 r7:00000001 r6:00000000 r5:ecfcdd80
 r4:ee3fcc00
[<c0085ff8>] (handle_irq_event) from [<c0089240>] (handle_simple_irq+0x90/0x118)
 r5:ee4a9020 r4:ee3fcc00
[<c00891b0>] (handle_simple_irq) from [<c0085514>] (generic_handle_irq+0x30/0x44)
 r5:ee4a9020 r4:000001cc
[<c00854e4>] (generic_handle_irq) from [<c034a7d4>] (dra7xx_pcie_msi_irq_handler+0x7c/0x8c)
 r5:ee4a9020 r4:00000001
[<c034a758>] (dra7xx_pcie_msi_irq_handler) from [<c0086f08>] (irq_forced_thread_fn+0x28/0x5c)
 r5:ee1d0900 r4:ee4b59c0
[<c0086ee0>] (irq_forced_thread_fn) from [<c00871dc>] (irq_thread+0x128/0x204)
 r7:00000001 r6:00000000 r5:ee4d2000 r4:ee4b59e4
[<c00870b4>] (irq_thread) from [<c005e2fc>] (kthread+0xd4/0xec)
 r10:00000000 r9:00000000 r8:00000000 r7:c00870b4 r6:ee4b59c0 r5:ee4b5a00
 r4:00000000
[<c005e228>] (kthread) from [<c000faa8>] (ret_from_fork+0x14/0x2c)
 r7:00000000 r6:00000000 r5:c005e228 r4:ee4b5a00
---[ end trace 0000000000000002 ]---

This backtrace is triggered because dra7xx_pcie_msi_irq_handler()
forced to be threaded by default on -RT but, in the same time, it's
IRQ dispatcher and calls generic_handle_irq(), which leads to
handle_simple_irq() call. The handle_simple_irq() expected to be
called with IRQ disabled.

The same issue will also happen if kernel will boot with "threadirqs"
cmdline parameter (CONFIG_IRQ_FORCED_THREADING).

As discussed in [1], the current DRA7 PCIe hw configuration supports
32 interrupts, which cannot change because it's hardwired in silicon,
this is a single status read and assuming that only a few (most of the
time it will be exactly ONE) of those interrupts are pending at the
same time is pretty much a sane assumption. And recommended fix for
this issue is to request dra7xx_pcie_msi IRQ with IRQF_NO_THREAD flag.

[1] https://lkml.org/lkml/2015/11/3/660

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/pci/host/pci-dra7xx.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
index 6589e7e..31460f4 100644
--- a/drivers/pci/host/pci-dra7xx.c
+++ b/drivers/pci/host/pci-dra7xx.c
@@ -302,8 +302,30 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx,
 		return -EINVAL;
 	}
 
+	/*
+	 * Mark dra7xx_pcie_msi IRQ as IRQF_NO_THREAD
+	 * On -RT and if kernel is booting with "threadirqs" cmd line parameter
+	 * the dra7xx_pcie_msi_irq_handler() will be forced threaded but,
+	 * in the same time, it's IRQ dispatcher and calls generic_handle_irq(),
+	 * which, in turn, will be resolved to handle_simple_irq() call.
+	 * The handle_simple_irq() expected to be called with IRQ disabled, as
+	 * result kernle will display warning:
+	 * "irq XXX handler YYY+0x0/0x14 enabled interrupts".
+	 *
+	 * Current DRA7 PCIe hw configuration supports 32 interrupts,
+	 * which cannot change because it's hardwired in silicon, and can assume
+	 * that only a few (most of the time it will be exactly ONE) of those
+	 * interrupts are pending at the same time. So, It's sane way to dial
+	 * with above issue by marking dra7xx_pcie_msi IRQ as IRQF_NO_THREAD.
+	 * if some platform will utilize a lot of MSI IRQs and will suffer
+	 * form -RT latencies degradation because of that - IRQF_NO_THREAD can
+	 * be removed and "Warning Annihilation" W/A can be applied [1]
+	 *
+	 * [1] https://lkml.org/lkml/2015/11/2/578
+	 */
 	ret = devm_request_irq(&pdev->dev, pp->irq,
-			       dra7xx_pcie_msi_irq_handler, IRQF_SHARED,
+			       dra7xx_pcie_msi_irq_handler,
+			       IRQF_SHARED | IRQF_NO_THREAD,
 			       "dra7-pcie-msi",	pp);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to request irq\n");
-- 
2.6.2

[PATCH] PCI: dra7xx: mark dra7xx_pcie_msi irq as IRQF_NO_THREAD

[Grygorii Strashko]

On -RT, TI DRA7 PCIe driver always produces below backtrace when the
first PCI interrupt is triggered:

------------[ cut here ]------------
WARNING: CPU: 1 PID: 82 at kernel/irq/handle.c:150 handle_irq_event_percpu+0x14c/0x174()
irq 460 handler irq_default_primary_handler+0x0/0x14 enabled interrupts
Modules linked in: ahci_platform(+) ti_vip(+) libahci_platform xhci_pci(+) c_can_platform(+)
libahci xhci_hcd ti_vpe c_can libata v4l2_mem2mem can_dev gpio_keys leds_gpio
snd_soc_simple_card usbcore spi_ti_qspi videobuf2_core extcon_usb_gpio omap_wdt
ti_vpdma videobuf2_dma_contig ti_soc_thermal videobuf2_memops dwc3_omap extcon
rtc_omap ov1063x v4l2_common videodev media snd_soc_tlv320aic3x omap_rng rng_core omap_remoteproc
CPU: 1 PID: 82 Comm: irq/26-dra7-pci Not tainted 4.1.10-rt10-02638-g6486d7e-dirty #1
Hardware name: Generic DRA74X (Flattened Device Tree)
Backtrace:
[<c0013060>] (dump_backtrace) from [<c0013280>] (show_stack+0x18/0x1c)
 r7:c07acca0 r6:00000000 r5:c09034e4 r4:00000000
[<c0013268>] (show_stack) from [<c061ef14>] (dump_stack+0x90/0xac)
[<c061ee84>] (dump_stack) from [<c00436e4>] (warn_slowpath_common+0x7c/0xb8)
 r7:c07acca0 r6:00000096 r5:00000009 r4:ee4d3e38
[<c0043668>] (warn_slowpath_common) from [<c0043758>] (warn_slowpath_fmt+0x38/0x40)
 r8:ee3fcc00 r7:000001cc r6:00000000 r5:00000002 r4:c07acc78
[<c0043724>] (warn_slowpath_fmt) from [<c0085fd0>] (handle_irq_event_percpu+0x14c/0x174)
 r3:000001cc r2:c07acc78
 r4:ecfcdd80
[<c0085e84>] (handle_irq_event_percpu) from [<c008607c>] (handle_irq_event+0x84/0xb8)
 r10:00000001 r9:ee4b59c0 r8:ee1d0900 r7:00000001 r6:00000000 r5:ecfcdd80
 r4:ee3fcc00
[<c0085ff8>] (handle_irq_event) from [<c0089240>] (handle_simple_irq+0x90/0x118)
 r5:ee4a9020 r4:ee3fcc00
[<c00891b0>] (handle_simple_irq) from [<c0085514>] (generic_handle_irq+0x30/0x44)
 r5:ee4a9020 r4:000001cc
[<c00854e4>] (generic_handle_irq) from [<c034a7d4>] (dra7xx_pcie_msi_irq_handler+0x7c/0x8c)
 r5:ee4a9020 r4:00000001
[<c034a758>] (dra7xx_pcie_msi_irq_handler) from [<c0086f08>] (irq_forced_thread_fn+0x28/0x5c)
 r5:ee1d0900 r4:ee4b59c0
[<c0086ee0>] (irq_forced_thread_fn) from [<c00871dc>] (irq_thread+0x128/0x204)
 r7:00000001 r6:00000000 r5:ee4d2000 r4:ee4b59e4
[<c00870b4>] (irq_thread) from [<c005e2fc>] (kthread+0xd4/0xec)
 r10:00000000 r9:00000000 r8:00000000 r7:c00870b4 r6:ee4b59c0 r5:ee4b5a00
 r4:00000000
[<c005e228>] (kthread) from [<c000faa8>] (ret_from_fork+0x14/0x2c)
 r7:00000000 r6:00000000 r5:c005e228 r4:ee4b5a00
---[ end trace 0000000000000002 ]---

This backtrace is triggered because dra7xx_pcie_msi_irq_handler()
forced to be threaded by default on -RT but, in the same time, it's
IRQ dispatcher and calls generic_handle_irq(), which leads to
handle_simple_irq() call. The handle_simple_irq() expected to be
called with IRQ disabled.

The same issue will also happen if kernel will boot with "threadirqs"
cmdline parameter (CONFIG_IRQ_FORCED_THREADING).

As discussed in [1], the current DRA7 PCIe hw configuration supports
32 interrupts, which cannot change because it's hardwired in silicon,
this is a single status read and assuming that only a few (most of the
time it will be exactly ONE) of those interrupts are pending at the
same time is pretty much a sane assumption. And recommended fix for
this issue is to request dra7xx_pcie_msi IRQ with IRQF_NO_THREAD flag.

[1] https://lkml.org/lkml/2015/11/3/660

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/pci/host/pci-dra7xx.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
index 6589e7e..31460f4 100644
--- a/drivers/pci/host/pci-dra7xx.c
+++ b/drivers/pci/host/pci-dra7xx.c
@@ -302,8 +302,30 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx,
 		return -EINVAL;
 	}
 
+	/*
+	 * Mark dra7xx_pcie_msi IRQ as IRQF_NO_THREAD
+	 * On -RT and if kernel is booting with "threadirqs" cmd line parameter
+	 * the dra7xx_pcie_msi_irq_handler() will be forced threaded but,
+	 * in the same time, it's IRQ dispatcher and calls generic_handle_irq(),
+	 * which, in turn, will be resolved to handle_simple_irq() call.
+	 * The handle_simple_irq() expected to be called with IRQ disabled, as
+	 * result kernle will display warning:
+	 * "irq XXX handler YYY+0x0/0x14 enabled interrupts".
+	 *
+	 * Current DRA7 PCIe hw configuration supports 32 interrupts,
+	 * which cannot change because it's hardwired in silicon, and can assume
+	 * that only a few (most of the time it will be exactly ONE) of those
+	 * interrupts are pending at the same time. So, It's sane way to dial
+	 * with above issue by marking dra7xx_pcie_msi IRQ as IRQF_NO_THREAD.
+	 * if some platform will utilize a lot of MSI IRQs and will suffer
+	 * form -RT latencies degradation because of that - IRQF_NO_THREAD can
+	 * be removed and "Warning Annihilation" W/A can be applied [1]
+	 *
+	 * [1] https://lkml.org/lkml/2015/11/2/578
+	 */
 	ret = devm_request_irq(&pdev->dev, pp->irq,
-			       dra7xx_pcie_msi_irq_handler, IRQF_SHARED,
+			       dra7xx_pcie_msi_irq_handler,
+			       IRQF_SHARED | IRQF_NO_THREAD,
 			       "dra7-pcie-msi",	pp);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to request irq\n");
-- 
2.6.2

Re: [PATCH] PCI: dra7xx: mark dra7xx_pcie_msi irq as IRQF_NO_THREAD

[Sebastian Andrzej Siewior]

On 11/05/2015 07:43 PM, Grygorii Strashko wrote:
> diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
> index 6589e7e..31460f4 100644
> --- a/drivers/pci/host/pci-dra7xx.c
> +++ b/drivers/pci/host/pci-dra7xx.c
> @@ -302,8 +302,30 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx,
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * Mark dra7xx_pcie_msi IRQ as IRQF_NO_THREAD
> +	 * On -RT and if kernel is booting with "threadirqs" cmd line parameter
> +	 * the dra7xx_pcie_msi_irq_handler() will be forced threaded but,
> +	 * in the same time, it's IRQ dispatcher and calls generic_handle_irq(),
> +	 * which, in turn, will be resolved to handle_simple_irq() call.
> +	 * The handle_simple_irq() expected to be called with IRQ disabled, as
> +	 * result kernle will display warning:
> +	 * "irq XXX handler YYY+0x0/0x14 enabled interrupts".
> +	 *
> +	 * Current DRA7 PCIe hw configuration supports 32 interrupts,
> +	 * which cannot change because it's hardwired in silicon, and can assume
> +	 * that only a few (most of the time it will be exactly ONE) of those
> +	 * interrupts are pending at the same time. So, It's sane way to dial
> +	 * with above issue by marking dra7xx_pcie_msi IRQ as IRQF_NO_THREAD.
> +	 * if some platform will utilize a lot of MSI IRQs and will suffer
> +	 * form -RT latencies degradation because of that - IRQF_NO_THREAD can
> +	 * be removed and "Warning Annihilation" W/A can be applied [1]
> +	 *
> +	 * [1] https://lkml.org/lkml/2015/11/2/578

I don't think this novel is required. Also a reference to a patch which
was not accepted is not a smart idea (since people might think they
will improve their -RT latency by applying it without thinking).

Do you have any *real* numbers where you can say this patch does
better/worse than what you suggester earlier? I'm simply asking because
each gpio-irq multiplexing driver does the same thing.

> +	 */
>  	ret = devm_request_irq(&pdev->dev, pp->irq,
> -			       dra7xx_pcie_msi_irq_handler, IRQF_SHARED,
> +			       dra7xx_pcie_msi_irq_handler,
> +			       IRQF_SHARED | IRQF_NO_THREAD,
>  			       "dra7-pcie-msi",	pp);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to request irq\n");
> 
Sebastian

Re: [PATCH] PCI: dra7xx: mark dra7xx_pcie_msi irq as IRQF_NO_THREAD

[Grygorii Strashko]

Hi Sebastian,

On 11/06/2015 10:53 AM, Sebastian Andrzej Siewior wrote:
> On 11/05/2015 07:43 PM, Grygorii Strashko wrote:
>> diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
>> index 6589e7e..31460f4 100644
>> --- a/drivers/pci/host/pci-dra7xx.c
>> +++ b/drivers/pci/host/pci-dra7xx.c
>> @@ -302,8 +302,30 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx,
>>   		return -EINVAL;
>>   	}
>>   
>> +	/*
>> +	 * Mark dra7xx_pcie_msi IRQ as IRQF_NO_THREAD
>> +	 * On -RT and if kernel is booting with "threadirqs" cmd line parameter
>> +	 * the dra7xx_pcie_msi_irq_handler() will be forced threaded but,
>> +	 * in the same time, it's IRQ dispatcher and calls generic_handle_irq(),
>> +	 * which, in turn, will be resolved to handle_simple_irq() call.
>> +	 * The handle_simple_irq() expected to be called with IRQ disabled, as
>> +	 * result kernle will display warning:
>> +	 * "irq XXX handler YYY+0x0/0x14 enabled interrupts".
>> +	 *
>> +	 * Current DRA7 PCIe hw configuration supports 32 interrupts,
>> +	 * which cannot change because it's hardwired in silicon, and can assume
>> +	 * that only a few (most of the time it will be exactly ONE) of those
>> +	 * interrupts are pending at the same time. So, It's sane way to dial
>> +	 * with above issue by marking dra7xx_pcie_msi IRQ as IRQF_NO_THREAD.

I can remove below text.

>> +	 * if some platform will utilize a lot of MSI IRQs and will suffer
>> +	 * form -RT latencies degradation because of that - IRQF_NO_THREAD can
>> +	 * be removed and "Warning Annihilation" W/A can be applied [1]
>> +	 *
>> +	 * [1] https://lkml.org/lkml/2015/11/2/578
> 
> I don't think this novel is required. Also a reference to a patch which
> was not accepted is not a smart idea (since people might think they
> will improve their -RT latency by applying it without thinking).

Agree. Honestly, I don't like both these "solution"/W/A... :(
but have nothing better in mind.

> 
> Do you have any *real* numbers where you can say this patch does
> better/worse than what you suggester earlier? I'm simply asking because
> each gpio-irq multiplexing driver does the same thing.
> 

Indeed. And not only gpio :( Below you can find list of files which
contain "generic_handle_irq" and do not contain "irq_set_chained_handler"
(of course this list is inaccurate). [Addendum 1]

Regarding *real* numbers, I've tested it only on TI dra7-evm + pcie USB3 expander card
which generates legacy IRQ every 1 ms and didn't observe latency changes.
I'm not sure that I'd be able to test any scenario soon (next week), which will be
close to the potential "worst" case. Plus, I found acceptable assumption, that only
few pci irqs might be pending at the same time (at least for the reference Kernel code).
That's why I gave up without fight :(

Seems, I need explain why I'm trying to avoid using IRQF_NO_THREAD by all means
(I'm sorry, if below may sound dummy, and may be I'm overestimating/demonizing problem -
 my -RT experience is not really good [yet:)]).

- IRQF_NO_THREAD is the first considered option for such kind of issues.
  But: Now in LKML there are ~60 occurrences of IRQF_NO_THREAD - most of
  them are used by Arch code. And It's only used by 6 drivers (drivers/*) [Addendum 2].
  During past year, I've found only two threads related to IRQF_NO_THREAD
  and, in both cases, IRQF_NO_THREAD was added for arch specific IRQs which
  can't be threaded (https://lists.ozlabs.org/pipermail/linuxppc-dev/2014-November/122659.html,
  https://lkml.org/lkml/2015/4/21/404).
  
- ARM UP system: TI's am437xx SoCs for example.
   Here everything is started from /drivers/irqchip/irq-gic.c -> gic_handle_irq()

  static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
  {
	u32 irqstat, irqnr;
	struct gic_chip_data *gic = &gic_data[0];
	void __iomem *cpu_base = gic_data_cpu_base(gic);

	do {
		irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
		irqnr = irqstat & GICC_IAR_INT_ID_MASK;

		if (likely(irqnr > 15 && irqnr < 1021)) {
			if (static_key_true(&supports_deactivate))
				writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
			handle_domain_irq(gic->domain, irqnr, regs);
			|- struct pt_regs *old_regs = set_irq_regs(regs);
*			|- irq_enter();

			|- generic_handle_irq(irq);
[1]			   //*** -RT: all IRQ are threaded (except SPI, timers,..) ***
			   |- handle_fasteoi_irq()
			      |- irq_default_primary_handler()
			      |- _irq_wake_thread(desc, action);

[2]			   //***  chained irqX ***
			   chained_irq_handler
			   |- chained_irq_enter(chip, desc);
			   |- handle IRQX.1
				|- generic_handle_irq(IRQX.1);
				   ...
			   |- handle IRQX.2
			   |- handle IRQX.n
			   |- chained_irq_exit(chip, desc);

[3]			   //*** IRQF_NO_THREAD irqY ***
			   |- handle_fasteoi_irq()
				|- driverY_hw_irq_handler()
				   |- handle IRQY.1
					|- generic_handle_irq(IRQY.1);
					...
			           |- handle IRQY.2
				   |- handle IRQY.n

*			|- irq_exit();
			|- set_irq_regs(old_regs);
			continue;
		}
		if (irqnr < 16) {
			writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
			if (static_key_true(&supports_deactivate))
				writel_relaxed(irqstat, cpu_base + GIC_CPU_DEACTIVATE);
#ifdef CONFIG_SMP
			handle_IPI(irqnr, regs);
#endif
			continue;
		}
		break;
	} while (1);
  }

GIC IRQ handler gic_handle_irq() may process more than one IRQ without leaving HW IRQ mode
(during my experiments I saw up to 6 IRQs processed in one cycle).

As result, It was concluded, that having such current HW/SW and all IRQs forced threaded [1],
it will be potentially possible to predict system behavior, because gic_handle_irq() will
do the same things for most of processed IRQs.
But, once there will be chained [2] or IRQF_NO_THREAD [3] IRQs - complete unpredictability.

So, It was selected as goal to have all PPI IRQs (forced) threaded. And if someone
will require faster IRQ handling - IRQF_NO_THREAD can be always added, but it will
be custom solution then. 

I'd be appreciated for your comments - if above problem is not a problem.
Good - IRQF_NO_THREAD forever!

Thanks.
-- 
regards,
-grygorii


=== Addendum 1
x0174654@KBP1-T01-F55654:~/kernel.org/linux-master/linux$ grep -L irq_set_chained_handler $(grep -r -I -l generic_handle_irq drivers/*)
drivers/bcma/driver_gpio.c
drivers/clk/at91/pmc.c
drivers/gpio/gpio-lynxpoint.c
drivers/gpio/gpio-brcmstb.c
drivers/gpio/gpio-grgpio.c
drivers/gpio/gpio-etraxfs.c
drivers/gpio/gpio-omap.c
drivers/gpio/gpio-ml-ioh.c
drivers/gpio/gpio-dln2.c
drivers/gpio/gpio-rcar.c
drivers/gpio/gpio-pch.c
drivers/gpio/gpio-zx.c
drivers/gpio/gpio-tb10x.c
drivers/gpio/gpio-em.c
drivers/gpio/gpio-altera.c
drivers/gpio/gpio-omap.mod.c
drivers/gpio/gpio-zynq.c
drivers/gpio/gpio-pl061.c
drivers/gpio/gpio-vf610.c
drivers/gpio/gpio-xlp.c
drivers/gpio/gpio-intel-mid.c
drivers/gpio/gpio-sodaville.c
drivers/gpio/gpio-sta2x11.c
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
drivers/iio/industrialio-trigger.c
drivers/irqchip/irq-renesas-irqc.c
drivers/irqchip/irq-ingenic.c
drivers/irqchip/irq-renesas-intc-irqpin.c
drivers/memory/omap-gpmc.c
drivers/mfd/db8500-prcmu.c
drivers/mfd/htc-i2cpld.c
drivers/parisc/superio.c
drivers/parisc/gsc.c
drivers/parisc/eisa.c
drivers/parisc/dino.c
drivers/pci/host/pcie-xilinx.c
drivers/pci/host/pci-keystone-dw.c
drivers/pci/host/pcie-rcar.c
drivers/pci/host/pci-dra7xx.c
drivers/pci/host/pcie-designware.c
drivers/pci/host/pci-tegra.c
drivers/pinctrl/qcom/pinctrl-msm.c
drivers/pinctrl/pinctrl-amd.c
drivers/pinctrl/samsung/pinctrl-exynos5440.c
drivers/pinctrl/nomadik/pinctrl-nomadik.c
drivers/pinctrl/sirf/pinctrl-atlas7.c
drivers/pinctrl/sirf/pinctrl-sirf.c
drivers/pinctrl/spear/pinctrl-plgpio.c
drivers/pinctrl/pinctrl-at91.c
drivers/pinctrl/bcm/pinctrl-bcm2835.c
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c
drivers/pinctrl/intel/pinctrl-cherryview.c
drivers/pinctrl/intel/pinctrl-baytrail.c
drivers/pinctrl/intel/pinctrl-intel.c
drivers/pinctrl/pinctrl-pistachio.c
drivers/pinctrl/pinctrl-coh901.c
drivers/platform/x86/intel_pmic_gpio.c
drivers/ssb/driver_gpio.c
drivers/xen/events/events_2l.c
drivers/xen/events/events_fifo.c

=== Addendum 2
dmar.c
irq-i8259.c
arm_pmu.c
pinctrl-single.c
mips_ejtag_fdc.c (2 matches)
octeon-wdt-main.c

Re: [PATCH] PCI: dra7xx: mark dra7xx_pcie_msi irq as IRQF_NO_THREAD

[Grygorii Strashko]

Hi Sebastian,

On 11/06/2015 10:53 AM, Sebastian Andrzej Siewior wrote:
> On 11/05/2015 07:43 PM, Grygorii Strashko wrote:
>> diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
>> index 6589e7e..31460f4 100644
>> --- a/drivers/pci/host/pci-dra7xx.c
>> +++ b/drivers/pci/host/pci-dra7xx.c
>> @@ -302,8 +302,30 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx,
>>   		return -EINVAL;
>>   	}
>>   
>> +	/*
>> +	 * Mark dra7xx_pcie_msi IRQ as IRQF_NO_THREAD
>> +	 * On -RT and if kernel is booting with "threadirqs" cmd line parameter
>> +	 * the dra7xx_pcie_msi_irq_handler() will be forced threaded but,
>> +	 * in the same time, it's IRQ dispatcher and calls generic_handle_irq(),
>> +	 * which, in turn, will be resolved to handle_simple_irq() call.
>> +	 * The handle_simple_irq() expected to be called with IRQ disabled, as
>> +	 * result kernle will display warning:
>> +	 * "irq XXX handler YYY+0x0/0x14 enabled interrupts".
>> +	 *
>> +	 * Current DRA7 PCIe hw configuration supports 32 interrupts,
>> +	 * which cannot change because it's hardwired in silicon, and can assume
>> +	 * that only a few (most of the time it will be exactly ONE) of those
>> +	 * interrupts are pending at the same time. So, It's sane way to dial
>> +	 * with above issue by marking dra7xx_pcie_msi IRQ as IRQF_NO_THREAD.

I can remove below text.

>> +	 * if some platform will utilize a lot of MSI IRQs and will suffer
>> +	 * form -RT latencies degradation because of that - IRQF_NO_THREAD can
>> +	 * be removed and "Warning Annihilation" W/A can be applied [1]
>> +	 *
>> +	 * [1] https://lkml.org/lkml/2015/11/2/578
> 
> I don't think this novel is required. Also a reference to a patch which
> was not accepted is not a smart idea (since people might think they
> will improve their -RT latency by applying it without thinking).

Agree. Honestly, I don't like both these "solution"/W/A... :(
but have nothing better in mind.

> 
> Do you have any *real* numbers where you can say this patch does
> better/worse than what you suggester earlier? I'm simply asking because
> each gpio-irq multiplexing driver does the same thing.
> 

Indeed. And not only gpio :( Below you can find list of files which
contain "generic_handle_irq" and do not contain "irq_set_chained_handler"
(of course this list is inaccurate). [Addendum 1]

Regarding *real* numbers, I've tested it only on TI dra7-evm + pcie USB3 expander card
which generates legacy IRQ every 1 ms and didn't observe latency changes.
I'm not sure that I'd be able to test any scenario soon (next week), which will be
close to the potential "worst" case. Plus, I found acceptable assumption, that only
few pci irqs might be pending at the same time (at least for the reference Kernel code).
That's why I gave up without fight :(

Seems, I need explain why I'm trying to avoid using IRQF_NO_THREAD by all means
(I'm sorry, if below may sound dummy, and may be I'm overestimating/demonizing problem -
 my -RT experience is not really good [yet:)]).

- IRQF_NO_THREAD is the first considered option for such kind of issues.
  But: Now in LKML there are ~60 occurrences of IRQF_NO_THREAD - most of
  them are used by Arch code. And It's only used by 6 drivers (drivers/*) [Addendum 2].
  During past year, I've found only two threads related to IRQF_NO_THREAD
  and, in both cases, IRQF_NO_THREAD was added for arch specific IRQs which
  can't be threaded (https://lists.ozlabs.org/pipermail/linuxppc-dev/2014-November/122659.html,
  https://lkml.org/lkml/2015/4/21/404).
  
- ARM UP system: TI's am437xx SoCs for example.
   Here everything is started from /drivers/irqchip/irq-gic.c -> gic_handle_irq()

  static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
  {
	u32 irqstat, irqnr;
	struct gic_chip_data *gic = &gic_data[0];
	void __iomem *cpu_base = gic_data_cpu_base(gic);

	do {
		irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
		irqnr = irqstat & GICC_IAR_INT_ID_MASK;

		if (likely(irqnr > 15 && irqnr < 1021)) {
			if (static_key_true(&supports_deactivate))
				writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
			handle_domain_irq(gic->domain, irqnr, regs);
			|- struct pt_regs *old_regs = set_irq_regs(regs);
*			|- irq_enter();

			|- generic_handle_irq(irq);
[1]			   //*** -RT: all IRQ are threaded (except SPI, timers,..) ***
			   |- handle_fasteoi_irq()
			      |- irq_default_primary_handler()
			      |- _irq_wake_thread(desc, action);

[2]			   //***  chained irqX ***
			   chained_irq_handler
			   |- chained_irq_enter(chip, desc);
			   |- handle IRQX.1
				|- generic_handle_irq(IRQX.1);
				   ...
			   |- handle IRQX.2
			   |- handle IRQX.n
			   |- chained_irq_exit(chip, desc);

[3]			   //*** IRQF_NO_THREAD irqY ***
			   |- handle_fasteoi_irq()
				|- driverY_hw_irq_handler()
				   |- handle IRQY.1
					|- generic_handle_irq(IRQY.1);
					...
			           |- handle IRQY.2
				   |- handle IRQY.n

*			|- irq_exit();
			|- set_irq_regs(old_regs);
			continue;
		}
		if (irqnr < 16) {
			writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
			if (static_key_true(&supports_deactivate))
				writel_relaxed(irqstat, cpu_base + GIC_CPU_DEACTIVATE);
#ifdef CONFIG_SMP
			handle_IPI(irqnr, regs);
#endif
			continue;
		}
		break;
	} while (1);
  }

GIC IRQ handler gic_handle_irq() may process more than one IRQ without leaving HW IRQ mode
(during my experiments I saw up to 6 IRQs processed in one cycle).

As result, It was concluded, that having such current HW/SW and all IRQs forced threaded [1],
it will be potentially possible to predict system behavior, because gic_handle_irq() will
do the same things for most of processed IRQs.
But, once there will be chained [2] or IRQF_NO_THREAD [3] IRQs - complete unpredictability.

So, It was selected as goal to have all PPI IRQs (forced) threaded. And if someone
will require faster IRQ handling - IRQF_NO_THREAD can be always added, but it will
be custom solution then. 

I'd be appreciated for your comments - if above problem is not a problem.
Good - IRQF_NO_THREAD forever!

Thanks.
-- 
regards,
-grygorii


=== Addendum 1
x0174654@KBP1-T01-F55654:~/kernel.org/linux-master/linux$ grep -L irq_set_chained_handler $(grep -r -I -l generic_handle_irq drivers/*)
drivers/bcma/driver_gpio.c
drivers/clk/at91/pmc.c
drivers/gpio/gpio-lynxpoint.c
drivers/gpio/gpio-brcmstb.c
drivers/gpio/gpio-grgpio.c
drivers/gpio/gpio-etraxfs.c
drivers/gpio/gpio-omap.c
drivers/gpio/gpio-ml-ioh.c
drivers/gpio/gpio-dln2.c
drivers/gpio/gpio-rcar.c
drivers/gpio/gpio-pch.c
drivers/gpio/gpio-zx.c
drivers/gpio/gpio-tb10x.c
drivers/gpio/gpio-em.c
drivers/gpio/gpio-altera.c
drivers/gpio/gpio-omap.mod.c
drivers/gpio/gpio-zynq.c
drivers/gpio/gpio-pl061.c
drivers/gpio/gpio-vf610.c
drivers/gpio/gpio-xlp.c
drivers/gpio/gpio-intel-mid.c
drivers/gpio/gpio-sodaville.c
drivers/gpio/gpio-sta2x11.c
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
drivers/iio/industrialio-trigger.c
drivers/irqchip/irq-renesas-irqc.c
drivers/irqchip/irq-ingenic.c
drivers/irqchip/irq-renesas-intc-irqpin.c
drivers/memory/omap-gpmc.c
drivers/mfd/db8500-prcmu.c
drivers/mfd/htc-i2cpld.c
drivers/parisc/superio.c
drivers/parisc/gsc.c
drivers/parisc/eisa.c
drivers/parisc/dino.c
drivers/pci/host/pcie-xilinx.c
drivers/pci/host/pci-keystone-dw.c
drivers/pci/host/pcie-rcar.c
drivers/pci/host/pci-dra7xx.c
drivers/pci/host/pcie-designware.c
drivers/pci/host/pci-tegra.c
drivers/pinctrl/qcom/pinctrl-msm.c
drivers/pinctrl/pinctrl-amd.c
drivers/pinctrl/samsung/pinctrl-exynos5440.c
drivers/pinctrl/nomadik/pinctrl-nomadik.c
drivers/pinctrl/sirf/pinctrl-atlas7.c
drivers/pinctrl/sirf/pinctrl-sirf.c
drivers/pinctrl/spear/pinctrl-plgpio.c
drivers/pinctrl/pinctrl-at91.c
drivers/pinctrl/bcm/pinctrl-bcm2835.c
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c
drivers/pinctrl/intel/pinctrl-cherryview.c
drivers/pinctrl/intel/pinctrl-baytrail.c
drivers/pinctrl/intel/pinctrl-intel.c
drivers/pinctrl/pinctrl-pistachio.c
drivers/pinctrl/pinctrl-coh901.c
drivers/platform/x86/intel_pmic_gpio.c
drivers/ssb/driver_gpio.c
drivers/xen/events/events_2l.c
drivers/xen/events/events_fifo.c

=== Addendum 2
dmar.c
irq-i8259.c
arm_pmu.c
pinctrl-single.c
mips_ejtag_fdc.c (2 matches)
octeon-wdt-main.c

[PATCH] PCI: dra7xx: mark dra7xx_pcie_msi irq as IRQF_NO_THREAD

[Grygorii Strashko]

Hi Sebastian,

On 11/06/2015 10:53 AM, Sebastian Andrzej Siewior wrote:
> On 11/05/2015 07:43 PM, Grygorii Strashko wrote:
>> diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
>> index 6589e7e..31460f4 100644
>> --- a/drivers/pci/host/pci-dra7xx.c
>> +++ b/drivers/pci/host/pci-dra7xx.c
>> @@ -302,8 +302,30 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx,
>>   		return -EINVAL;
>>   	}
>>   
>> +	/*
>> +	 * Mark dra7xx_pcie_msi IRQ as IRQF_NO_THREAD
>> +	 * On -RT and if kernel is booting with "threadirqs" cmd line parameter
>> +	 * the dra7xx_pcie_msi_irq_handler() will be forced threaded but,
>> +	 * in the same time, it's IRQ dispatcher and calls generic_handle_irq(),
>> +	 * which, in turn, will be resolved to handle_simple_irq() call.
>> +	 * The handle_simple_irq() expected to be called with IRQ disabled, as
>> +	 * result kernle will display warning:
>> +	 * "irq XXX handler YYY+0x0/0x14 enabled interrupts".
>> +	 *
>> +	 * Current DRA7 PCIe hw configuration supports 32 interrupts,
>> +	 * which cannot change because it's hardwired in silicon, and can assume
>> +	 * that only a few (most of the time it will be exactly ONE) of those
>> +	 * interrupts are pending at the same time. So, It's sane way to dial
>> +	 * with above issue by marking dra7xx_pcie_msi IRQ as IRQF_NO_THREAD.

I can remove below text.

>> +	 * if some platform will utilize a lot of MSI IRQs and will suffer
>> +	 * form -RT latencies degradation because of that - IRQF_NO_THREAD can
>> +	 * be removed and "Warning Annihilation" W/A can be applied [1]
>> +	 *
>> +	 * [1] https://lkml.org/lkml/2015/11/2/578
> 
> I don't think this novel is required. Also a reference to a patch which
> was not accepted is not a smart idea (since people might think they
> will improve their -RT latency by applying it without thinking).

Agree. Honestly, I don't like both these "solution"/W/A... :(
but have nothing better in mind.

> 
> Do you have any *real* numbers where you can say this patch does
> better/worse than what you suggester earlier? I'm simply asking because
> each gpio-irq multiplexing driver does the same thing.
> 

Indeed. And not only gpio :( Below you can find list of files which
contain "generic_handle_irq" and do not contain "irq_set_chained_handler"
(of course this list is inaccurate). [Addendum 1]

Regarding *real* numbers, I've tested it only on TI dra7-evm + pcie USB3 expander card
which generates legacy IRQ every 1 ms and didn't observe latency changes.
I'm not sure that I'd be able to test any scenario soon (next week), which will be
close to the potential "worst" case. Plus, I found acceptable assumption, that only
few pci irqs might be pending at the same time (at least for the reference Kernel code).
That's why I gave up without fight :(

Seems, I need explain why I'm trying to avoid using IRQF_NO_THREAD by all means
(I'm sorry, if below may sound dummy, and may be I'm overestimating/demonizing problem -
 my -RT experience is not really good [yet:)]).

- IRQF_NO_THREAD is the first considered option for such kind of issues.
  But: Now in LKML there are ~60 occurrences of IRQF_NO_THREAD - most of
  them are used by Arch code. And It's only used by 6 drivers (drivers/*) [Addendum 2].
  During past year, I've found only two threads related to IRQF_NO_THREAD
  and, in both cases, IRQF_NO_THREAD was added for arch specific IRQs which
  can't be threaded (https://lists.ozlabs.org/pipermail/linuxppc-dev/2014-November/122659.html,
  https://lkml.org/lkml/2015/4/21/404).
  
- ARM UP system: TI's am437xx SoCs for example.
   Here everything is started from /drivers/irqchip/irq-gic.c -> gic_handle_irq()

  static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
  {
	u32 irqstat, irqnr;
	struct gic_chip_data *gic = &gic_data[0];
	void __iomem *cpu_base = gic_data_cpu_base(gic);

	do {
		irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
		irqnr = irqstat & GICC_IAR_INT_ID_MASK;

		if (likely(irqnr > 15 && irqnr < 1021)) {
			if (static_key_true(&supports_deactivate))
				writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
			handle_domain_irq(gic->domain, irqnr, regs);
			|- struct pt_regs *old_regs = set_irq_regs(regs);
*			|- irq_enter();

			|- generic_handle_irq(irq);
[1]			   //*** -RT: all IRQ are threaded (except SPI, timers,..) ***
			   |- handle_fasteoi_irq()
			      |- irq_default_primary_handler()
			      |- _irq_wake_thread(desc, action);

[2]			   //***  chained irqX ***
			   chained_irq_handler
			   |- chained_irq_enter(chip, desc);
			   |- handle IRQX.1
				|- generic_handle_irq(IRQX.1);
				   ...
			   |- handle IRQX.2
			   |- handle IRQX.n
			   |- chained_irq_exit(chip, desc);

[3]			   //*** IRQF_NO_THREAD irqY ***
			   |- handle_fasteoi_irq()
				|- driverY_hw_irq_handler()
				   |- handle IRQY.1
					|- generic_handle_irq(IRQY.1);
					...
			           |- handle IRQY.2
				   |- handle IRQY.n

*			|- irq_exit();
			|- set_irq_regs(old_regs);
			continue;
		}
		if (irqnr < 16) {
			writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
			if (static_key_true(&supports_deactivate))
				writel_relaxed(irqstat, cpu_base + GIC_CPU_DEACTIVATE);
#ifdef CONFIG_SMP
			handle_IPI(irqnr, regs);
#endif
			continue;
		}
		break;
	} while (1);
  }

GIC IRQ handler gic_handle_irq() may process more than one IRQ without leaving HW IRQ mode
(during my experiments I saw up to 6 IRQs processed in one cycle).

As result, It was concluded, that having such current HW/SW and all IRQs forced threaded [1],
it will be potentially possible to predict system behavior, because gic_handle_irq() will
do the same things for most of processed IRQs.
But, once there will be chained [2] or IRQF_NO_THREAD [3] IRQs - complete unpredictability.

So, It was selected as goal to have all PPI IRQs (forced) threaded. And if someone
will require faster IRQ handling - IRQF_NO_THREAD can be always added, but it will
be custom solution then. 

I'd be appreciated for your comments - if above problem is not a problem.
Good - IRQF_NO_THREAD forever!

Thanks.
-- 
regards,
-grygorii


=== Addendum 1
x0174654@KBP1-T01-F55654:~/kernel.org/linux-master/linux$ grep -L irq_set_chained_handler $(grep -r -I -l generic_handle_irq drivers/*)
drivers/bcma/driver_gpio.c
drivers/clk/at91/pmc.c
drivers/gpio/gpio-lynxpoint.c
drivers/gpio/gpio-brcmstb.c
drivers/gpio/gpio-grgpio.c
drivers/gpio/gpio-etraxfs.c
drivers/gpio/gpio-omap.c
drivers/gpio/gpio-ml-ioh.c
drivers/gpio/gpio-dln2.c
drivers/gpio/gpio-rcar.c
drivers/gpio/gpio-pch.c
drivers/gpio/gpio-zx.c
drivers/gpio/gpio-tb10x.c
drivers/gpio/gpio-em.c
drivers/gpio/gpio-altera.c
drivers/gpio/gpio-omap.mod.c
drivers/gpio/gpio-zynq.c
drivers/gpio/gpio-pl061.c
drivers/gpio/gpio-vf610.c
drivers/gpio/gpio-xlp.c
drivers/gpio/gpio-intel-mid.c
drivers/gpio/gpio-sodaville.c
drivers/gpio/gpio-sta2x11.c
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
drivers/iio/industrialio-trigger.c
drivers/irqchip/irq-renesas-irqc.c
drivers/irqchip/irq-ingenic.c
drivers/irqchip/irq-renesas-intc-irqpin.c
drivers/memory/omap-gpmc.c
drivers/mfd/db8500-prcmu.c
drivers/mfd/htc-i2cpld.c
drivers/parisc/superio.c
drivers/parisc/gsc.c
drivers/parisc/eisa.c
drivers/parisc/dino.c
drivers/pci/host/pcie-xilinx.c
drivers/pci/host/pci-keystone-dw.c
drivers/pci/host/pcie-rcar.c
drivers/pci/host/pci-dra7xx.c
drivers/pci/host/pcie-designware.c
drivers/pci/host/pci-tegra.c
drivers/pinctrl/qcom/pinctrl-msm.c
drivers/pinctrl/pinctrl-amd.c
drivers/pinctrl/samsung/pinctrl-exynos5440.c
drivers/pinctrl/nomadik/pinctrl-nomadik.c
drivers/pinctrl/sirf/pinctrl-atlas7.c
drivers/pinctrl/sirf/pinctrl-sirf.c
drivers/pinctrl/spear/pinctrl-plgpio.c
drivers/pinctrl/pinctrl-at91.c
drivers/pinctrl/bcm/pinctrl-bcm2835.c
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c
drivers/pinctrl/intel/pinctrl-cherryview.c
drivers/pinctrl/intel/pinctrl-baytrail.c
drivers/pinctrl/intel/pinctrl-intel.c
drivers/pinctrl/pinctrl-pistachio.c
drivers/pinctrl/pinctrl-coh901.c
drivers/platform/x86/intel_pmic_gpio.c
drivers/ssb/driver_gpio.c
drivers/xen/events/events_2l.c
drivers/xen/events/events_fifo.c

=== Addendum 2
dmar.c
irq-i8259.c
arm_pmu.c
pinctrl-single.c
mips_ejtag_fdc.c (2 matches)
octeon-wdt-main.c

Re: [PATCH] PCI: dra7xx: mark dra7xx_pcie_msi irq as IRQF_NO_THREAD

[Sebastian Andrzej Siewior]

On 11/06/2015 08:59 PM, Grygorii Strashko wrote:
> Hi Sebastian,

Hi Grygorii,

> - IRQF_NO_THREAD is the first considered option for such kind of issues.
>   But: Now in LKML there are ~60 occurrences of IRQF_NO_THREAD - most of
>   them are used by Arch code. And It's only used by 6 drivers (drivers/*) [Addendum 2].
>   During past year, I've found only two threads related to IRQF_NO_THREAD
>   and, in both cases, IRQF_NO_THREAD was added for arch specific IRQs which
>   can't be threaded (https://lists.ozlabs.org/pipermail/linuxppc-dev/2014-November/122659.html,
>   https://lkml.org/lkml/2015/4/21/404).

That powerpc patch you reference is doing the same thing you are doing
here.

> - ARM UP system: TI's am437xx SoCs for example.
>    Here everything is started from /drivers/irqchip/irq-gic.c -> gic_handle_irq()
> 

> GIC IRQ handler gic_handle_irq() may process more than one IRQ without leaving HW IRQ mode
> (during my experiments I saw up to 6 IRQs processed in one cycle).

not only GIC. But then what good does it do if it leaves and returns
immediately back to the routine?

> As result, It was concluded, that having such current HW/SW and all IRQs forced threaded [1],
> it will be potentially possible to predict system behavior, because gic_handle_irq() will
> do the same things for most of processed IRQs.
> But, once there will be chained [2] or IRQF_NO_THREAD [3] IRQs - complete unpredictability.

I would not go as far as "complete unpredictability". What you do (or
should do) is testing the system for longer period of time with
different behavior in order to estimate the worst case.
You can't predict the system anyway since it is way too complex. Just
try something that ensures that cyclictest is no longer cache hot and
see what happens then.

> So, It was selected as goal to have all PPI IRQs (forced) threaded. And if someone
> will require faster IRQ handling - IRQF_NO_THREAD can be always added, but it will
> be custom solution then. 
> 
> I'd be appreciated for your comments - if above problem is not a problem.
> Good - IRQF_NO_THREAD forever!

Yes, we try to avoid IRQF_NO_THREAD under all circumstances. However it
is required for low-level arch code. This includes basically
everything that is using raw-locks which includes interrupt controller
(the "real" ones like GIC or cascading like MSI or GPIO).
Here it is simple - you have a cascading MSI-interrupt controller and
as such it should be IRQF_NO_THREAD marked.
The latency spikes in worst case are not huge as explained earlier: The
only thing your cascading controller is allowed to do is to mark
interrupt as pending (which is with threaded interrupts just a task
wakeup).
And this is not a -RT only problem: it is broken in vanilla linux with
threaded interrupts as well.

Btw: There is an exception to this rule (as always): If you are on a
slow/blocking bus then you don't do IRQF_NO_THREAD. A slow bus would be
a GPIO controller behind an I2C/SPI controller which also acts as an
interrupt controller. Here you use a threaded-handler +
handle_nested_irq().

> Thanks.

Sebastian

[PATCH] PCI: dra7xx: mark dra7xx_pcie_msi irq as IRQF_NO_THREAD

[Sebastian Andrzej Siewior]

On 11/06/2015 08:59 PM, Grygorii Strashko wrote:
> Hi Sebastian,

Hi Grygorii,

> - IRQF_NO_THREAD is the first considered option for such kind of issues.
>   But: Now in LKML there are ~60 occurrences of IRQF_NO_THREAD - most of
>   them are used by Arch code. And It's only used by 6 drivers (drivers/*) [Addendum 2].
>   During past year, I've found only two threads related to IRQF_NO_THREAD
>   and, in both cases, IRQF_NO_THREAD was added for arch specific IRQs which
>   can't be threaded (https://lists.ozlabs.org/pipermail/linuxppc-dev/2014-November/122659.html,
>   https://lkml.org/lkml/2015/4/21/404).

That powerpc patch you reference is doing the same thing you are doing
here.

> - ARM UP system: TI's am437xx SoCs for example.
>    Here everything is started from /drivers/irqchip/irq-gic.c -> gic_handle_irq()
> 

> GIC IRQ handler gic_handle_irq() may process more than one IRQ without leaving HW IRQ mode
> (during my experiments I saw up to 6 IRQs processed in one cycle).

not only GIC. But then what good does it do if it leaves and returns
immediately back to the routine?

> As result, It was concluded, that having such current HW/SW and all IRQs forced threaded [1],
> it will be potentially possible to predict system behavior, because gic_handle_irq() will
> do the same things for most of processed IRQs.
> But, once there will be chained [2] or IRQF_NO_THREAD [3] IRQs - complete unpredictability.

I would not go as far as "complete unpredictability". What you do (or
should do) is testing the system for longer period of time with
different behavior in order to estimate the worst case.
You can't predict the system anyway since it is way too complex. Just
try something that ensures that cyclictest is no longer cache hot and
see what happens then.

> So, It was selected as goal to have all PPI IRQs (forced) threaded. And if someone
> will require faster IRQ handling - IRQF_NO_THREAD can be always added, but it will
> be custom solution then. 
> 
> I'd be appreciated for your comments - if above problem is not a problem.
> Good - IRQF_NO_THREAD forever!

Yes, we try to avoid IRQF_NO_THREAD under all circumstances. However it
is required for low-level arch code. This includes basically
everything that is using raw-locks which includes interrupt controller
(the "real" ones like GIC or cascading like MSI or GPIO).
Here it is simple - you have a cascading MSI-interrupt controller and
as such it should be IRQF_NO_THREAD marked.
The latency spikes in worst case are not huge as explained earlier: The
only thing your cascading controller is allowed to do is to mark
interrupt as pending (which is with threaded interrupts just a task
wakeup).
And this is not a -RT only problem: it is broken in vanilla linux with
threaded interrupts as well.

Btw: There is an exception to this rule (as always): If you are on a
slow/blocking bus then you don't do IRQF_NO_THREAD. A slow bus would be
a GPIO controller behind an I2C/SPI controller which also acts as an
interrupt controller. Here you use a threaded-handler +
handle_nested_irq().

> Thanks.

Sebastian

Re: [PATCH] PCI: dra7xx: mark dra7xx_pcie_msi irq as IRQF_NO_THREAD

[Grygorii Strashko]

On 11/12/2015 11:19 AM, Sebastian Andrzej Siewior wrote:
> On 11/06/2015 08:59 PM, Grygorii Strashko wrote:
>> Hi Sebastian,
> 
> Hi Grygorii,
> 
>> - IRQF_NO_THREAD is the first considered option for such kind of issues.
>>    But: Now in LKML there are ~60 occurrences of IRQF_NO_THREAD - most of
>>    them are used by Arch code. And It's only used by 6 drivers (drivers/*) [Addendum 2].
>>    During past year, I've found only two threads related to IRQF_NO_THREAD
>>    and, in both cases, IRQF_NO_THREAD was added for arch specific IRQs which
>>    can't be threaded (https://lists.ozlabs.org/pipermail/linuxppc-dev/2014-November/122659.html,
>>    https://lkml.org/lkml/2015/4/21/404).
> 
> That powerpc patch you reference is doing the same thing you are doing
> here.

Probably. I don't know this hw, so my assumption was based on commits descriptions.

> 
>> - ARM UP system: TI's am437xx SoCs for example.
>>     Here everything is started from /drivers/irqchip/irq-gic.c -> gic_handle_irq()
>>
> 
>> GIC IRQ handler gic_handle_irq() may process more than one IRQ without leaving HW IRQ mode
>> (during my experiments I saw up to 6 IRQs processed in one cycle).
> 
> not only GIC. But then what good does it do if it leaves and returns
> immediately back to the routine?
> 
>> As result, It was concluded, that having such current HW/SW and all IRQs forced threaded [1],
>> it will be potentially possible to predict system behavior, because gic_handle_irq() will
>> do the same things for most of processed IRQs.
>> But, once there will be chained [2] or IRQF_NO_THREAD [3] IRQs - complete unpredictability.
> 
> I would not go as far as "complete unpredictability". What you do (or
> should do) is testing the system for longer period of time with
> different behavior in order to estimate the worst case.
> You can't predict the system anyway since it is way too complex. Just
> try something that ensures that cyclictest is no longer cache hot and
> see what happens then.

I understand that. That's the current plan and work is in progress.
The nearest target is to get rid of all -RT specific backtracks and
ensure TI -RT kernel supports the same functionality as non-RT.
next step - try to optimize.

> 
>> So, It was selected as goal to have all PPI IRQs (forced) threaded. And if someone
>> will require faster IRQ handling - IRQF_NO_THREAD can be always added, but it will
>> be custom solution then.
>>
>> I'd be appreciated for your comments - if above problem is not a problem.
>> Good - IRQF_NO_THREAD forever!
> 
> Yes, we try to avoid IRQF_NO_THREAD under all circumstances. However it
> is required for low-level arch code. This includes basically
> everything that is using raw-locks which includes interrupt controller
> (the "real" ones like GIC or cascading like MSI or GPIO).
> Here it is simple - you have a cascading MSI-interrupt controller and
> as such it should be IRQF_NO_THREAD marked.
> The latency spikes in worst case are not huge as explained earlier: The
> only thing your cascading controller is allowed to do is to mark
> interrupt as pending (which is with threaded interrupts just a task
> wakeup).
> And this is not a -RT only problem: it is broken in vanilla linux with
> threaded interrupts as well.
> 

Ok. I've got it.  IRQF_NO_THREAD will be a solution for reference code and
for issues like this. I understand, that each, -RT based, real solution is
unique and need to be specifically tuned, so if someone will have problem
with IRQF_NO_THREAD - it can be removed easily and replaced with any sort of
custom hacks/improvements.

Thanks a lot for your comments.
I'll apply your previous comments and re-send.

-- 
regards,
-grygorii

Re: [PATCH] PCI: dra7xx: mark dra7xx_pcie_msi irq as IRQF_NO_THREAD

[Grygorii Strashko]

On 11/12/2015 11:19 AM, Sebastian Andrzej Siewior wrote:
> On 11/06/2015 08:59 PM, Grygorii Strashko wrote:
>> Hi Sebastian,
> 
> Hi Grygorii,
> 
>> - IRQF_NO_THREAD is the first considered option for such kind of issues.
>>    But: Now in LKML there are ~60 occurrences of IRQF_NO_THREAD - most of
>>    them are used by Arch code. And It's only used by 6 drivers (drivers/*) [Addendum 2].
>>    During past year, I've found only two threads related to IRQF_NO_THREAD
>>    and, in both cases, IRQF_NO_THREAD was added for arch specific IRQs which
>>    can't be threaded (https://lists.ozlabs.org/pipermail/linuxppc-dev/2014-November/122659.html,
>>    https://lkml.org/lkml/2015/4/21/404).
> 
> That powerpc patch you reference is doing the same thing you are doing
> here.

Probably. I don't know this hw, so my assumption was based on commits descriptions.

> 
>> - ARM UP system: TI's am437xx SoCs for example.
>>     Here everything is started from /drivers/irqchip/irq-gic.c -> gic_handle_irq()
>>
> 
>> GIC IRQ handler gic_handle_irq() may process more than one IRQ without leaving HW IRQ mode
>> (during my experiments I saw up to 6 IRQs processed in one cycle).
> 
> not only GIC. But then what good does it do if it leaves and returns
> immediately back to the routine?
> 
>> As result, It was concluded, that having such current HW/SW and all IRQs forced threaded [1],
>> it will be potentially possible to predict system behavior, because gic_handle_irq() will
>> do the same things for most of processed IRQs.
>> But, once there will be chained [2] or IRQF_NO_THREAD [3] IRQs - complete unpredictability.
> 
> I would not go as far as "complete unpredictability". What you do (or
> should do) is testing the system for longer period of time with
> different behavior in order to estimate the worst case.
> You can't predict the system anyway since it is way too complex. Just
> try something that ensures that cyclictest is no longer cache hot and
> see what happens then.

I understand that. That's the current plan and work is in progress.
The nearest target is to get rid of all -RT specific backtracks and
ensure TI -RT kernel supports the same functionality as non-RT.
next step - try to optimize.

> 
>> So, It was selected as goal to have all PPI IRQs (forced) threaded. And if someone
>> will require faster IRQ handling - IRQF_NO_THREAD can be always added, but it will
>> be custom solution then.
>>
>> I'd be appreciated for your comments - if above problem is not a problem.
>> Good - IRQF_NO_THREAD forever!
> 
> Yes, we try to avoid IRQF_NO_THREAD under all circumstances. However it
> is required for low-level arch code. This includes basically
> everything that is using raw-locks which includes interrupt controller
> (the "real" ones like GIC or cascading like MSI or GPIO).
> Here it is simple - you have a cascading MSI-interrupt controller and
> as such it should be IRQF_NO_THREAD marked.
> The latency spikes in worst case are not huge as explained earlier: The
> only thing your cascading controller is allowed to do is to mark
> interrupt as pending (which is with threaded interrupts just a task
> wakeup).
> And this is not a -RT only problem: it is broken in vanilla linux with
> threaded interrupts as well.
> 

Ok. I've got it.  IRQF_NO_THREAD will be a solution for reference code and
for issues like this. I understand, that each, -RT based, real solution is
unique and need to be specifically tuned, so if someone will have problem
with IRQF_NO_THREAD - it can be removed easily and replaced with any sort of
custom hacks/improvements.

Thanks a lot for your comments.
I'll apply your previous comments and re-send.

-- 
regards,
-grygorii

[PATCH] PCI: dra7xx: mark dra7xx_pcie_msi irq as IRQF_NO_THREAD

[Grygorii Strashko]

On 11/12/2015 11:19 AM, Sebastian Andrzej Siewior wrote:
> On 11/06/2015 08:59 PM, Grygorii Strashko wrote:
>> Hi Sebastian,
> 
> Hi Grygorii,
> 
>> - IRQF_NO_THREAD is the first considered option for such kind of issues.
>>    But: Now in LKML there are ~60 occurrences of IRQF_NO_THREAD - most of
>>    them are used by Arch code. And It's only used by 6 drivers (drivers/*) [Addendum 2].
>>    During past year, I've found only two threads related to IRQF_NO_THREAD
>>    and, in both cases, IRQF_NO_THREAD was added for arch specific IRQs which
>>    can't be threaded (https://lists.ozlabs.org/pipermail/linuxppc-dev/2014-November/122659.html,
>>    https://lkml.org/lkml/2015/4/21/404).
> 
> That powerpc patch you reference is doing the same thing you are doing
> here.

Probably. I don't know this hw, so my assumption was based on commits descriptions.

> 
>> - ARM UP system: TI's am437xx SoCs for example.
>>     Here everything is started from /drivers/irqchip/irq-gic.c -> gic_handle_irq()
>>
> 
>> GIC IRQ handler gic_handle_irq() may process more than one IRQ without leaving HW IRQ mode
>> (during my experiments I saw up to 6 IRQs processed in one cycle).
> 
> not only GIC. But then what good does it do if it leaves and returns
> immediately back to the routine?
> 
>> As result, It was concluded, that having such current HW/SW and all IRQs forced threaded [1],
>> it will be potentially possible to predict system behavior, because gic_handle_irq() will
>> do the same things for most of processed IRQs.
>> But, once there will be chained [2] or IRQF_NO_THREAD [3] IRQs - complete unpredictability.
> 
> I would not go as far as "complete unpredictability". What you do (or
> should do) is testing the system for longer period of time with
> different behavior in order to estimate the worst case.
> You can't predict the system anyway since it is way too complex. Just
> try something that ensures that cyclictest is no longer cache hot and
> see what happens then.

I understand that. That's the current plan and work is in progress.
The nearest target is to get rid of all -RT specific backtracks and
ensure TI -RT kernel supports the same functionality as non-RT.
next step - try to optimize.

> 
>> So, It was selected as goal to have all PPI IRQs (forced) threaded. And if someone
>> will require faster IRQ handling - IRQF_NO_THREAD can be always added, but it will
>> be custom solution then.
>>
>> I'd be appreciated for your comments - if above problem is not a problem.
>> Good - IRQF_NO_THREAD forever!
> 
> Yes, we try to avoid IRQF_NO_THREAD under all circumstances. However it
> is required for low-level arch code. This includes basically
> everything that is using raw-locks which includes interrupt controller
> (the "real" ones like GIC or cascading like MSI or GPIO).
> Here it is simple - you have a cascading MSI-interrupt controller and
> as such it should be IRQF_NO_THREAD marked.
> The latency spikes in worst case are not huge as explained earlier: The
> only thing your cascading controller is allowed to do is to mark
> interrupt as pending (which is with threaded interrupts just a task
> wakeup).
> And this is not a -RT only problem: it is broken in vanilla linux with
> threaded interrupts as well.
> 

Ok. I've got it.  IRQF_NO_THREAD will be a solution for reference code and
for issues like this. I understand, that each, -RT based, real solution is
unique and need to be specifically tuned, so if someone will have problem
with IRQF_NO_THREAD - it can be removed easily and replaced with any sort of
custom hacks/improvements.

Thanks a lot for your comments.
I'll apply your previous comments and re-send.

-- 
regards,
-grygorii