| Message ID | 1389073086-6763-1-git-send-email-dongsheng.wang@freescale.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, 2014-01-07 at 13:38 +0800, Dongsheng Wang wrote: > From: Wang Dongsheng <dongsheng.wang@freescale.com> > > Currently MPIC provides .mask, but not .disable. This means that > effectively disable_irq() soft-disables the interrupt, and you get > a .mask call if an interrupt actually occurs. > > I'm not sure if this was intended as a performance benefit (it seems common > to omit .disable on powerpc interrupt controllers, but nowhere else), but it > interacts badly with threaded/workqueue interrupts (including KVM > reflection). In such cases, where the real interrupt handler does a > disable_irq_nosync(), schedules defered handling, and returns, we get two > interrupts for every real interrupt. The second interrupt does nothing > but see that IRQ_DISABLED is set, and decide that it would be a good > idea to actually call .mask. We probably don't want to do that for edge, only level interrupts. Cheers, Ben. > > Signed-off-by: Scott Wood <scottwood@freescale.com> > Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com> > > diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c > index 0e166ed..dd7564b 100644 > --- a/arch/powerpc/sysdev/mpic.c > +++ b/arch/powerpc/sysdev/mpic.c > @@ -975,6 +975,7 @@ void mpic_set_destination(unsigned int virq, unsigned int cpuid) > } > > static struct irq_chip mpic_irq_chip = { > + .irq_disable = mpic_mask_irq, > .irq_mask = mpic_mask_irq, > .irq_unmask = mpic_unmask_irq, > .irq_eoi = mpic_end_irq, > @@ -984,6 +985,7 @@ static struct irq_chip mpic_irq_chip = { > > #ifdef CONFIG_SMP > static struct irq_chip mpic_ipi_chip = { > + .irq_disable = mpic_mask_ipi, > .irq_mask = mpic_mask_ipi, > .irq_unmask = mpic_unmask_ipi, > .irq_eoi = mpic_end_ipi, > @@ -991,6 +993,7 @@ static struct irq_chip mpic_ipi_chip = { > #endif /* CONFIG_SMP */ > > static struct irq_chip mpic_tm_chip = { > + .irq_disable = mpic_mask_tm, > .irq_mask = mpic_mask_tm, > .irq_unmask = mpic_unmask_tm, > .irq_eoi = mpic_end_irq, > @@ -1001,6 +1004,7 @@ static struct irq_chip mpic_tm_chip = { > static struct irq_chip mpic_irq_ht_chip = { > .irq_startup = mpic_startup_ht_irq, > .irq_shutdown = mpic_shutdown_ht_irq, > + .irq_disable = mpic_mask_irq, > .irq_mask = mpic_mask_irq, > .irq_unmask = mpic_unmask_ht_irq, > .irq_eoi = mpic_end_ht_irq,
On Tue, 2014-01-07 at 13:38 +0800, Dongsheng Wang wrote:
> From: Wang Dongsheng <dongsheng.wang@freescale.com>
Why did you change the author field?
-Scott
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Tuesday, January 07, 2014 2:39 PM > To: Wang Dongsheng-B40534 > Cc: benh@kernel.crashing.org; linuxppc-dev@lists.ozlabs.org > Subject: Re: [PATCH] powerpc/mpic: supply a .disable callback > > On Tue, 2014-01-07 at 13:38 +0800, Dongsheng Wang wrote: > > From: Wang Dongsheng <dongsheng.wang@freescale.com> > > Why did you change the author field? > :(. I forgot that, I added this code, not use git. Sorry about that, I'll change back after our discussion -Dongsheng > -Scott >
> -----Original Message----- > From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org] > Sent: Tuesday, January 07, 2014 1:50 PM > To: Wang Dongsheng-B40534 > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org > Subject: Re: [PATCH] powerpc/mpic: supply a .disable callback > > On Tue, 2014-01-07 at 13:38 +0800, Dongsheng Wang wrote: > > From: Wang Dongsheng <dongsheng.wang@freescale.com> > > > > Currently MPIC provides .mask, but not .disable. This means that > > effectively disable_irq() soft-disables the interrupt, and you get > > a .mask call if an interrupt actually occurs. > > > > I'm not sure if this was intended as a performance benefit (it seems common > > to omit .disable on powerpc interrupt controllers, but nowhere else), but it > > interacts badly with threaded/workqueue interrupts (including KVM > > reflection). In such cases, where the real interrupt handler does a > > disable_irq_nosync(), schedules defered handling, and returns, we get two > > interrupts for every real interrupt. The second interrupt does nothing > > but see that IRQ_DISABLED is set, and decide that it would be a good > > idea to actually call .mask. > > We probably don't want to do that for edge, only level interrupts. > Sorry Ben, I am not understand your comments. This issue is the kernel api irq_disable() only use chip->irq_disable(), but mpic not have this interface so we don't real disable the interrupt. -Dongsheng > Cheers, > Ben. >
On Tue, 2014-01-07 at 04:18 -0600, Wang Dongsheng-B40534 wrote: > > > -----Original Message----- > > From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org] > > Sent: Tuesday, January 07, 2014 1:50 PM > > To: Wang Dongsheng-B40534 > > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org > > Subject: Re: [PATCH] powerpc/mpic: supply a .disable callback > > > > On Tue, 2014-01-07 at 13:38 +0800, Dongsheng Wang wrote: > > > From: Wang Dongsheng <dongsheng.wang@freescale.com> > > > > > > Currently MPIC provides .mask, but not .disable. This means that > > > effectively disable_irq() soft-disables the interrupt, and you get > > > a .mask call if an interrupt actually occurs. > > > > > > I'm not sure if this was intended as a performance benefit (it seems common > > > to omit .disable on powerpc interrupt controllers, but nowhere else), but it > > > interacts badly with threaded/workqueue interrupts (including KVM > > > reflection). In such cases, where the real interrupt handler does a > > > disable_irq_nosync(), schedules defered handling, and returns, we get two > > > interrupts for every real interrupt. The second interrupt does nothing > > > but see that IRQ_DISABLED is set, and decide that it would be a good > > > idea to actually call .mask. > > > > We probably don't want to do that for edge, only level interrupts. > > > Sorry Ben, I am not understand your comments. > > This issue is the kernel api irq_disable() only use chip->irq_disable(), but mpic > not have this interface so we don't real disable the interrupt. I think he means that the "two interrupts for every real interrupt" effect will only happen with level triggered interrupts, and he'd like to keep the potential performance benefit of lazy disabling for edge interrupts. To implement this for "ordinary" edge interrupts (not IPI, timer, etc) we'd need to add a new .irq_disable() function that checks whether it's level/edge and only calls .irq_mask() if level -- or, introduce a separate struct irq_chip for edge versus level. -Scott
On Tue, 2014-01-07 at 10:18 +0000, Dongsheng.Wang@freescale.com wrote: > > > -----Original Message----- > > From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org] > > Sent: Tuesday, January 07, 2014 1:50 PM > > To: Wang Dongsheng-B40534 > > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org > > Subject: Re: [PATCH] powerpc/mpic: supply a .disable callback > > > > On Tue, 2014-01-07 at 13:38 +0800, Dongsheng Wang wrote: > > > From: Wang Dongsheng <dongsheng.wang@freescale.com> > > > > > > Currently MPIC provides .mask, but not .disable. This means that > > > effectively disable_irq() soft-disables the interrupt, and you get > > > a .mask call if an interrupt actually occurs. > > > > > > I'm not sure if this was intended as a performance benefit (it seems common > > > to omit .disable on powerpc interrupt controllers, but nowhere else), but it > > > interacts badly with threaded/workqueue interrupts (including KVM > > > reflection). In such cases, where the real interrupt handler does a > > > disable_irq_nosync(), schedules defered handling, and returns, we get two > > > interrupts for every real interrupt. The second interrupt does nothing > > > but see that IRQ_DISABLED is set, and decide that it would be a good > > > idea to actually call .mask. > > > > We probably don't want to do that for edge, only level interrupts. > > > Sorry Ben, I am not understand your comments. > > This issue is the kernel api irq_disable() only use chip->irq_disable(), but mpic > not have this interface so we don't real disable the interrupt. Yes, because we want to keep the existing behaviour of "lazy disable" for edge interrupts. It's faster. Cheers, Ben. > -Dongsheng > > > Cheers, > > Ben. > > >
diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c index 0e166ed..dd7564b 100644 --- a/arch/powerpc/sysdev/mpic.c +++ b/arch/powerpc/sysdev/mpic.c @@ -975,6 +975,7 @@ void mpic_set_destination(unsigned int virq, unsigned int cpuid) } static struct irq_chip mpic_irq_chip = { + .irq_disable = mpic_mask_irq, .irq_mask = mpic_mask_irq, .irq_unmask = mpic_unmask_irq, .irq_eoi = mpic_end_irq, @@ -984,6 +985,7 @@ static struct irq_chip mpic_irq_chip = { #ifdef CONFIG_SMP static struct irq_chip mpic_ipi_chip = { + .irq_disable = mpic_mask_ipi, .irq_mask = mpic_mask_ipi, .irq_unmask = mpic_unmask_ipi, .irq_eoi = mpic_end_ipi, @@ -991,6 +993,7 @@ static struct irq_chip mpic_ipi_chip = { #endif /* CONFIG_SMP */ static struct irq_chip mpic_tm_chip = { + .irq_disable = mpic_mask_tm, .irq_mask = mpic_mask_tm, .irq_unmask = mpic_unmask_tm, .irq_eoi = mpic_end_irq, @@ -1001,6 +1004,7 @@ static struct irq_chip mpic_tm_chip = { static struct irq_chip mpic_irq_ht_chip = { .irq_startup = mpic_startup_ht_irq, .irq_shutdown = mpic_shutdown_ht_irq, + .irq_disable = mpic_mask_irq, .irq_mask = mpic_mask_irq, .irq_unmask = mpic_unmask_ht_irq, .irq_eoi = mpic_end_ht_irq,