public inbox for linux-arm-kernel@lists.infradead.org 
 help / color / mirror / Atom feed

• * [PATCH] PCI: dra7xx: mark dra7xx_pcie_msi irq as IRQF_NO_THREAD
         [not found] ` <563C6A7D.7040503@linutronix.de>
  @ 2015-11-06 19:59   ` Grygorii Strashko
    2015-11-12  9:19     ` Sebastian Andrzej Siewior
    0 siblings, 1 reply; 3+ messages in thread
  From: Grygorii Strashko @ 2015-11-06 19:59 UTC (permalink / raw)
    To: linux-arm-kernel
  
  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
  
  ^ permalink raw reply	[flat|nested] 3+ messages in thread

• • * [PATCH] PCI: dra7xx: mark dra7xx_pcie_msi irq as IRQF_NO_THREAD
      2015-11-06 19:59   ` [PATCH] PCI: dra7xx: mark dra7xx_pcie_msi irq as IRQF_NO_THREAD Grygorii Strashko
    @ 2015-11-12  9:19     ` Sebastian Andrzej Siewior
      2015-11-13 18:48       ` Grygorii Strashko
      0 siblings, 1 reply; 3+ messages in thread
    From: Sebastian Andrzej Siewior @ 2015-11-12  9:19 UTC (permalink / raw)
      To: linux-arm-kernel
    
    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
    
    ^ permalink raw reply	[flat|nested] 3+ messages in thread

  • • * [PATCH] PCI: dra7xx: mark dra7xx_pcie_msi irq as IRQF_NO_THREAD
        2015-11-12  9:19     ` Sebastian Andrzej Siewior
      @ 2015-11-13 18:48       ` Grygorii Strashko
        0 siblings, 0 replies; 3+ messages in thread
      From: Grygorii Strashko @ 2015-11-13 18:48 UTC (permalink / raw)
        To: linux-arm-kernel
      
      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
      
      ^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2015-11-13 18:48 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <1446748990-27722-1-git-send-email-grygorii.strashko@ti.com>
     [not found] ` <563C6A7D.7040503@linutronix.de>
2015-11-06 19:59   ` [PATCH] PCI: dra7xx: mark dra7xx_pcie_msi irq as IRQF_NO_THREAD Grygorii Strashko
2015-11-12  9:19     ` Sebastian Andrzej Siewior
2015-11-13 18:48       ` Grygorii Strashko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox