modify the interrupt handler so it only processes the rings once, rather

than looping over them until it runs out of work to do.

in my testing i have found that under what i consider high pps
(>160kpps) ix would loop 4 or 5 times in the interrupt handler,
where each loop does a bus_space_read and the mclgeti loop (ie, rx
dequeue followed by rx ring fill).

looping in the isr is bad for several reasons:

firstly, the chip does interrupt mitigation so you have a
decent/predictable amount of work to do in the isr. your first loop
will do that chunk of work (ie, it pulls off 50ish packets), and
then the successive looping aggressively pull one or two packets
off the rx ring. these extra loops work against the benefit that
interrupt mitigation provides.

bus space reads are slow. we should avoid doing them where possible
(but we should always do them when necessary).

doing the loop 5 times per isr works against the mclgeti semantics.
it knows a nic is busy and therefore needs more rx descriptors by
watching to see when the nic uses all of its descriptors between
interrupts. if we're aggressively pulling packets off by looping
in the isr then we're skewing this check.

ok deraadt@ claudio@
testing by phessler@ bluhm@ and me in production

1 files changed, 30 insertions, 35 deletions

diff --git a/sys/dev/pci/if_ix.c b/sys/dev/pci/if_ix.c
index 5ce67675bee..2cc1f905c20 100644
--- a/sys/dev/pci/if_ix.c
+++ b/sys/dev/pci/if_ix.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if_ix.c,v 1.49 2011/04/07 15:30:16 miod Exp $	*/
+/*	$OpenBSD: if_ix.c,v 1.50 2011/04/13 00:14:18 dlg Exp $	*/
 
 /******************************************************************************
 
@@ -756,53 +756,48 @@ ixgbe_legacy_irq(void *arg)
 {
 	struct ix_softc	*sc = (struct ix_softc *)arg;
 	struct ifnet	*ifp = &sc->arpcom.ac_if;
-	uint32_t	 reg_eicr;
 	struct tx_ring	*txr = sc->tx_rings;
 	struct rx_ring	*rxr = sc->rx_rings;
 	struct ixgbe_hw	*hw = &sc->hw;
-	int		 claimed = 0, refill = 0;
-
-	for (;;) {
-		reg_eicr = IXGBE_READ_REG(&sc->hw, IXGBE_EICR);
-		if (reg_eicr == 0)
-			break;
+	uint32_t	 reg_eicr;
+	int		 refill = 0;
 
-		claimed = 1;
-		refill = 0;
+	reg_eicr = IXGBE_READ_REG(&sc->hw, IXGBE_EICR);
+	if (reg_eicr == 0)
+		return (0);
 
-		if (ifp->if_flags & IFF_RUNNING) {
-			ixgbe_rxeof(rxr, -1);
-			ixgbe_txeof(txr);
-			refill = 1;
-		}
+	if (ifp->if_flags & IFF_RUNNING) {
+		ixgbe_rxeof(rxr, -1);
+		ixgbe_txeof(txr);
+		refill = 1;
+	}
 
-		/* Check for fan failure */
-		if ((hw->phy.media_type == ixgbe_media_type_copper) &&
-		    (reg_eicr & IXGBE_EICR_GPI_SDP1)) {
-	                printf("%s: \nCRITICAL: FAN FAILURE!! "
-			    "REPLACE IMMEDIATELY!!\n", ifp->if_xname);
-			IXGBE_WRITE_REG(&sc->hw, IXGBE_EIMS,
-			    IXGBE_EICR_GPI_SDP1);
-		}
+	/* Check for fan failure */
+	if ((hw->phy.media_type == ixgbe_media_type_copper) &&
+	    (reg_eicr & IXGBE_EICR_GPI_SDP1)) {
+                printf("%s: \nCRITICAL: FAN FAILURE!! "
+		    "REPLACE IMMEDIATELY!!\n", ifp->if_xname);
+		IXGBE_WRITE_REG(&sc->hw, IXGBE_EIMS,
+		    IXGBE_EICR_GPI_SDP1);
+	}
 
-		/* Link status change */
-		if (reg_eicr & IXGBE_EICR_LSC) {
-			timeout_del(&sc->timer);
-		        ixgbe_update_link_status(sc);
-			timeout_add_sec(&sc->timer, 1);
-		}
+	/* Link status change */
+	if (reg_eicr & IXGBE_EICR_LSC) {
+		timeout_del(&sc->timer);
+	        ixgbe_update_link_status(sc);
+		timeout_add_sec(&sc->timer, 1);
+	}
 
-		if (refill && ixgbe_rxfill(rxr)) {
-			/* Advance the Rx Queue "Tail Pointer" */
-			IXGBE_WRITE_REG(&sc->hw, IXGBE_RDT(rxr->me),
-			    rxr->last_rx_desc_filled);
-		}
+	if (refill && ixgbe_rxfill(rxr)) {
+		/* Advance the Rx Queue "Tail Pointer" */
+		IXGBE_WRITE_REG(&sc->hw, IXGBE_RDT(rxr->me),
+		    rxr->last_rx_desc_filled);
 	}
 
 	if (ifp->if_flags & IFF_RUNNING && !IFQ_IS_EMPTY(&ifp->if_snd))
 		ixgbe_start_locked(txr, ifp);
 
-	return (claimed);
+	return (1);
 }
 
 /*********************************************************************