[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]

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

[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

[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