: Reduce spurious interrupts.
Subsystems (2)
PATCH 10/11 : Reduce spurious interrupts.
[TG3]: Reduce spurious interrupts.
Spurious interrupts are often encountered especially on systems using the 8259 PIC mode. This is because the I/O write to deassert the interrupt is posted and won't get to the chip immediately. As a result, the IRQ may remain asserted after the IRQ handler exits, causing spurious interrupts.
An unconditional read to flush the I/O write to force the IRQ to de- assert immediately is not desirable because it impacts performance in the fast path. So we only do this after we have some indications of spurious interrupts.
drivers/net/tg3.c +34 -7
@@ -3608,12 +3608,31 @@ out:return IRQ_RETVAL (handled); } +static void tg3_check_spurious_int (struct tg3 *tp) +{ +if (tp->spurious_int_mode) +return ; + +/* Interrupt mailbox and status block have been flushed at this +* point. If the tag is unchanged, we are either getting a spurious +* interrupt or sharing interrupt with another device. +*/ +if (tp->hw_status->status_tag == tp->last_tag) { + tp->spurious_int++; +if (tp->spurious_int >500 ) +/* Assume we're getting too many spurious interrupts. +* Setting the flag will cause the IRQ handler to +* do a read back to immediately deassert the IRQ. +*/ + tp->spurious_int_mode =1 ; + } +} +static irqreturn_t tg3_interrupt_tagged (int irq,void *dev_id) {struct net_device *dev = dev_id;struct tg3 *tp =netdev_priv (dev);struct tg3_hw_status *sblk = tp->hw_status; -unsigned int handled =1 ;/* In INTx mode, it is possible for the interrupt to arrive at * the CPU before the status block posted prior to the interrupt.
@@ -3621,11 +3640,15 @@ static irqreturn_t tg3_interrupt_tagged(int irq, void *dev_id)* interrupt is ours and will flush the status block. */ if (unlikely (sblk->status_tag == tp->last_tag)) { -if ((tp->tg3_flags &TG3_FLAG_CHIP_RESETTING ) || - (tr32 (TG3PCI_PCISTATE ) &PCISTATE_INT_NOT_ACTIVE )) { - handled =0 ; - goto out; - } +u32 inta; + +if (tp->tg3_flags &TG3_FLAG_CHIP_RESETTING ) +return IRQ_NONE ; + + inta =tr32 (TG3PCI_PCISTATE ); +tg3_check_spurious_int (tp); +if (inta &PCISTATE_INT_NOT_ACTIVE ) +return IRQ_NONE ; }/*
@@ -3636,6 +3659,8 @@ static irqreturn_t tg3_interrupt_tagged(int irq, void *dev_id)* event coalescing. */ tw32_mailbox (MAILBOX_INTERRUPT_0 +TG3_64BIT_REG_LOW ,0x00000001 ); +if (unlikely (tp->spurious_int_mode)) +tr32_mailbox (MAILBOX_INTERRUPT_0 +TG3_64BIT_REG_LOW );if (tg3_irq_sync (tp)) goto out;if (netif_rx_schedule_prep (dev)) {
@@ -3649,7 +3674,7 @@ static irqreturn_t tg3_interrupt_tagged(int irq, void *dev_id)__netif_rx_schedule (dev); } out: -return IRQ_RETVAL (handled); +return IRQ_HANDLED ; }/* ISR for interrupt test */
@@ -6886,6 +6911,8 @@ static int tg3_request_irq(struct tg3 *tp)unsigned long flags;struct net_device *dev = tp->dev; + tp->spurious_int =0 ; + tp->spurious_int_mode =0 ;if (tp->tg3_flags2 &TG3_FLG2_USING_MSI ) { fn = tg3_msi;if (tp->tg3_flags2 &TG3_FLG2_1SHOT_MSI )
drivers/net/tg3.h +3 -0
@@ -2151,6 +2151,9 @@ struct tg3 {dma_addr_t status_mapping;u32 last_tag; +u16 spurious_int; +u16 spurious_int_mode; +u32 msg_enable;/* begin "tx thread" cacheline section */
Quoted 8 lines · Michael Chan
[TG3]: Reduce spurious interrupts.
Spurious interrupts are often encountered especially on systems using the 8259 PIC mode. This is because the I/O write to deassert the interrupt is posted and won't get to the chip immediately. As a result, the IRQ may remain asserted after the IRQ handler exits, causing spurious interrupts.
Quoted 6 lines
An unconditional read to flush the I/O write to force the IRQ to de- assert immediately is not desirable because it impacts performance in the fast path. So we only do this after we have some indications of spurious interrupts.
Signed-off-by Michael Chan <[email protected]>
hmmmm, this is a bit questionable that it needs to be here.
I think it's just a fact of life that it is important to flush certain writes...
I'm not sure a driver needs to be adding code to avoid the obvious solution. It would be annoying if all drivers had code to do this.
Jeff
Quoted 16 lines · Jeff Garzik
Quoted 8 lines · Michael Chan
[TG3]: Reduce spurious interrupts.
Spurious interrupts are often encountered especially on systems using the 8259 PIC mode. This is because the I/O write to deassert the interrupt is posted and won't get to the chip immediately. As a result, the IRQ may remain asserted after the IRQ handler exits, causing spurious interrupts.
Quoted 6 lines
An unconditional read to flush the I/O write to force the IRQ to de- assert immediately is not desirable because it impacts performance in the fast path. So we only do this after we have some indications of spurious interrupts.
Signed-off-by Michael Chan <[email protected]>
Quoted 7 lines
hmmmm, this is a bit questionable that it needs to be here.
I think it's just a fact of life that it is important to flush certain writes...
I'm not sure a driver needs to be adding code to avoid the obvious solution. It would be annoying if all drivers had code to do this.
We had a discussion about 2 years ago and David decided to remove the I/O read to improve performance. Since then a small number of users have been complaining about spurious interrupts. We can add back the unconditional read or do this detection thing which I agree is somewhat annoying. David, what do you think?
David Miller
maintainerFrom: "Michael Chan" <[email protected]>
Date: Fri, 04 May 2007 18:45:58 -0700
Quoted 5 lines
We had a discussion about 2 years ago and David decided to remove the I/O read to improve performance. Since then a small number of users have been complaining about spurious interrupts. We can add back the unconditional read or do this detection thing which I agree is somewhat annoying. David, what do you think?
If we get an extra interrupt, we should just silently ignore it.
The performance impact of adding that thing back is just too great.
Quoted 10 lines · David Miller
From: "Michael Chan" <[email protected]>
Date: Fri, 04 May 2007 18:45:58 -0700Quoted 6 lines
We had a discussion about 2 years ago and David decided to remove the I/O read to improve performance. Since then a small number of users have been complaining about spurious interrupts. We can add back the unconditional read or do this detection thing which I agree is somewhat annoying. David, what do you think?
Quoted 4 lines
If we get an extra interrupt, we should just silently ignore it.
The performance impact of adding that thing back is just too great.
In a small number of cases, we really get a ton of spurious interrupts. We have even seen the kernel shutting off IRQ7 because of too many unclaimed spurious interrupts.
David Miller
maintainerFrom: "Michael Chan" <[email protected]>
Date: Fri, 4 May 2007 19:50:10 -0700
Quoted 3 lines
In a small number of cases, we really get a ton of spurious interrupts. We have even seen the kernel shutting off IRQ7 because of too many unclaimed spurious interrupts.
Ok, that's a big problem.
In retrospect the read back is only needed on non-MSI right? I guess that'd be ok.
Quoted 12 lines · David Miller
From: "Michael Chan" <[email protected]>
Date: Fri, 4 May 2007 19:50:10 -0700Quoted 3 lines
In a small number of cases, we really get a ton of spurious interrupts. We have even seen the kernel shutting off IRQ7 because of too many unclaimed spurious interrupts.
Ok, that's a big problem.
In retrospect the read back is only needed on non-MSI right? I guess that'd be ok.
Right, not needed in MSI as MSI is considered an edge interrupt. I will resubmit a new patch to just flush the mailbox.
PATCH 10/11 : Eliminate spurious interrupts.
In reply to David Miller[TG3]: Eliminate spurious interrupts.
Spurious interrupts are often encountered especially on systems using the 8259 PIC mode. This is because the I/O write to deassert the interrupt is posted and won't get to the chip immediately. As a result, the IRQ may remain asserted after the IRQ handler exits, causing spurious interrupts.
Flush the interrupt mailbox in non-MSI handlers to de-assert the IRQ immediately. This seems to be the most straight forward approach after discussion with Jeff Garzik and David Miller.
drivers/net/tg3.c +10 -2
@@ -3590,8 +3590,12 @@ static irqreturn_t tg3_interrupt(int irq, void *dev_id)* Writing non-zero to intr-mbox-0 additional tells the * NIC to stop sending us irqs, engaging "in-intr-handler" * event coalescing. +* +* Flush the mailbox to de-assert the IRQ immediately to prevent +* spurious interrupts. The flush impacts performance but +* excessive spurious interrupts can be worse in some cases. */ -tw32_mailbox (MAILBOX_INTERRUPT_0 +TG3_64BIT_REG_LOW ,0x00000001 ); +tw32_mailbox_f (MAILBOX_INTERRUPT_0 +TG3_64BIT_REG_LOW ,0x00000001 );if (tg3_irq_sync (tp)) goto out; sblk->status &= ~SD_STATUS_UPDATED ;
@@ -3635,8 +3639,12 @@ static irqreturn_t tg3_interrupt_tagged(int irq, void *dev_id)* writing non-zero to intr-mbox-0 additional tells the * NIC to stop sending us irqs, engaging "in-intr-handler" * event coalescing. +* +* Flush the mailbox to de-assert the IRQ immediately to prevent +* spurious interrupts. The flush impacts performance but +* excessive spurious interrupts can be worse in some cases. */ -tw32_mailbox (MAILBOX_INTERRUPT_0 +TG3_64BIT_REG_LOW ,0x00000001 ); +tw32_mailbox_f (MAILBOX_INTERRUPT_0 +TG3_64BIT_REG_LOW ,0x00000001 );if (tg3_irq_sync (tp)) goto out;if (netif_rx_schedule_prep (dev)) {
Re: [PATCH revised 10/11][TG3]: Eliminate spurious interrupts.
From: "Michael Chan" <[email protected]>
Date: Sat, 05 May 2007 17:19:31 -0700
Quoted 8 lines
[TG3]: Eliminate spurious interrupts.
Spurious interrupts are often encountered especially on systems using the 8259 PIC mode. This is because the I/O write to deassert the interrupt is posted and won't get to the chip immediately. As a result, the IRQ may remain asserted after the IRQ handler exits, causing spurious interrupts.
Quoted 5 lines
Flush the interrupt mailbox in non-MSI handlers to de-assert the IRQ immediately. This seems to be the most straight forward approach after discussion with Jeff Garzik and David Miller.
Signed-off-by Michael Chan <[email protected]>
Applied.
PATCH 11/11 : Update version and reldate.
In reply to David Miller[TG3]: Update version and reldate.
Update version to 3.76.
drivers/net/tg3.c +2 -2
@@ -64,8 +64,8 @@#define DRV_MODULE_NAME "tg3"#define PFX DRV_MODULE_NAME ": " -#define DRV_MODULE_VERSION "3.75" -#define DRV_MODULE_RELDATE "March 23, 2007" +#define DRV_MODULE_VERSION "3.76" +#define DRV_MODULE_RELDATE "May 5, 2007"#define TG3_DEF_MAC_MODE 0#define TG3_DEF_RX_MODE 0
Re: [PATCH revised 11/11][TG3]: Update version and reldate.
From: "Michael Chan" <[email protected]>
Date: Sat, 05 May 2007 17:21:06 -0700
Quoted 5 lines
[TG3]: Update version and reldate.
Update version to 3.76.
Signed-off-by Michael Chan <[email protected]>
Applied.
On how "topologically big" a system has this resurrection of the PIO read been tried so far?
rick jones
ever the paranoid :)
A message that is not in the archive (sent off-list or not yet received): 1178562469.4859.70.camel@dell
Quoted 14 lines · Michael Chan
Quoted 2 lines · Rick Jones
On how "topologically big" a system has this resurrection of the PIO read been tried so far?
If you're asking how much impact the read will have on performance, the answer is that it will depend on whether you frequently get spurious interrupts or not without the read. The read itself has a measurable impact on performance, but it will offset the performance hit of entering the IRQ handler twice if you get frequent spurious interrupts. It also prevents the kernel from shutting down the IRQ in some extreme cases.
Quoted 2 lines
If you get spurious interrupts infrequently, the read is unfortunately a net loss.
Mostly I was thinking that not all PIO reads are the same "length" and so the effect of the PIO read will vary, perhaps considerably, with the platform, particularly for a very large NUMA platform.
rick jones
Quoted 5 lines · Rick Jones
Mostly I was thinking that not all PIO reads are the same "length" and so the effect of the PIO read will vary, perhaps considerably, with the platform, particularly for a very large NUMA platform.
Oh I see. If the PIO read is "long" on the platform, the performance impact will be bigger. But it probably also means that it takes longer for the write to get to the chip to de-assert the IRQ. Without the read, it is therefore more likely to get an extra interrupt in such a case.