Re: [PATCH 10/11][TG3]: Reduce spurious interrupts.

Re: [PATCH 10/11][TG3]: Reduce spurious interrupts.

[Jeff Garzik]

Michael Chan wrote:
> [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.
> 
> Signed-off-by: Michael Chan <mchan@broadcom.com>

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

[PATCH 10/11][TG3]: Reduce spurious interrupts.

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

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 <mchan@broadcom.com>

diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index b3681e4..c264eb0 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -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)
diff --git a/drivers/net/tg3.h b/drivers/net/tg3.h
index d88a859..7413643 100644
--- a/drivers/net/tg3.h
+++ b/drivers/net/tg3.h
@@ -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 */

Re: [PATCH 10/11][TG3]: Reduce spurious interrupts.

[Michael Chan]

On Fri, 2007-05-04 at 20:42 -0400, Jeff Garzik wrote:
> Michael Chan wrote:
> > [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.
> > 
> > Signed-off-by: Michael Chan <mchan@broadcom.com>
> 
> 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?

Re: [PATCH 10/11][TG3]: Reduce spurious interrupts.

[David Miller]

From: "Michael Chan" <mchan@broadcom.com>
Date: Fri, 04 May 2007 18:45:58 -0700

> 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.

Re: [PATCH 10/11][TG3]: Reduce spurious interrupts.

[Michael Chan]

David Miller wrote:

> From: "Michael Chan" <mchan@broadcom.com>
> Date: Fri, 04 May 2007 18:45:58 -0700
> 
> > 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.
> 

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.

Re: [PATCH 10/11][TG3]: Reduce spurious interrupts.

[David Miller]

From: "Michael Chan" <mchan@broadcom.com>
Date: Fri, 4 May 2007 19:50:10 -0700

> 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.

Re: [PATCH 10/11][TG3]: Reduce spurious interrupts.

[Michael Chan]

On Fri, 2007-05-04 at 20:47 -0700, David Miller wrote:
> From: "Michael Chan" <mchan@broadcom.com>
> Date: Fri, 4 May 2007 19:50:10 -0700
> 
> > 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 revised 10/11][TG3]: Eliminate spurious interrupts.

[Michael Chan]

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

Signed-off-by: Michael Chan <mchan@broadcom.com>

diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index da1ab01..70b8c5e 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -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)) {

[PATCH revised 11/11][TG3]: Update version and reldate.

[Michael Chan]

[TG3]: Update version and reldate.

Update version to 3.76.

Signed-off-by: Michael Chan <mchan@broadcom.com>

diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 70b8c5e..184c275 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -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 10/11][TG3]: Eliminate spurious interrupts.

[David Miller]

From: "Michael Chan" <mchan@broadcom.com>
Date: Sat, 05 May 2007 17:19:31 -0700

> [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.
> 
> Signed-off-by: Michael Chan <mchan@broadcom.com>

Applied.

Re: [PATCH revised 11/11][TG3]: Update version and reldate.

[David Miller]

From: "Michael Chan" <mchan@broadcom.com>
Date: Sat, 05 May 2007 17:21:06 -0700

> [TG3]: Update version and reldate.
> 
> Update version to 3.76.
> 
> Signed-off-by: Michael Chan <mchan@broadcom.com>

Applied.

Re: [PATCH 10/11][TG3]: Reduce spurious interrupts.

[Rick Jones]

On how "topologically big" a system has this resurrection of the PIO read been 
tried so far?

rick jones
ever the paranoid :)

Re: [PATCH 10/11][TG3]: Reduce spurious interrupts.

[Rick Jones]

Michael Chan wrote:
> On Mon, 2007-05-07 at 09:39 -0700, Rick Jones wrote:
> 
>>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.
> 
> 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

Re: [PATCH 10/11][TG3]: Reduce spurious interrupts.

[Michael Chan]

On Mon, 2007-05-07 at 10:44 -0700, Rick Jones wrote:
> 
> 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.