[PATCH 2.6] Add NAPI support to sungem.c

[PATCH 2.6] Add NAPI support to sungem.c

[Harald Welte]

[-- Attachment #1.1: Type: text/plain, Size: 590 bytes --]

Hi Dave!

Please find the attached patch to add NAPI support to the sungem.c
driver (I was annoyed by the fact the pktgen could kill my Powerbook).

Since this is my first hack of a network driver within at least 5 years,
please be kind with me and point me to all the locking bugs and other
mistakes ;)

Any comments welcome,

	Harald

-- 
- Harald Welte <laforge@gnumonks.org>               http://www.gnumonks.org/
============================================================================
Programming is like sex: One mistake and you have to support it your lifetime

[-- Attachment #1.2: linux-2.6.9-rc1-sungem-napi-0.01.patch --]
[-- Type: text/plain, Size: 7542 bytes --]

diff -Nru linux-2.6.8.1-davem269_4/drivers/net/Kconfig linux-2.6.8.1-davem269_4-sungem-napi/drivers/net/Kconfig
--- linux-2.6.8.1-davem269_4/drivers/net/Kconfig	2004-08-14 12:56:00.000000000 +0200
+++ linux-2.6.8.1-davem269_4-sungem-napi/drivers/net/Kconfig	2004-09-14 11:40:40.000000000 +0200
@@ -569,6 +569,22 @@
 	help
 	  Support for the Sun GEM chip, aka Sun GigabitEthernet/P 2.0.  See also
 	  <http://www.sun.com/products-n-solutions/hardware/docs/pdf/806-3985-10.pdf>.
+config SUNGEM_NAPI
+	bool "Use Rx Polling (NAPI) (EXPERIMENTAL)"
+	depends on SUNGEM && EXPERIMENTAL
+	help
+	  NAPI is a new driver API designed to reduce CPU and interrupt load
+	  when the driver is receiving lots of packets from the card. It is
+	  still somewhat experimental and thus not yet enabled by default.
+
+	  If your estimated Rx load is 10kpps or more, or if the card will be
+	  deployed on potentially unfriendly networks (e.g. in a firewall),
+	  then say Y here.
+
+	  See <file:Documentation/networking/NAPI_HOWTO.txt> for more
+	  information.
+
+	  If in doubt, say N.
 
 config NET_VENDOR_3COM
 	bool "3COM cards"
--- linux-2.6.8-rc2-nfpending-tcpwin/drivers/net/sungem.c	2004-07-22 17:48:43.000000000 +0200
+++ linux-2.6.8-rc2-nfpending-tcpwin-napi/drivers/net/sungem.c	2004-09-13 15:48:17.299866224 +0200
@@ -5,6 +5,9 @@
  * 
  * Support for Apple GMAC and assorted PHYs by
  * Benjamin Herrenscmidt (benh@kernel.crashing.org)
+ *
+ * Support for NAPI and NETPOLL 
+ * 	(C) 2004 by Harald Welte <laforge@gnumonks.org>
  * 
  * TODO: 
  *  - Get rid of all those nasty mdelay's and replace them
@@ -187,6 +190,26 @@
 		printk(KERN_DEBUG "%s: mif interrupt\n", gp->dev->name);
 }
 
+static inline void
+gem_irq_disable(struct gem *gp)
+{
+	/* Make sure we won't get any more interrupts */
+	writel(0xffffffff, gp->regs + GREG_IMASK);
+}
+
+static inline void
+gem_irq_enable(struct gem *gp, unsigned int mask)
+{
+	/* We don't want TXDONE, but all other interrupts */
+	writel(mask, gp->regs + GREG_IMASK);
+}
+
+static inline void
+gem_irq_acknowledge(struct gem *gp, unsigned int mask)
+{
+	writel(mask, gp->regs + GREG_IACK);
+}
+
 static int gem_pcs_interrupt(struct net_device *dev, struct gem *gp, u32 gem_status)
 {
 	u32 pcs_istat = readl(gp->regs + PCS_ISTAT);
@@ -537,6 +560,7 @@
 			printk(KERN_DEBUG "%s: no buffer for rx frame\n",
 				gp->dev->name);
 		gp->net_stats.rx_dropped++;
+		gem_irq_acknowledge(gp, GREG_STAT_RXNOBUF);
 	}
 
 	if (gem_status & GREG_STAT_RXTAGERR) {
@@ -545,6 +569,7 @@
 			printk(KERN_DEBUG "%s: corrupt rx tag framing\n",
 				gp->dev->name);
 		gp->net_stats.rx_errors++;
+		gem_irq_acknowledge(gp, GREG_STAT_RXTAGERR);
 
 		goto do_reset;
 	}
@@ -596,6 +621,8 @@
 		printk(KERN_DEBUG "%s: tx interrupt, gem_status: 0x%x\n",
 			gp->dev->name, gem_status);
 
+	gem_irq_acknowledge(gp, GREG_STAT_TXALL|GREG_STAT_TXINTME);
+
 	entry = gp->tx_old;
 	limit = ((gem_status & GREG_STAT_TXNR) >> GREG_STAT_TXNR_SHIFT);
 	while (entry != limit) {
@@ -678,7 +705,11 @@
 	}
 }
 
+#ifdef CONFIG_SUNGEM_NAPI
+static void gem_rx(struct gem *gp, int *work_done, int work_to_do)
+#else
 static void gem_rx(struct gem *gp)
+#endif
 {
 	int entry, drops;
 	u32 done;
@@ -687,6 +718,8 @@
 		printk(KERN_DEBUG "%s: rx interrupt, done: %d, rx_new: %d\n",
 			gp->dev->name, readl(gp->regs + RXDMA_DONE), gp->rx_new);
 
+	gem_irq_acknowledge(gp, GREG_STAT_RXDONE);
+
 	entry = gp->rx_new;
 	drops = 0;
 	done = readl(gp->regs + RXDMA_DONE);
@@ -713,6 +746,11 @@
 				break;
 		}
 
+#ifdef CONFIG_SUNGEM_NAPI
+		if (*work_done >= work_to_do)
+			break;
+
+#endif
 		skb = gp->rx_skbs[entry];
 
 		len = (status & RXDCTRL_BUFSZ) >> 16;
@@ -775,7 +813,12 @@
 		skb->csum = ntohs((status & RXDCTRL_TCPCSUM) ^ 0xffff);
 		skb->ip_summed = CHECKSUM_HW;
 		skb->protocol = eth_type_trans(skb, gp->dev);
+#ifdef CONFIG_SUNGEM_NAPI
+		netif_receive_skb(skb);
+		(*work_done)++;
+#else
 		netif_rx(skb);
+#endif
 
 		gp->net_stats.rx_packets++;
 		gp->net_stats.rx_bytes += len;
@@ -798,7 +841,7 @@
 {
 	struct net_device *dev = dev_id;
 	struct gem *gp = dev->priv;
-	u32 gem_status = readl(gp->regs + GREG_STAT);
+	u32 gem_status = readl(gp->regs + GREG_STAT2);
 
 	/* Swallow interrupts when shutting the chip down */
 	if (gp->hw_running == 0)
@@ -810,10 +853,21 @@
 		if (gem_abnormal_irq(dev, gp, gem_status))
 			goto out;
 	}
+
 	if (gem_status & (GREG_STAT_TXALL | GREG_STAT_TXINTME))
 		gem_tx(dev, gp, gem_status);
-	if (gem_status & GREG_STAT_RXDONE)
+
+	if (gem_status & GREG_STAT_RXDONE) {
+#ifdef CONFIG_SUNGEM_NAPI
+		if (netif_rx_schedule_prep(dev)) {
+			/* Disable interrupts and register for poll */
+			gem_irq_disable(gp);
+			__netif_rx_schedule(dev);
+		}
+#else
 		gem_rx(gp);
+#endif
+	}
 
 out:
 	spin_unlock(&gp->lock);
@@ -821,6 +875,29 @@
 	return IRQ_HANDLED;
 }
 
+#ifdef CONFIG_SUNGEM_NAPI
+static int gem_clean(struct net_device *dev, int *budget)
+{
+	struct gem *gp = dev->priv;
+	int work_to_do = min(*budget, dev->quota);
+	int work_done = 0;
+	u32 gem_status = readl(gp->regs + GREG_STAT2);
+
+	if (gem_status & GREG_STAT_RXDONE)
+		gem_rx(gp, &work_done, work_to_do);
+
+	*budget -= work_done;
+	dev->quota -= work_done;
+
+	if (work_done < work_to_do || !netif_running(dev)) {
+		__netif_rx_complete(dev);
+		gem_irq_enable(gp, GREG_STAT_TXDONE);
+	}
+
+	return (work_done >= work_to_do);
+}
+#endif
+
 static void gem_tx_timeout(struct net_device *dev)
 {
 	struct gem *gp = dev->priv;
@@ -1018,7 +1096,7 @@
 	u32 val;
 
 	/* Make sure we won't get any more interrupts */
-	writel(0xffffffff, gp->regs + GREG_IMASK);
+	gem_irq_disable(gp);
 
 	/* Reset the chip */
 	writel(gp->swrst_base | GREG_SWRST_TXRST | GREG_SWRST_RXRST,
@@ -1055,7 +1133,7 @@
 	(void) readl(gp->regs + MAC_RXCFG);
 	udelay(100);
 
-	writel(GREG_STAT_TXDONE, gp->regs + GREG_IMASK);
+	gem_irq_enable(gp, GREG_STAT_TXDONE);
 
 	writel(RX_RING_SIZE - 4, gp->regs + RXDMA_KICK);
 
@@ -1323,7 +1401,7 @@
 		/* Make sure we don't get interrupts or tx packets */
 		netif_stop_queue(gp->dev);
 
-		writel(0xffffffff, gp->regs + GREG_IMASK);
+		gem_irq_disable(gp);
 
 		/* Reset the chip & rings */
 		gem_stop(gp);
@@ -2220,7 +2298,7 @@
 	spin_lock_irq(&gp->lock);
 
 	gp->opened = 0;	
-	writel(0xffffffff, gp->regs + GREG_IMASK);
+	gem_irq_disable(gp);
 	netif_stop_queue(dev);
 
 	/* Stop chip */
@@ -2264,7 +2342,7 @@
 		/* Stop traffic, mark us closed */
 		netif_device_detach(dev);
 
-		writel(0xffffffff, gp->regs + GREG_IMASK);
+		gem_irq_disable(gp);
 
 		/* Stop chip */
 		gem_stop(gp);
@@ -2651,6 +2729,16 @@
 	return 0;
 }
 
+#ifdef CONFIG_NET_POLL_CONTROLLER
+static void gem_netpoll(struct net_device *dev)
+{
+	struct gem *gp = dev->priv;
+	disable_irq(gp->pdev->irq);
+	gem_interrupt(gp->pdev->irq, dev, NULL);
+	enable_irq(gp->pdev->irq);
+}
+#endif
+
 static int __devinit gem_init_one(struct pci_dev *pdev,
 				  const struct pci_device_id *ent)
 {
@@ -2811,6 +2899,15 @@
 	dev->ethtool_ops = &gem_ethtool_ops;
 	dev->tx_timeout = gem_tx_timeout;
 	dev->watchdog_timeo = 5 * HZ;
+#ifdef CONFIG_SUNGEM_NAPI
+	dev->poll = gem_clean;
+	dev->weight = 64;
+#endif
+
+#ifdef CONFIG_NET_POLL_CONTROLLER
+	dev->poll_controller = gem_netpoll;
+#endif
+
 	dev->change_mtu = gem_change_mtu;
 	dev->irq = pdev->irq;
 	dev->dma = 0;

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2.6] Add NAPI support to sungem.c

[Jeff Garzik]

Harald Welte wrote:
> Hi Dave!
> 
> Please find the attached patch to add NAPI support to the sungem.c
> driver (I was annoyed by the fact the pktgen could kill my Powerbook).
> 
> Since this is my first hack of a network driver within at least 5 years,
> please be kind with me and point me to all the locking bugs and other
> mistakes ;)
> 
> Any comments welcome,
> 
> 	Harald
> 
> 
> 
> ------------------------------------------------------------------------
> 
> diff -Nru linux-2.6.8.1-davem269_4/drivers/net/Kconfig linux-2.6.8.1-davem269_4-sungem-napi/drivers/net/Kconfig
> --- linux-2.6.8.1-davem269_4/drivers/net/Kconfig	2004-08-14 12:56:00.000000000 +0200
> +++ linux-2.6.8.1-davem269_4-sungem-napi/drivers/net/Kconfig	2004-09-14 11:40:40.000000000 +0200
> @@ -569,6 +569,22 @@
>  	help
>  	  Support for the Sun GEM chip, aka Sun GigabitEthernet/P 2.0.  See also
>  	  <http://www.sun.com/products-n-solutions/hardware/docs/pdf/806-3985-10.pdf>.
> +config SUNGEM_NAPI
> +	bool "Use Rx Polling (NAPI) (EXPERIMENTAL)"
> +	depends on SUNGEM && EXPERIMENTAL
> +	help
> +	  NAPI is a new driver API designed to reduce CPU and interrupt load
> +	  when the driver is receiving lots of packets from the card. It is
> +	  still somewhat experimental and thus not yet enabled by default.
> +
> +	  If your estimated Rx load is 10kpps or more, or if the card will be
> +	  deployed on potentially unfriendly networks (e.g. in a firewall),
> +	  then say Y here.
> +
> +	  See <file:Documentation/networking/NAPI_HOWTO.txt> for more
> +	  information.
> +
> +	  If in doubt, say N.
>  
>  config NET_VENDOR_3COM
>  	bool "3COM cards"
> --- linux-2.6.8-rc2-nfpending-tcpwin/drivers/net/sungem.c	2004-07-22 17:48:43.000000000 +0200
> +++ linux-2.6.8-rc2-nfpending-tcpwin-napi/drivers/net/sungem.c	2004-09-13 15:48:17.299866224 +0200
> @@ -5,6 +5,9 @@
>   * 
>   * Support for Apple GMAC and assorted PHYs by
>   * Benjamin Herrenscmidt (benh@kernel.crashing.org)
> + *
> + * Support for NAPI and NETPOLL 
> + * 	(C) 2004 by Harald Welte <laforge@gnumonks.org>
>   * 
>   * TODO: 
>   *  - Get rid of all those nasty mdelay's and replace them
> @@ -187,6 +190,26 @@
>  		printk(KERN_DEBUG "%s: mif interrupt\n", gp->dev->name);
>  }
>  
> +static inline void
> +gem_irq_disable(struct gem *gp)
> +{
> +	/* Make sure we won't get any more interrupts */
> +	writel(0xffffffff, gp->regs + GREG_IMASK);
> +}
> +
> +static inline void
> +gem_irq_enable(struct gem *gp, unsigned int mask)
> +{
> +	/* We don't want TXDONE, but all other interrupts */
> +	writel(mask, gp->regs + GREG_IMASK);
> +}
> +
> +static inline void
> +gem_irq_acknowledge(struct gem *gp, unsigned int mask)
> +{
> +	writel(mask, gp->regs + GREG_IACK);
> +}

PCI posting


>  static int gem_pcs_interrupt(struct net_device *dev, struct gem *gp, u32 gem_status)
>  {
>  	u32 pcs_istat = readl(gp->regs + PCS_ISTAT);
> @@ -537,6 +560,7 @@
>  			printk(KERN_DEBUG "%s: no buffer for rx frame\n",
>  				gp->dev->name);
>  		gp->net_stats.rx_dropped++;
> +		gem_irq_acknowledge(gp, GREG_STAT_RXNOBUF);
>  	}
>  
>  	if (gem_status & GREG_STAT_RXTAGERR) {
> @@ -545,6 +569,7 @@
>  			printk(KERN_DEBUG "%s: corrupt rx tag framing\n",
>  				gp->dev->name);
>  		gp->net_stats.rx_errors++;
> +		gem_irq_acknowledge(gp, GREG_STAT_RXTAGERR);
>  
>  		goto do_reset;
>  	}
> @@ -596,6 +621,8 @@
>  		printk(KERN_DEBUG "%s: tx interrupt, gem_status: 0x%x\n",
>  			gp->dev->name, gem_status);
>  
> +	gem_irq_acknowledge(gp, GREG_STAT_TXALL|GREG_STAT_TXINTME);

Are these in separate functions?  If not, it's usually best to go ahead 
and ACK all the interrupts you are going to handle, in a single IO write 
(usually _before_ you handle the events).


>  	entry = gp->tx_old;
>  	limit = ((gem_status & GREG_STAT_TXNR) >> GREG_STAT_TXNR_SHIFT);
>  	while (entry != limit) {
> @@ -678,7 +705,11 @@
>  	}
>  }
>  
> +#ifdef CONFIG_SUNGEM_NAPI
> +static void gem_rx(struct gem *gp, int *work_done, int work_to_do)
> +#else
>  static void gem_rx(struct gem *gp)
> +#endif
>  {
>  	int entry, drops;
>  	u32 done;
> @@ -687,6 +718,8 @@
>  		printk(KERN_DEBUG "%s: rx interrupt, done: %d, rx_new: %d\n",
>  			gp->dev->name, readl(gp->regs + RXDMA_DONE), gp->rx_new);
>  
> +	gem_irq_acknowledge(gp, GREG_STAT_RXDONE);
> +
>  	entry = gp->rx_new;
>  	drops = 0;
>  	done = readl(gp->regs + RXDMA_DONE);
> @@ -713,6 +746,11 @@
>  				break;
>  		}
>  
> +#ifdef CONFIG_SUNGEM_NAPI
> +		if (*work_done >= work_to_do)
> +			break;
> +
> +#endif
>  		skb = gp->rx_skbs[entry];
>  
>  		len = (status & RXDCTRL_BUFSZ) >> 16;
> @@ -775,7 +813,12 @@
>  		skb->csum = ntohs((status & RXDCTRL_TCPCSUM) ^ 0xffff);
>  		skb->ip_summed = CHECKSUM_HW;
>  		skb->protocol = eth_type_trans(skb, gp->dev);
> +#ifdef CONFIG_SUNGEM_NAPI
> +		netif_receive_skb(skb);
> +		(*work_done)++;

minor nit:  surely it's better code to increment a local variable, then 
store the local to *work_done once all iterations are complete?


> +#else
>  		netif_rx(skb);
> +#endif
>  
>  		gp->net_stats.rx_packets++;
>  		gp->net_stats.rx_bytes += len;
> @@ -798,7 +841,7 @@
>  {
>  	struct net_device *dev = dev_id;
>  	struct gem *gp = dev->priv;
> -	u32 gem_status = readl(gp->regs + GREG_STAT);
> +	u32 gem_status = readl(gp->regs + GREG_STAT2);

what does this do?


>  	/* Swallow interrupts when shutting the chip down */
>  	if (gp->hw_running == 0)
> @@ -810,10 +853,21 @@
>  		if (gem_abnormal_irq(dev, gp, gem_status))
>  			goto out;
>  	}
> +
>  	if (gem_status & (GREG_STAT_TXALL | GREG_STAT_TXINTME))
>  		gem_tx(dev, gp, gem_status);
> -	if (gem_status & GREG_STAT_RXDONE)
> +
> +	if (gem_status & GREG_STAT_RXDONE) {
> +#ifdef CONFIG_SUNGEM_NAPI
> +		if (netif_rx_schedule_prep(dev)) {
> +			/* Disable interrupts and register for poll */
> +			gem_irq_disable(gp);
> +			__netif_rx_schedule(dev);
> +		}
> +#else
>  		gem_rx(gp);
> +#endif
> +	}
>  
>  out:
>  	spin_unlock(&gp->lock);
> @@ -821,6 +875,29 @@
>  	return IRQ_HANDLED;
>  }
>  
> +#ifdef CONFIG_SUNGEM_NAPI
> +static int gem_clean(struct net_device *dev, int *budget)
> +{
> +	struct gem *gp = dev->priv;
> +	int work_to_do = min(*budget, dev->quota);
> +	int work_done = 0;
> +	u32 gem_status = readl(gp->regs + GREG_STAT2);
> +
> +	if (gem_status & GREG_STAT_RXDONE)
> +		gem_rx(gp, &work_done, work_to_do);
> +
> +	*budget -= work_done;
> +	dev->quota -= work_done;
> +
> +	if (work_done < work_to_do || !netif_running(dev)) {
> +		__netif_rx_complete(dev);
> +		gem_irq_enable(gp, GREG_STAT_TXDONE);
> +	}
> +
> +	return (work_done >= work_to_do);
> +}
> +#endif
> +
>  static void gem_tx_timeout(struct net_device *dev)
>  {
>  	struct gem *gp = dev->priv;
> @@ -1018,7 +1096,7 @@
>  	u32 val;
>  
>  	/* Make sure we won't get any more interrupts */
> -	writel(0xffffffff, gp->regs + GREG_IMASK);
> +	gem_irq_disable(gp);
>  
>  	/* Reset the chip */
>  	writel(gp->swrst_base | GREG_SWRST_TXRST | GREG_SWRST_RXRST,
> @@ -1055,7 +1133,7 @@
>  	(void) readl(gp->regs + MAC_RXCFG);
>  	udelay(100);
>  
> -	writel(GREG_STAT_TXDONE, gp->regs + GREG_IMASK);
> +	gem_irq_enable(gp, GREG_STAT_TXDONE);
>  
>  	writel(RX_RING_SIZE - 4, gp->regs + RXDMA_KICK);
>  
> @@ -1323,7 +1401,7 @@
>  		/* Make sure we don't get interrupts or tx packets */
>  		netif_stop_queue(gp->dev);
>  
> -		writel(0xffffffff, gp->regs + GREG_IMASK);
> +		gem_irq_disable(gp);
>  
>  		/* Reset the chip & rings */
>  		gem_stop(gp);
> @@ -2220,7 +2298,7 @@
>  	spin_lock_irq(&gp->lock);
>  
>  	gp->opened = 0;	
> -	writel(0xffffffff, gp->regs + GREG_IMASK);
> +	gem_irq_disable(gp);
>  	netif_stop_queue(dev);
>  
>  	/* Stop chip */
> @@ -2264,7 +2342,7 @@
>  		/* Stop traffic, mark us closed */
>  		netif_device_detach(dev);
>  
> -		writel(0xffffffff, gp->regs + GREG_IMASK);
> +		gem_irq_disable(gp);
>  
>  		/* Stop chip */
>  		gem_stop(gp);

are all these enable/disables inside locks?

	Jeff

Re: [PATCH 2.6] Add NAPI support to sungem.c

[Harald Welte]

[-- Attachment #1.1: Type: text/plain, Size: 2249 bytes --]

Hi Jeff, thanks for your comments.


On Tue, Sep 14, 2004 at 11:56:08AM -0400, Jeff Garzik wrote:
> >+static inline void
> >+gem_irq_acknowledge(struct gem *gp, unsigned int mask)
> >+{
> >+	writel(mask, gp->regs + GREG_IACK);
> >+}
> 
> PCI posting

I'm not sure whether I need mb() or wmb() ? 

Or even better: Can anybody direct me to a document describing the
primitives in more detail than the linux include files? I'm happy to
RTFM.

> >+		gem_irq_acknowledge(gp, GREG_STAT_RXNOBUF);
> >+		gem_irq_acknowledge(gp, GREG_STAT_RXTAGERR);
> >+	gem_irq_acknowledge(gp, GREG_STAT_TXALL|GREG_STAT_TXINTME);
> 
> Are these in separate functions?  If not, it's usually best to go ahead 
> and ACK all the interrupts you are going to handle, in a single IO write 
> (usually _before_ you handle the events).

Not all of them are in the same function, although two of them can be
combined.

> >+#ifdef CONFIG_SUNGEM_NAPI
> >+		netif_receive_skb(skb);
> >+		(*work_done)++;
> 
> minor nit:  surely it's better code to increment a local variable, then 
> store the local to *work_done once all iterations are complete?

That's actually copy&paste from e1000_main.c ;)  

Introducing additional local variables would add #ifdef'ed declarations,
though :(

Would you prefer a tg3-like rx() function that returns work_done rather
than making it a return argument?

> >-	u32 gem_status = readl(gp->regs + GREG_STAT);
> >+	u32 gem_status = readl(gp->regs + GREG_STAT2);
> 
> what does this do?

GREG_STAT implicitly clears event bits 0..6 on read, which according to
NAPI-HOWTO requires that you move all interrupt processing into dev->poll().

GREG_STAT2 is read-only without implicit clear, that's why I've added
the explicit gem_irq_acknowledge() all over the place.

> are all these enable/disables inside locks?

No, enable() is also called from within the dev->poll() function, which
doesn't grab the lock.

> 	Jeff

Partially revised patch attached

-- 
- Harald Welte <laforge@gnumonks.org>               http://www.gnumonks.org/
============================================================================
Programming is like sex: One mistake and you have to support it your lifetime

[-- Attachment #1.2: linux-2.6.9-rc1-sungem-napi-0.02.patch --]
[-- Type: text/plain, Size: 7881 bytes --]

diff -Nru linux-2.6.8.1-davem269_4/drivers/net/Kconfig linux-2.6.8.1-davem269_4-sungem-napi/drivers/net/Kconfig
--- linux-2.6.8.1-davem269_4/drivers/net/Kconfig	2004-08-14 12:56:00.000000000 +0200
+++ linux-2.6.8.1-davem269_4-sungem-napi/drivers/net/Kconfig	2004-09-14 11:40:40.000000000 +0200
@@ -569,6 +569,22 @@
 	help
 	  Support for the Sun GEM chip, aka Sun GigabitEthernet/P 2.0.  See also
 	  <http://www.sun.com/products-n-solutions/hardware/docs/pdf/806-3985-10.pdf>.
+config SUNGEM_NAPI
+	bool "Use Rx Polling (NAPI) (EXPERIMENTAL)"
+	depends on SUNGEM && EXPERIMENTAL
+	help
+	  NAPI is a new driver API designed to reduce CPU and interrupt load
+	  when the driver is receiving lots of packets from the card. It is
+	  still somewhat experimental and thus not yet enabled by default.
+
+	  If your estimated Rx load is 10kpps or more, or if the card will be
+	  deployed on potentially unfriendly networks (e.g. in a firewall),
+	  then say Y here.
+
+	  See <file:Documentation/networking/NAPI_HOWTO.txt> for more
+	  information.
+
+	  If in doubt, say N.
 
 config NET_VENDOR_3COM
 	bool "3COM cards"

--- linux-2.6.8.1-davem269_4/drivers/net/sungem.c	2004-08-14 12:56:23.000000000 +0200
+++ linux-2.6.8.1-davem269_4-sungem-napi/drivers/net/sungem.c	2004-09-14 21:48:04.421487058 +0200
@@ -5,6 +5,9 @@
  * 
  * Support for Apple GMAC and assorted PHYs by
  * Benjamin Herrenscmidt (benh@kernel.crashing.org)
+ *
+ * Support for NAPI and NETPOLL 
+ * 	(C) 2004 by Harald Welte <laforge@gnumonks.org>
  * 
  * TODO: 
  *  - Get rid of all those nasty mdelay's and replace them
@@ -71,8 +74,8 @@
 			 SUPPORTED_1000baseT_Half | SUPPORTED_1000baseT_Full)
 
 #define DRV_NAME	"sungem"
-#define DRV_VERSION	"0.98"
-#define DRV_RELDATE	"8/24/03"
+#define DRV_VERSION	"0.98-napi"
+#define DRV_RELDATE	"9/14/04"
 #define DRV_AUTHOR	"David S. Miller (davem@redhat.com)"
 
 static char version[] __devinitdata =
@@ -187,6 +190,29 @@
 		printk(KERN_DEBUG "%s: mif interrupt\n", gp->dev->name);
 }
 
+static inline void
+gem_irq_disable(struct gem *gp)
+{
+	/* Make sure we won't get any more interrupts */
+	writel(0xffffffff, gp->regs + GREG_IMASK);
+	mb();
+}
+
+static inline void
+gem_irq_enable(struct gem *gp, unsigned int mask)
+{
+	/* We don't want TXDONE, but all other interrupts */
+	writel(mask, gp->regs + GREG_IMASK);
+	mb();
+}
+
+static inline void
+gem_irq_acknowledge(struct gem *gp, unsigned int mask)
+{
+	writel(mask, gp->regs + GREG_IACK);
+	mb();
+}
+
 static int gem_pcs_interrupt(struct net_device *dev, struct gem *gp, u32 gem_status)
 {
 	u32 pcs_istat = readl(gp->regs + PCS_ISTAT);
@@ -531,6 +557,10 @@
  */
 static int gem_abnormal_irq(struct net_device *dev, struct gem *gp, u32 gem_status)
 {
+	/* only those two are part of bits 0..6 and need explicit
+	 * acknowledgement via GREG_IACK */
+	gem_irq_acknowledge(gp, (GREG_STAT_RXNOBUF|GREG_STAT_RXTAGERR));
+
 	if (gem_status & GREG_STAT_RXNOBUF) {
 		/* Frame arrived, no free RX buffers available. */
 		if (netif_msg_rx_err(gp))
@@ -596,6 +626,8 @@
 		printk(KERN_DEBUG "%s: tx interrupt, gem_status: 0x%x\n",
 			gp->dev->name, gem_status);
 
+	gem_irq_acknowledge(gp, GREG_STAT_TXALL|GREG_STAT_TXINTME);
+
 	entry = gp->tx_old;
 	limit = ((gem_status & GREG_STAT_TXNR) >> GREG_STAT_TXNR_SHIFT);
 	while (entry != limit) {
@@ -678,7 +710,11 @@
 	}
 }
 
+#ifdef CONFIG_SUNGEM_NAPI
+static void gem_rx(struct gem *gp, int *work_done, int work_to_do)
+#else
 static void gem_rx(struct gem *gp)
+#endif
 {
 	int entry, drops;
 	u32 done;
@@ -687,6 +723,8 @@
 		printk(KERN_DEBUG "%s: rx interrupt, done: %d, rx_new: %d\n",
 			gp->dev->name, readl(gp->regs + RXDMA_DONE), gp->rx_new);
 
+	gem_irq_acknowledge(gp, GREG_STAT_RXDONE);
+
 	entry = gp->rx_new;
 	drops = 0;
 	done = readl(gp->regs + RXDMA_DONE);
@@ -713,6 +751,11 @@
 				break;
 		}
 
+#ifdef CONFIG_SUNGEM_NAPI
+		if (*work_done >= work_to_do)
+			break;
+
+#endif
 		skb = gp->rx_skbs[entry];
 
 		len = (status & RXDCTRL_BUFSZ) >> 16;
@@ -775,7 +818,12 @@
 		skb->csum = ntohs((status & RXDCTRL_TCPCSUM) ^ 0xffff);
 		skb->ip_summed = CHECKSUM_HW;
 		skb->protocol = eth_type_trans(skb, gp->dev);
+#ifdef CONFIG_SUNGEM_NAPI
+		netif_receive_skb(skb);
+		(*work_done)++;
+#else
 		netif_rx(skb);
+#endif
 
 		gp->net_stats.rx_packets++;
 		gp->net_stats.rx_bytes += len;
@@ -798,7 +846,7 @@
 {
 	struct net_device *dev = dev_id;
 	struct gem *gp = dev->priv;
-	u32 gem_status = readl(gp->regs + GREG_STAT);
+	u32 gem_status = readl(gp->regs + GREG_STAT2);
 
 	/* Swallow interrupts when shutting the chip down */
 	if (gp->hw_running == 0)
@@ -810,10 +858,21 @@
 		if (gem_abnormal_irq(dev, gp, gem_status))
 			goto out;
 	}
+
 	if (gem_status & (GREG_STAT_TXALL | GREG_STAT_TXINTME))
 		gem_tx(dev, gp, gem_status);
-	if (gem_status & GREG_STAT_RXDONE)
+
+	if (gem_status & GREG_STAT_RXDONE) {
+#ifdef CONFIG_SUNGEM_NAPI
+		if (netif_rx_schedule_prep(dev)) {
+			/* Disable interrupts and register for poll */
+			gem_irq_disable(gp);
+			__netif_rx_schedule(dev);
+		}
+#else
 		gem_rx(gp);
+#endif
+	}
 
 out:
 	spin_unlock(&gp->lock);
@@ -821,6 +880,29 @@
 	return IRQ_HANDLED;
 }
 
+#ifdef CONFIG_SUNGEM_NAPI
+static int gem_clean(struct net_device *dev, int *budget)
+{
+	struct gem *gp = dev->priv;
+	int work_to_do = min(*budget, dev->quota);
+	int work_done = 0;
+	u32 gem_status = readl(gp->regs + GREG_STAT2);
+
+	if (gem_status & GREG_STAT_RXDONE)
+		gem_rx(gp, &work_done, work_to_do);
+
+	*budget -= work_done;
+	dev->quota -= work_done;
+
+	if (work_done < work_to_do || !netif_running(dev)) {
+		__netif_rx_complete(dev);
+		gem_irq_enable(gp, GREG_STAT_TXDONE);
+	}
+
+	return (work_done >= work_to_do);
+}
+#endif
+
 static void gem_tx_timeout(struct net_device *dev)
 {
 	struct gem *gp = dev->priv;
@@ -1018,7 +1100,7 @@
 	u32 val;
 
 	/* Make sure we won't get any more interrupts */
-	writel(0xffffffff, gp->regs + GREG_IMASK);
+	gem_irq_disable(gp);
 
 	/* Reset the chip */
 	writel(gp->swrst_base | GREG_SWRST_TXRST | GREG_SWRST_RXRST,
@@ -1055,7 +1137,7 @@
 	(void) readl(gp->regs + MAC_RXCFG);
 	udelay(100);
 
-	writel(GREG_STAT_TXDONE, gp->regs + GREG_IMASK);
+	gem_irq_enable(gp, GREG_STAT_TXDONE);
 
 	writel(RX_RING_SIZE - 4, gp->regs + RXDMA_KICK);
 
@@ -1323,7 +1405,7 @@
 		/* Make sure we don't get interrupts or tx packets */
 		netif_stop_queue(gp->dev);
 
-		writel(0xffffffff, gp->regs + GREG_IMASK);
+		gem_irq_disable(gp);
 
 		/* Reset the chip & rings */
 		gem_stop(gp);
@@ -2218,7 +2300,7 @@
 	spin_lock_irq(&gp->lock);
 
 	gp->opened = 0;	
-	writel(0xffffffff, gp->regs + GREG_IMASK);
+	gem_irq_disable(gp);
 	netif_stop_queue(dev);
 
 	/* Stop chip */
@@ -2262,7 +2344,7 @@
 		/* Stop traffic, mark us closed */
 		netif_device_detach(dev);
 
-		writel(0xffffffff, gp->regs + GREG_IMASK);
+		gem_irq_disable(gp);
 
 		/* Stop chip */
 		gem_stop(gp);
@@ -2649,6 +2731,16 @@
 	return 0;
 }
 
+#ifdef CONFIG_NET_POLL_CONTROLLER
+static void gem_netpoll(struct net_device *dev)
+{
+	struct gem *gp = dev->priv;
+	disable_irq(gp->pdev->irq);
+	gem_interrupt(gp->pdev->irq, dev, NULL);
+	enable_irq(gp->pdev->irq);
+}
+#endif
+
 static int __devinit gem_init_one(struct pci_dev *pdev,
 				  const struct pci_device_id *ent)
 {
@@ -2809,6 +2901,15 @@
 	dev->ethtool_ops = &gem_ethtool_ops;
 	dev->tx_timeout = gem_tx_timeout;
 	dev->watchdog_timeo = 5 * HZ;
+#ifdef CONFIG_SUNGEM_NAPI
+	dev->poll = gem_clean;
+	dev->weight = 64;
+#endif
+
+#ifdef CONFIG_NET_POLL_CONTROLLER
+	dev->poll_controller = gem_netpoll;
+#endif
+
 	dev->change_mtu = gem_change_mtu;
 	dev->irq = pdev->irq;
 	dev->dma = 0;

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2.6] Add NAPI support to sungem.c

[David S. Miller]

On Tue, 14 Sep 2004 21:50:52 +0200
Harald Welte <laforge@gnumonks.org> wrote:

> Hi Jeff, thanks for your comments.
> 
> 
> On Tue, Sep 14, 2004 at 11:56:08AM -0400, Jeff Garzik wrote:
> > >+static inline void
> > >+gem_irq_acknowledge(struct gem *gp, unsigned int mask)
> > >+{
> > >+	writel(mask, gp->regs + GREG_IACK);
> > >+}
> > 
> > PCI posting
> 
> I'm not sure whether I need mb() or wmb() ? 

No, it's something else.

If you do something like, as one example:

	/* Chip resets require 40msec settle delay. */
	writel(FOO_RESET, regs + FOO);
	udelay(40);

It's not going to work properly because the writel() can
be delayed quite a long time, so you have to force the
writel() to completion somehow, the usual way is via:

	writel(FOO_RESET, regs + FOO);
	(void) readl(regs + FOO);
	udelay(40);

or similar.  The read back of the register forces the
write to complete on the PCI bus.

But be very careful and don't do the readl() readbacks too
much, especially in critical code paths, because they are
expensive as they cause a full round-trip to occur on the PCI
bus from the cpu.

Make sure you absolutely do need the read back.

Jeff, can you comment on why it is needed in this specific
case of writing the chip interrupt enabling?  I think it might
not be.

Re: [PATCH 2.6] Add NAPI support to sungem.c

[David S. Miller]

Harald.... check current BK it has NAPI support added
to the sungem.c driver already, it was written by Eric Lemoine
with some fixes by myself and benh :-)

I guess these are one of the old mails you're resending today
after your outage.

Re: [PATCH 2.6] Add NAPI support to sungem.c

[Harald Welte]

[-- Attachment #1: Type: text/plain, Size: 799 bytes --]

On Tue, Sep 14, 2004 at 12:43:15PM -0700, David S. Miller wrote:
> 
> Harald.... check current BK it has NAPI support added
> to the sungem.c driver already, it was written by Eric Lemoine
> with some fixes by myself and benh :-)

Gnah, I hate duplicated efforts.  Well, I learned a bit about NIC
drivers, though :(

> I guess these are one of the old mails you're resending today
> after your outage.

No, unfortunately not :(

Before I start NAPI-enabling natsemi.c (and again duplicate work):  Do
you know of anybody working on this yet?

-- 
- Harald Welte <laforge@gnumonks.org>               http://www.gnumonks.org/
============================================================================
Programming is like sex: One mistake and you have to support it your lifetime

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2.6] Add NAPI support to sungem.c

[David S. Miller]

On Tue, 14 Sep 2004 22:02:38 +0200
Harald Welte <laforge@gnumonks.org> wrote:

> Before I start NAPI-enabling natsemi.c (and again duplicate work):  Do
> you know of anybody working on this yet?

No I do not, have at it.

I'm going to try and work on sunhme myself.

Re: [PATCH 2.6] Add NAPI support to sungem.c

[Jeff Garzik]

On Tue, Sep 14, 2004 at 01:48:49PM -0700, David S. Miller wrote:
> On Tue, 14 Sep 2004 22:02:38 +0200
> Harald Welte <laforge@gnumonks.org> wrote:
> 
> > Before I start NAPI-enabling natsemi.c (and again duplicate work):  Do
> > you know of anybody working on this yet?
> 
> No I do not, have at it.

Talk to Manfred Spraul, he's pokes fixes natsemi bugs and pokes at it
now and again...

	Jeff

Re: [PATCH 2.6] Add NAPI support to sungem.c

[Manfred Spraul]

Jeff Garzik wrote:

>On Tue, Sep 14, 2004 at 01:48:49PM -0700, David S. Miller wrote:
>  
>
>>On Tue, 14 Sep 2004 22:02:38 +0200
>>Harald Welte <laforge@gnumonks.org> wrote:
>>
>>    
>>
>>>Before I start NAPI-enabling natsemi.c (and again duplicate work):  Do
>>>you know of anybody working on this yet?
>>>      
>>>
>>No I do not, have at it.
>>    
>>
>
>Talk to Manfred Spraul, he's pokes fixes natsemi bugs and pokes at it
>now and again...
>
>  
>
I don't have a NAPI patch in my tree, but I can't guarantee that there 
won't be an embedded system guy who suddenly appears with a patch.

--
    Manfred

Re: [PATCH 2.6] Add NAPI support to sungem.c

[Eric Lemoine]

On Tue, 14 Sep 2004 22:02:38 +0200, Harald Welte <laforge@gnumonks.org> wrote:
> On Tue, Sep 14, 2004 at 12:43:15PM -0700, David S. Miller wrote:
> >
> > Harald.... check current BK it has NAPI support added
> > to the sungem.c driver already, it was written by Eric Lemoine
> > with some fixes by myself and benh :-)
> 
> Gnah, I hate duplicated efforts.  Well, I learned a bit about NIC
> drivers, though :(

Your patch has the netpoll bits while current BK does not.

> Before I start NAPI-enabling natsemi.c (and again duplicate work):  Do
> you know of anybody working on this yet?

I'm not ;-)

PS: If you happened to experiment with the NAPIfied SunGEM driver on
your setup, could you please share the data. Thx.

-- 
Eric

Re: [PATCH 2.6] Add NAPI support to sungem.c

[Harald Welte]

[-- Attachment #1: Type: text/plain, Size: 698 bytes --]

On Wed, Sep 15, 2004 at 07:50:28AM +0200, Eric Lemoine wrote:
> Your patch has the netpoll bits while current BK does not.

Well, it's straight forward to merge them into BK...

> PS: If you happened to experiment with the NAPIfied SunGEM driver on
> your setup, could you please share the data. Thx.

I didn't do any measurements so far... I only know that my Powerbook
doesn't stop responding anymore when I flood it with pktgen ;)  

> Eric

-- 
- Harald Welte <laforge@gnumonks.org>               http://www.gnumonks.org/
============================================================================
Programming is like sex: One mistake and you have to support it your lifetime

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]