: powerpc/cell spidernet ethernet driver fixes

The following series of patches implement some fixes and performance improvements for the Spedernet ethernet device driver. The high point of the patch series is some code to implement a low-watermark interrupt on the transmit queue. The bundle of patches raises transmit performance from some embarassingly low value to a reasonable 730 Megabits per second for 1500 byte packets.

Please note that the spider is an ethernet controller that is integrated into the south bridge, and is thus available only on Cell-based platforms.

These have been well-tested over the last few weeks. Please apply.

--linas

This patch increases the Burst Address alignment from 64 to 1024 in the Spidernet driver. This improves transmit performance for large packets from about 100Mbps to 300-400Mbps.

----

 drivers/net/spider_net.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6.18-rc3-mm2/drivers/net/spider_net.h
===================================================================
@@ -209,7 +209,7 @@ extern char spider_net_driver_name[];
 #define SPIDER_NET_DMA_RX_FEND_VALUE	0x00030003
 /* to set TX_DMA_EN */
 #define SPIDER_NET_TX_DMA_EN		0x80000000
-#define SPIDER_NET_GDTDCEIDIS		0x00000002
+#define SPIDER_NET_GDTDCEIDIS		0x00000302
 #define SPIDER_NET_DMA_TX_VALUE		SPIDER_NET_TX_DMA_EN | \
 					SPIDER_NET_GDTDCEIDIS
 #define SPIDER_NET_DMA_TX_FEND_VALUE	0x00030003

Implement basic low-watermark support for the transmit queue.

The basic idea of a low-watermark interrupt is as follows. The device driver queues up a bunch of packets for the hardware to transmit, and then kicks he hardware to get it started. As the hardware drains the queue of pending, untransmitted packets, the device driver will want to know when the queue is almost empty, so that it can queue some more packets.

This is accomplished by setting the DESCR_TXDESFLG flag in one of the packets. When the hardware sees this flag, it will interrupt the device driver. Because this flag is on a fixed packet, rather than at fixed location in the queue, the code below needs to move the flag as more packets are queued up. This implementation attempts to keep te flag at about 3/4's of the way into the queue.

This patch boosts driver performance from about 300-400Mbps for 1500 byte packets, to about 710-740Mbps.

----

 drivers/net/spider_net.c |   56 ++++++++++++++++++++++++++++++++++++++++++-----
 drivers/net/spider_net.h |    6 +++--
 2 files changed, 55 insertions(+), 7 deletions(-)

Index: linux-2.6.18-rc3-mm2/drivers/net/spider_net.c
===================================================================
@@ -700,6 +700,39 @@ spider_net_release_tx_descr(struct spide
 	dev_kfree_skb_any(skb);
 }
 
+static void
+spider_net_set_low_watermark(struct spider_net_card *card)
+{
+	int status;
+	int cnt=0;
+	int i;
+	struct spider_net_descr *descr = card->tx_chain.tail;
+
+	/* Measure the length of the queue. */
+	while (descr != card->tx_chain.head) {
+		status = descr->dmac_cmd_status & SPIDER_NET_DESCR_NOT_IN_USE;
+		if (status == SPIDER_NET_DESCR_NOT_IN_USE)
+			break;
+		descr = descr->next;
+		cnt++;
+	}
+	if (cnt == 0)
+		return;
+
+	/* Set low-watermark 3/4th's of the way into the queue. */
+	descr = card->tx_chain.tail;
+	cnt = (cnt*3)/4;
+	for (i=0;i<cnt; i++)
+		descr = descr->next;
+
+	/* Set the new watermark, clear the old wtermark */
+	descr->dmac_cmd_status |= SPIDER_NET_DESCR_TXDESFLG;
+	if (card->low_watermark && card->low_watermark != descr)
+		card->low_watermark->dmac_cmd_status =
+		     card->low_watermark->dmac_cmd_status & ~SPIDER_NET_DESCR_TXDESFLG;
+	card->low_watermark = descr;
+}
+
 /**
  * spider_net_release_tx_chain - processes sent tx descriptors
  * @card: adapter structure

@@ -717,6 +750,7 @@ spider_net_release_tx_chain(struct spide
 {
 	struct spider_net_descr_chain *chain = &card->tx_chain;
 	int status;
+	int rc=0;
 
 	spider_net_read_reg(card, SPIDER_NET_GDTDMACCNTR);
 

@@ -729,8 +763,10 @@ spider_net_release_tx_chain(struct spide
 			break;
 
 		case SPIDER_NET_DESCR_CARDOWNED:
-			if (!brutal)
-				return 1;
+			if (!brutal) {
+				rc = 1;
+				goto done;
+			}
 			/* fallthrough, if we release the descriptors
 			 * brutally (then we don't care about
 			 * SPIDER_NET_DESCR_CARDOWNED) */

@@ -747,12 +783,15 @@ spider_net_release_tx_chain(struct spide
 
 		default:
 			card->netdev_stats.tx_dropped++;
-			return 1;
+			rc = 1;
+			goto done;
 		}
 		spider_net_release_tx_descr(card);
 	}
-
-	return 0;
+done:
+	if (rc == 1)
+		spider_net_set_low_watermark(card);
+	return rc;
 }
 
 /**

@@ -1453,6 +1492,10 @@ spider_net_interrupt(int irq, void *ptr,
 		spider_net_rx_irq_off(card);
 		netif_rx_schedule(netdev);
 	}
+	if (status_reg & SPIDER_NET_TXINT ) {
+		spider_net_cleanup_tx_ring(card);
+		netif_wake_queue(netdev);
+	}
 
 	if (status_reg & SPIDER_NET_ERRINT )
 		spider_net_handle_error_irq(card, status_reg);

@@ -1615,6 +1658,9 @@ spider_net_open(struct net_device *netde
 			card->descr,
 			PCI_DMA_TODEVICE, tx_descriptors))
 		goto alloc_tx_failed;
+
+	card->low_watermark = NULL;
+
 	if (spider_net_init_chain(card, &card->rx_chain,
 			card->descr + tx_descriptors,
 			PCI_DMA_FROMDEVICE, rx_descriptors))

Index: linux-2.6.18-rc3-mm2/drivers/net/spider_net.h
===================================================================
@@ -47,7 +47,7 @@ extern char spider_net_driver_name[];
 #define SPIDER_NET_TX_DESCRIPTORS_MIN		16
 #define SPIDER_NET_TX_DESCRIPTORS_MAX		512
 
-#define SPIDER_NET_TX_TIMER			20
+#define SPIDER_NET_TX_TIMER			(HZ/5)
 
 #define SPIDER_NET_RX_CSUM_DEFAULT		1
 

@@ -209,7 +209,7 @@ extern char spider_net_driver_name[];
 #define SPIDER_NET_DMA_RX_FEND_VALUE	0x00030003
 /* to set TX_DMA_EN */
 #define SPIDER_NET_TX_DMA_EN		0x80000000
-#define SPIDER_NET_GDTDCEIDIS		0x00000302
+#define SPIDER_NET_GDTDCEIDIS		0x00000300
 #define SPIDER_NET_DMA_TX_VALUE		SPIDER_NET_TX_DMA_EN | \
 					SPIDER_NET_GDTDCEIDIS
 #define SPIDER_NET_DMA_TX_FEND_VALUE	0x00030003

@@ -349,6 +349,7 @@ enum spider_net_int2_status {
 #define SPIDER_NET_DESCR_FORCE_END		0x50000000 /* used in rx and tx */
 #define SPIDER_NET_DESCR_CARDOWNED		0xA0000000 /* used in rx and tx */
 #define SPIDER_NET_DESCR_NOT_IN_USE		0xF0000000
+#define SPIDER_NET_DESCR_TXDESFLG		0x00800000
 
 struct spider_net_descr {
 	/* as defined by the hardware */

@@ -424,6 +425,7 @@ struct spider_net_card {
 
 	struct spider_net_descr_chain tx_chain;
 	struct spider_net_descr_chain rx_chain;
+	struct spider_net_descr *low_watermark;
 
 	struct net_device_stats netdev_stats;
 

Sounds good (without actually looking at the code though :), that was a long required improvement to that driver. Also, we should probably look into using NAPI polling for tx completion queue as well, no ?

Cheers,
Ben.

Just for a lark, I tried using NAPI polling, while disabling all TX interrupts. Performance was a disaster: 8Mbits/sec, fom which I conclude that the tcp ack packets do not flow back fast enough to allw reliance on NAPI polling for transmit.

I was able to get as high as 960 Mbits/sec in unusal circumstances, at 100% cpu usage. Oprofile indicates that the next major improvement would be to add scatter/gather, which I'll take a shot at next week, if I don't get interrupted. However, I'm getting interrupted a lot these days.

--linas

From: [email protected] (Linas Vepstas)
Date: Fri, 18 Aug 2006 14:23:56 -0500

The idea is to use NAPI polling with TX interrupts disabled.

We're not saying to use the RX interrupt as the trigger for RX and TX work. Rather, either of RX or TX interrupt will schedule the NAPI poll.

The idea of a low-watermark mark interrupt is that there are *zero* interrupts, as long as the kernel keeps feeding packets to the device.

In my last email to you, I attached a real-life vmstat trace showing exactly zero interrupts over a two minute period.

However, the zero-interrupt scenario only occurs if the kernel is actually feeding packets to the driver. If the socket beffers are small, then the app blocks, the kernel is idle, and there are not enough tcp ack packets coming back the other way to actually get the NAPI polling to keep the adapter fed.

And, for a lark, this is exactly what I did. Just to see. Because there are so few ack packets, there are very few RX interrupts -- not enough to get NAPI to actually keep the device busy.

------
I'm somewhat disoriened from this conversation. Its presumably clear that low-watermark mechanisms are superior to NAPI.

or low-function hardware; it adds nothing to this particular situation. Why are we talking about this?

--linas

From: [email protected] (Linas Vepstas)
Date: Fri, 18 Aug 2006 17:46:18 -0500

You're misreading me. TX interrupts are intended to be "enabled" and trigger NAPI polls. TX IRQ enabled, enabled :-)

If you want to eliminate them if the kernel keeps hopping into the ->hard_start_xmit() via hw interrupt mitigation or whatever, that's fine. But if you do need to do TX interrupt processing, do it in NAPI ->poll().

NAPI is meant to give fairness to all devices receiving packets in the system, particularly in times of high load or overload.

And equally importantly, it allows you to run the majority of your interrupt handler in software IRQ context. This allows not only your locking to be simpler, but it also allows things like oprofile to monitor almost your entire IRQ processing path even with just timer interrupt based oprofile profiling.

I see you moving TX reclaim into tasklets and stuff. I've vehemently against that because you wouldn't need it in order to move TX processing into software interrupts if you did it all in NAPI ->poll().

I don't understand what you are saying. If I call the transmit queue cleanup code from the poll() routine, nothing hapens, because the kernel does not call the poll() routine often enough. I've stated this several times.

--linas

OK, Arnd gave me a clue stick. I need to call the (misnamed) netif_rx_schedule() from the tx interrupt in order to get this to work. That makes sense, and its easy, I'll send the revised patch.. well, not tonight, but shortly.

--linas

You might not want to call it all the time though... You need some interrupt mitigation and thus a timer that calls netif_rx_schedule() might be of some use still...

Ben.

Well, again, the whole point of a low-watermark interrupt is to get zero of them when the system is working correctly; they're self-mitigating by design.

-------------
Anyway, I tried the suggestion, but am getting less-than-ideal results.

To recap: my original patch did this:

   spider_interrupt_handler(struct whatever *) {
      ...
      if (tx_interrupt) 
         schedule_work (tx_cleanup_handler)
   }

which David Miller objected to. Once I understood the why (sorry for not getting it right away), I then replaced the above with the below, which is what I think everyone wanted:

   spider_interrupt_handler(struct whatever *) {
      ...
      if (tx_interrupt) 
         netif_rx_schedule(netdev);
   }

   spidernet_poll(stuct whatever *) {
      tx_cleanup_handler(txring);
      // rx_stuff too ...
   }

I was expecting this to be a no-op from the performance point of view. Instead, I get a fairly dramatic (11%) slowdown:
the first patch runs in the 785-805 Mbits/sec range, while the second patch runs in the 705-715 Mbits/sec range. I am surprised, ad don't understand why this would be so.

For the record, the alternate patch is below.

----

Index: linux-2.6.18-rc2/drivers/net/spider_net.c
===================================================================
@@ -1087,6 +1090,8 @@ spider_net_poll(struct net_device *netde
 	int packets_to_do, packets_done = 0;
 	int no_more_packets = 0;
 
+	spider_net_cleanup_tx_ring(card);
+
 	packets_to_do = min(*budget, netdev->quota);
 
 	while (packets_to_do) {

@@ -1495,16 +1500,16 @@ spider_net_interrupt(int irq, void *ptr,
 	if (!status_reg)
 		return IRQ_NONE;
 
-	if (status_reg & SPIDER_NET_RXINT ) {
+	if (status_reg & SPIDER_NET_RXINT) {
 		spider_net_rx_irq_off(card);
 		netif_rx_schedule(netdev);
 	}
-	if (status_reg & SPIDER_NET_TXINT ) {
-		spider_net_cleanup_tx_ring(card);
-		netif_wake_queue(netdev);
-	}
 
-	if (status_reg & SPIDER_NET_ERRINT )
+	/* Call rx_schedule from the tx interrupt, so that NAPI poll runs. */
+	if (status_reg & SPIDER_NET_TXINT)
+		netif_rx_schedule(netdev);
+
+	if (status_reg & SPIDER_NET_ERRINT)
 		spider_net_handle_error_irq(card, status_reg);
 
 	/* clear interrupt sources */

From: [email protected] (Linas Vepstas)
Date: Mon, 21 Aug 2006 19:13:11 -0500

	if ((status_reg & (SPIDET_NET_RXINT | SPIDET_NET_TXINT))) {
		spider_net_rx_and_tx_irq_off(card);
		netif_rx_schedule();
	}

Maybe be because you actually typed "disabled" in your previous message ? :)

Well, we do need to harvest descriptors of course, though I suppose that can be done in hard_xmit as well. I'm not sure if there is any real benefit in batching those.

That is the most important point imho for the specific case of spidernet on cell.

Ben.

Turn off mis-interpretation of the queue-empty interrupt status bit as an error. This bit is set as a part of the previous low-watermark patch.

----

 drivers/net/spider_net.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Index: linux-2.6.18-rc3-mm2/drivers/net/spider_net.c
===================================================================
@@ -1275,12 +1275,15 @@ spider_net_handle_error_irq(struct spide
 	case SPIDER_NET_PHYINT:
 	case SPIDER_NET_GMAC2INT:
 	case SPIDER_NET_GMAC1INT:
-	case SPIDER_NET_GIPSINT:
 	case SPIDER_NET_GFIFOINT:
 	case SPIDER_NET_DMACINT:
 	case SPIDER_NET_GSYSINT:
 		break; */
 
+	case SPIDER_NET_GIPSINT:
+		show_error = 0;
+		break;
+
 	case SPIDER_NET_GPWOPCMPINT:
 		/* PHY write operation completed */
 		show_error = 0;

@@ -1339,9 +1342,10 @@ spider_net_handle_error_irq(struct spide
 	case SPIDER_NET_GDTDCEINT:
 		/* chain end. If a descriptor should be sent, kick off
 		 * tx dma
-		if (card->tx_chain.tail == card->tx_chain.head)
+		if (card->tx_chain.tail != card->tx_chain.head)
 			spider_net_kick_tx_dma(card);
-		show_error = 0; */
+		*/
+		show_error = 0;
 		break;
 
 	/* case SPIDER_NET_G1TMCNTINT: not used. print a message */

@@ -1455,8 +1459,9 @@ spider_net_handle_error_irq(struct spide
 	}
 
 	if ((show_error) && (netif_msg_intr(card)))
-		pr_err("Got error interrupt, GHIINT0STS = 0x%08x, "
+		pr_err("Got error interrupt on %s, GHIINT0STS = 0x%08x, "
 		       "GHIINT1STS = 0x%08x, GHIINT2STS = 0x%08x\n",
+		       card->netdev->name,
 		       status_reg, error_reg1, error_reg2);
 
 	/* clear interrupt sources */

This patch adds version information as reported by ethtool -i to the Spidernet driver.

----

 drivers/net/spider_net.c         |    3 +++
 drivers/net/spider_net.h         |    2 ++
 drivers/net/spider_net_ethtool.c |    2 +-
 3 files changed, 6 insertions(+), 1 deletion(-)

Index: linux-2.6.18-rc3-mm2/drivers/net/spider_net.c
===================================================================
@@ -55,6 +55,7 @@ MODULE_AUTHOR("Utz Bacher <utz.bacher@de
 	      "<[email protected]>");
 MODULE_DESCRIPTION("Spider Southbridge Gigabit Ethernet driver");
 MODULE_LICENSE("GPL");
+MODULE_VERSION(VERSION);
 
 static int rx_descriptors = SPIDER_NET_RX_DESCRIPTORS_DEFAULT;
 static int tx_descriptors = SPIDER_NET_TX_DESCRIPTORS_DEFAULT;

@@ -2293,6 +2294,8 @@ static struct pci_driver spider_net_driv
  */
 static int __init spider_net_init(void)
 {
+	printk("spidernet Version %s.\n",VERSION);
+
 	if (rx_descriptors < SPIDER_NET_RX_DESCRIPTORS_MIN) {
 		rx_descriptors = SPIDER_NET_RX_DESCRIPTORS_MIN;
 		pr_info("adjusting rx descriptors to %i.\n", rx_descriptors);

Index: linux-2.6.18-rc3-mm2/drivers/net/spider_net.h
===================================================================
@@ -24,6 +24,8 @@
 #ifndef _SPIDER_NET_H
 #define _SPIDER_NET_H
 
+#define VERSION "1.1 A"
+
 #include "sungem_phy.h"
 
 extern int spider_net_stop(struct net_device *netdev);

Index: linux-2.6.18-rc3-mm2/drivers/net/spider_net_ethtool.c
===================================================================
@@ -55,7 +55,7 @@ spider_net_ethtool_get_drvinfo(struct ne
 	/* clear and fill out info */
 	memset(drvinfo, 0, sizeof(struct ethtool_drvinfo));
 	strncpy(drvinfo->driver, spider_net_driver_name, 32);
-	strncpy(drvinfo->version, "0.1", 32);
+	strncpy(drvinfo->version, VERSION, 32);
 	strcpy(drvinfo->fw_version, "no information");
 	strncpy(drvinfo->bus_info, pci_name(card->pdev), 32);
 }

Why does a driver that's in the mainline kernel need to have a version number besides the kernel version?

I can understand it for drivers like e1000 that Intel maintain outside of the kernel as well. But spidernet is a fully mainline maintained driver, right?

-Olof

Hi Olof,

There are several reasons why an Ethernet driver should have an up to
date version number:

1. Customers like to see they are really getting a new version.

2. It makes it easier for support personnel (me in this case) to see which driver they have. Sure, sometimes I can talk them thru doing a "sum" on the .ko and all that, but why not just use the version number? That's what it is for. And no, you can't just assume they have the version that came with the kernel they are running. It doesn't work that way.

3. It makes bug reporting easier.

4. I have already run into too many problems and wasted too much time working with drivers when the number was NOT getting updated.

Jim Lewis
Advisory Software Engineer
IBM Linux Technology Center
512-838-7754

Olof Johansson <[email protected]>
08/11/2006 01:00 PM

To
Linas Vepstas <[email protected]>
cc
[email protected], [email protected], [email protected], Jens Osterkamp <[email protected]>, James K Lewis/Austin/IBM@ibmus, Arnd Bergmann <[email protected]> Subject
Re: [PATCH 4/4]: powerpc/cell spidernet ethtool -i version number info.

Why does a driver that's in the mainline kernel need to have a version number besides the kernel version?

I can understand it for drivers like e1000 that Intel maintain outside of the kernel as well. But spidernet is a fully mainline maintained driver, right?

-Olof

_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Hi Olof,

I'll let Jim be the primary defender. From what I can tell, "that's the way its done". For example:

linux-2.6.18-rc3-mm2 $ grep MODULE_VERSION */*/*.c |wc

     164     245    9081

Yes, the spidernet is a Linux-kernel only driver.

--linas

p.s. very strange, but I did not see your original email; only saw Jim's reply.

Thanks for the info, Jim.

Sounds like it's most useful if a customer (or distro) takes the driver out of the tree and run it with a different kernel, i.e. when kernel and driver versions no longer go together. Makes sense.

-Olof

It only makes sense in addition to the kernel version number (which in some cases can be meaningless), plus any distro and/or local patches, plus the kernel config.

Without all that information you don't really know what you're talking about, because any one of the many interfaces between the driver and the core kernel may have changed.

So in practice I find it's much simpler to just get the exact source that they're running, rather than trying to guess based on version numbers. But that's just me :)

cheers

Michael Ellerman
IBM OzLabs

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

With these patches the spidernet driver performance goes from bad to usable. They are all good changes. I'd expect some more bottlnecks to be identified now that these are cleared. Maybe Jim and Linas can get the driver performance up from usable to good next.

Hi Linas.
Just noticed a nit-pick detail.
The general rule is to add your Signed-off-by: at the bottom of the patch, so the top-most Signed-of-by: is also the original author whereas the last Signed-of-by: is the one that added this patch to the kernel. Likewise you add Cc: before your Signed-off-by: line.

See patches from for example Andrew Morton for examples.

Sam

I put my name at the top when I was the primary author. I put Jim's name at the top when he was the primary author.

Both names are there because I sat in Jim's office and used his keyboard. I got him to compile and run the tests on his hardware, and we'd then debate the results.

The patches I ave from akpm have the CC's after the signed-off by line, not before.

--linas

When the patch gets added to a git repository, they end up having your name on it, because the author is determined from the person who sent the patch.

For the patches where Jim is the main author, you should put a

From: James K Lewis <[email protected]>

into the first line of the email body. That will make the scripts do the right thing. The order of the Signed-off-by:
lines is used is independant from authorship and should list the name of the submitter last.

	Arnd <><

Please apply and forward upstream. This patch requires the previous sequence of 4 spidernet patches to be applied.

--linas

The recent set of low-waterark patches for the spider result in a significant amount of computing being done in an interrupt context. This patch moves this to a "bottom half" aka work queue, so that the code runs in a normal kernel context. Curiously, this seems to result in a performance boost of about 5% for large packets.

----

 drivers/net/spider_net.c |   21 +++++++++++++++------
 drivers/net/spider_net.h |    4 +++-
 2 files changed, 18 insertions(+), 7 deletions(-)

Index: linux-2.6.18-rc3-mm2/drivers/net/spider_net.c
===================================================================
@@ -883,9 +883,10 @@ out:
  * spider_net_cleanup_tx_ring - cleans up the TX ring
  * @card: card structure
  *
- * spider_net_cleanup_tx_ring is called by the tx_timer (as we don't use
- * interrupts to cleanup our TX ring) and returns sent packets to the stack
- * by freeing them
+ * spider_net_cleanup_tx_ring is called by either the tx_timer
+ * or from a work-queue scheduled by the tx-empty interrupt.
+ * This routine releases resources associted with transmitted
+ * packets, including updating the queue tail pointer.
  */
 static void
 spider_net_cleanup_tx_ring(struct spider_net_card *card)

@@ -895,12 +896,20 @@ spider_net_cleanup_tx_ring(struct spider
 	spin_lock_irqsave(&card->tx_chain.lock, flags);
 
 	if ((spider_net_release_tx_chain(card, 0) != 0) &&
-	    (card->netdev->flags & IFF_UP))
+	    (card->netdev->flags & IFF_UP)) {
 		spider_net_kick_tx_dma(card);
+		netif_wake_queue(card->netdev);
+	}
 
 	spin_unlock_irqrestore(&card->tx_chain.lock, flags);
 }
 
+static void
+spider_net_tx_cleanup_task(void * data)
+{
+	spider_net_cleanup_tx_ring((struct spider_net_card *) data);
+}
+
 /**
  * spider_net_do_ioctl - called for device ioctls
  * @netdev: interface device structure

@@ -1499,8 +1508,7 @@ spider_net_interrupt(int irq, void *ptr,
 		netif_rx_schedule(netdev);
 	}
 	if (status_reg & SPIDER_NET_TXINT ) {
-		spider_net_cleanup_tx_ring(card);
-		netif_wake_queue(netdev);
+		schedule_work(&card->tx_cleanup_task);
 	}
 
 	if (status_reg & SPIDER_NET_ERRINT )

@@ -2117,6 +2125,7 @@ spider_net_alloc_card(void)
 	card->netdev = netdev;
 	card->msg_enable = SPIDER_NET_DEFAULT_MSG;
 	INIT_WORK(&card->tx_timeout_task, spider_net_tx_timeout_task, netdev);
+	INIT_WORK(&card->tx_cleanup_task, spider_net_tx_cleanup_task, card);
 	init_waitqueue_head(&card->waitq);
 	atomic_set(&card->tx_timeout_task_counter, 0);
 

Index: linux-2.6.18-rc3-mm2/drivers/net/spider_net.h
===================================================================
@@ -24,7 +24,7 @@
 #ifndef _SPIDER_NET_H
 #define _SPIDER_NET_H
 
-#define VERSION "1.1 A"
+#define VERSION "1.1 B"
 
 #include "sungem_phy.h"
 

@@ -441,6 +441,8 @@ struct spider_net_card {
 	atomic_t tx_timeout_task_counter;
 	wait_queue_head_t waitq;
 
+	struct work_struct tx_cleanup_task;
+
 	/* for ethtool */
 	int msg_enable;
 

Let's not reinvented NAPI, shall we...

Jeff

??

I was under the impression that NAPI was for the receive side only. This round of patches were for the transmit queue.

Let me describe the technical problem; perhaps there's some other solution for it?

The default socket size seems to be 128KB; (cat /proc/sys/net/core/wmem_default) if a user application writes more than 128 KB to a socket, the app is blocked by the kernel till there's room in the socket for more. At gigabit speeds, a network card can drain 128KB in about a millisecond, or about four times a jiffy (assuming HZ=250). If the network card isn't generaing interrupts, (and there are no other interrupts flying around) then the tcp stack only wakes up once a jiffy, and so the user app is scheduled only once a jiffy. Thus, the max bandwidth that the app can see is (HZ * wmem_default) bytes per second, or about 250 Mbits/sec for my system. Disappointing for a gigabit adapter.

There's three ways out of this:

(1) tell the sysadmin to
"echo 1234567 > /proc/sys/net/core/wmem_default" which violates all the rules.

(2) Poll more frequently than once-a-jiffy. Arnd Bergmann and I got this working, using hrtimers. It worked pretty well, but seemed like a hack to me.

(3) Generate transmit queue low-watermark interrupts, which is an admitedly olde-fashioned but common engineering practice. This round of patches implement this.

--linas

That depends on the driver implementation.

Jeff

From: Jeff Garzik <[email protected]>
Date: Wed, 16 Aug 2006 16:34:31 -0400

What Jeff is trying to say is that TX reclaim can occur in the NAPI poll routine, and in fact this is what the vast majority of NAPI drivers do.

It also makes the locking simpler.

In practice, the best thing seems to be to put both RX and TX work into ->poll() and have a very mild hw interrupt mitigation setting programmed into the chip.

I'm not familiar with the spidernet TX side interrupt capabilities so I can't say whether that is something that can be directly implied. In fact, I get the impression that spidernet is limited in some way and that's where all the strange approaches are coming from :)

Am Wednesday 16 August 2006 22:46 schrieb David Miller:

Actually, the capabilities of the chip are quite powerful, it only seems to be hard to make it go fast using any of them. That may be the fault of strange locking rules and other bugs we had in the driver before, so maybe you can recommend which one to use.

Cleaning up the TX queue only from ->poll() like all the others sounds like the right approach to simplify the code.

The spider hardware offers at least these options:

- end of TX queue interrupt
- set a per-descriptor bit to fire an interrupt at a specific frame
- an interrupt for each frame (may be multiple descriptors)
- an interrupt for each descriptor
- timers implemented in the spidernet hardware

We first had an interrupt per descriptor, then got rid of all TX interrupts and replaced them by timers to reduce the interrupt load, but reducing throughput in the case where user space sleeps on a full socket buffer.

The last patches that were suggested introduce marking a single descriptor (not the last one, but somewhere near the end of the queue) so we fire an interrupt just before the TX queue gets empty.

	Arnd <><

From: Arnd Bergmann <[email protected]>
Date: Wed, 16 Aug 2006 23:24:46 +0200

The best schemes seem to be to interrupt mitigate using a combination of time and number of TX entries pending to be purged. This is what most gigabit chips seem to offer.

On Tigon3, for example, we tell the chip to interrupt if either 53 frames or 150usecs have passed since the first TX packet has become available for reclaim.

That bounds the latency as well as force the interrupt if a lot of TX work becomes available.

Can spidernet be told these kinds of parameters? "N packets or X usecs"?

This is all controllable via ethtool btw (via ETHTOOL_{S,G}COALESCE), so you can experiment if you want.

Am Wednesday 16 August 2006 23:32 schrieb David Miller:

It can not do exactly this but probably we can get close to it by

1.) Setting a one-time interrupt to fire x*10=B5s after putting a
packet into the TX queue.

and

2.a) Enabling end of TX queue interrupts whenever more than y frames
have been queued for TX.

or

2.b) Marking frame number y in the TX to fire an interrupt when it
has been transmitted, and move the mark whenever we clean up TX descriptors.

or

2.c) Marking frame number y to generate an interrupt, but counting
y from the end of the queue, and update the mark whenever we add frames to the queue tail.

I'm not sure which version of 2. would give us the best approximation.

	Arnd <><

From: Arnd Bergmann <[email protected]>
Date: Thu, 17 Aug 2006 00:16:46 +0200

Oh, you can only control TX packet counts using bits in the TX ring entries :(

Tigon3 can even be told to use different interrupt mitigation parameters when the cpu is actively servicing an interrupt for the chip.

Didn't you say spidernet's facilities were sophisticated? :) This Tigon3 stuff is like 5+ year old technology.

Am Thursday 17 August 2006 00:29 schrieb David Miller:

I was rather overwhelmed by the 34 different interrupts that the chip can create, that does not mean they chose the right events for generating them.
Interestingly, spidernet has five different counters you can set up to generate interrupts after a number of received frames, but none for transmit...

	Arnd <><

Why would you want o do this? It seems like a cruddier strategy than what we can already do (which is to never get an transmit interrupt, as long as the kernel can shove data into the device fast enough to keep the queue from going empty.) The whole *point* of a low-watermark interrupt is to never have to actually get the interrupt, if the rest of the system is on its toes and is supplying data fast enough.

--linas

From: [email protected] (Linas Vepstas)
Date: Wed, 16 Aug 2006 18:30:28 -0500

As long as TX packets get freed within a certain latency boundary, this kind of scheme should be fine.

I just had some fun making sure I wasn't making a liar out of myself. So far, I'm good.

Did

echo 768111 > /proc/sys/net/core/wmem_max
echo 768111 > /proc/sys/net/core/wmem_default

to make sure that the app never blocked on a full socket.

(If the socket is small, then the app blocks, the transmit queue drains, and we do get an interupt -- about 1K/sec, which is what is expected).

Ran 'vmstat 10' to watch the interrupts in real time. Ran netperf, got 904 Mbits/sec, and *no* interrupts. Yahoo!

Ran oprofile to see where he time went:

CPU: ppc64 Cell Broadband Engine, speed 3200 MHz (estimated)
Counted CYCLES events (Processor Cycles) with a unit mask of 0x00 (No unit mask) count 100000

samples  %        image name               app name                 symbol name
13748742 77.6620  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .cbe_idle
936172    5.2881  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .__copy_tofrom_user
569353    3.2161  spidernet.ko             spidernet                .spider_net_xmit
450826    2.5466  spidernet.ko             spidernet                .spider_net_release_tx_chain
220374    1.2448  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       ._spin_unlock_irqrestore
112432    0.6351  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       ._spin_lock
91328     0.5159  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .__qdisc_run
84804     0.4790  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .packet_sendmsg
76167     0.4302  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .kfree
74321     0.4198  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .sock_alloc_send_skb
65323     0.3690  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .kmem_cache_free
60334     0.3408  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       ._read_lock
60071     0.3393  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .dev_queue_xmit
56900     0.3214  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .kmem_cache_alloc_node
55281     0.3123  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .sock_wfree
51242     0.2894  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .dev_get_by_index
50438     0.2849  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .compat_sys_socketcall
49247     0.2782  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .fput
48589     0.2745  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .sync_buffer
46055     0.2601  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       system_call_common
40607     0.2294  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .local_bh_enable
40273     0.2275  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .__might_sleep
38757     0.2189  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .fget_light
38219     0.2159  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .__kfree_skb
36804     0.2079  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .sock_def_write_space
36443     0.2059  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .skb_release_data
32174     0.1817  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .sys_sendto
31828     0.1798  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .sock_sendmsg
30676     0.1733  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .pfifo_fast_dequeue
29607     0.1672  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .add_event_entry
25870     0.1461  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       syscall_exit
25329     0.1431  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .__alloc_skb
23885     0.1349  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .__do_softirq
22046     0.1245  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .kmem_find_general_cachep
21610     0.1221  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .__dev_get_by_index
21059     0.1190  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .dma_map_single
21044     0.1189  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       ._spin_lock_irqsave
20105     0.1136  vmlinux-2.6.18-rc2       vmlinux-2.6.18-rc2       .memset

.__copy_tofrom_user -- ouch spider does not currently do scatter-gather
.spider_net_xmit -- hmmm ?? why is it this large ??
.spider_net_release_tx_chain -- ?? a lot of time being spent cleaning up tx queues.
._spin_unlock_irqrestore -- hmm ? why so high? lock contention?

I presume the rest is normal.

I seem to be having a multi-hour delay for email delivery, so maybe we've crossed emails.

A "low watermark interrupt" is an interrupt that is generated when some queue is "almost empty". This last set of patches implement this for the TX queue. The interrupt pops when 3/4ths of the packets in the queue have been processed. Playing with ths setting (3/4ths or some other number) seemed to make little difference.

The nature of a low-watermark interrupt is that it NEVER pops, as long as the kernel keeps putting more stuff into the queue, so as to keep the queue at least 1/4'th full. I don't know how to mitigate interrupts more than that.

--linas

Last round of measurements seemed linear for packet sizes between 60 and 600 bytes, suggesting that the hardware can handle a maximum of 120K descriptors/second, independent of packet size. I don't know why this is.

My fault; a few months ago, we were in panic mode trying to get the thing functioning reliably, and I put locks around anything and everything. This last patch removes those locks, and protects only a few pointers (the incrementing of the head and the tail pointers, and the location ofthe low watermark) that actually needed protection. They need protection because the code can get called in various different ways.

I'll try this ...

Its not a big a driver. 'wc' says its 2.3 loc, which is 1/3 or 1/5 the size of tg3.c or the e1000*c files.

--linas

Am Thursday 17 August 2006 00:55 schrieb Linas Vepstas:

Could well be related to latencies when going to the remote node for descriptor DMAs. Have you tried if the hch's NUMA patch or using numactl makes a difference here?

Right, I was thinking of removing a lock or another, not throwing out half of the driver ;-)

	Arnd <><

No. I guess I should try.

There's only four lock points grand total.
-- One on the receive side,
-- one to protect the transmit head pointer,
-- one to protect the transmit tail pointer,
-- one to protect the location of the transmit low watermark.

The last three share the same lock. I tried using distinct locks, but this worsened things, probably due to cache-line trashing. I tried removing the head pointer lock, but this failed. I don't know why, and was surprised by this. I thought hard_start_xmit() was serialized.

--linas

DMA overhead perhaps? If it takes so many micro/nanoseconds to get a DMA going.... That used to be a reason the Tigon2 had such low PPS rates and issues with multiple buffer packets and a 1500 byte MTU - it had rather high DMA setup latency, and then if you put it into a system with highish DMA read/write latency... well that didn't make it any better :)

rick jones

I'll experiment with this. When doing, say, an ftp, there are enough TCP ack packets coming back to have NAPI netdev->poll be called frequently enough?

Hmm. Or maybe I'm just getting old. Once upon a time, low watermarks were considered the "best" way of doing anything; never occurred to me it would be considered "strange". Based on my probably obsolete idea of what constitutes "slick hardware", I was actually impressed by what the spidernet could do.

Aside from cleaning up the transmit ring in the receive poll loop, what would be the not-so-strange way of doing things?

--linas

Please apply and forward upstream. This patch requires the pervious set of four spidernet patches to be applied.

--linas

The transmit side of the spider ethernet driver currently places locks around some very large chunks of code. This results in a fair amount of lock contention is some cases. This patch makes the locks much more fine-grained, protecting only the cirtical sections (the queue head and tail pointers, and the queue low-watermark location).

This, with the previous patches, result in the following performance, using netperf, averaged over 5 minute runs:

pkt size    rate
========    ====
1500        804 Mbits/sec
 800        701 Mbits/sec
 600        600 Mbits/sec
 300        280 Mbits/sec

60 60 Mbits/sec

----

 drivers/net/spider_net.c |   77 ++++++++++++++++++++---------------------------
 drivers/net/spider_net.h |    2 -
 2 files changed, 35 insertions(+), 44 deletions(-)

Index: linux-2.6.18-rc3-mm2/drivers/net/spider_net.c
===================================================================
@@ -644,8 +644,9 @@ static int
 spider_net_prepare_tx_descr(struct spider_net_card *card,
 			    struct sk_buff *skb)
 {
-	struct spider_net_descr *descr = card->tx_chain.head;
+	struct spider_net_descr *descr;
 	dma_addr_t buf;
+	unsigned long flags;
 
 	buf = pci_map_single(card->pdev, skb->data, skb->len, PCI_DMA_TODEVICE);
 	if (buf == DMA_ERROR_CODE) {

@@ -655,6 +656,10 @@ spider_net_prepare_tx_descr(struct spide
 		return -ENOMEM;
 	}
 
+	spin_lock_irqsave(&card->tx_chain.lock, flags);
+	descr = card->tx_chain.head;
+	card->tx_chain.head = card->tx_chain.head->next;
+
 	descr->buf_addr = buf;
 	descr->buf_size = skb->len;
 	descr->next_descr_addr = 0;

@@ -663,6 +668,8 @@ spider_net_prepare_tx_descr(struct spide
 
 	descr->dmac_cmd_status =
 			SPIDER_NET_DESCR_CARDOWNED | SPIDER_NET_DMAC_NOCS;
+	spin_unlock_irqrestore(&card->tx_chain.lock, flags);
+
 	if (skb->protocol == htons(ETH_P_IP))
 		switch (skb->nh.iph->protocol) {
 		case IPPROTO_TCP:

@@ -673,37 +680,16 @@ spider_net_prepare_tx_descr(struct spide
 			break;
 		}
 
+	/* Chain the bus address, so that the DMA engine finds this descr. */
 	descr->prev->next_descr_addr = descr->bus_addr;
 
 	return 0;
 }
 
-/**
- * spider_net_release_tx_descr - processes a used tx descriptor
- * @card: card structure
- * @descr: descriptor to release
- *
- * releases a used tx descriptor (unmapping, freeing of skb)
- */
-static inline void
-spider_net_release_tx_descr(struct spider_net_card *card)
-{
-	struct spider_net_descr *descr = card->tx_chain.tail;
-	struct sk_buff *skb;
-
-	card->tx_chain.tail = card->tx_chain.tail->next;
-	descr->dmac_cmd_status |= SPIDER_NET_DESCR_NOT_IN_USE;
-
-	/* unmap the skb */
-	skb = descr->skb;
-	pci_unmap_single(card->pdev, descr->buf_addr, skb->len,
-			PCI_DMA_TODEVICE);
-	dev_kfree_skb_any(skb);
-}
-
 static void
 spider_net_set_low_watermark(struct spider_net_card *card)
 {
+	unsigned long flags;
 	int status;
 	int cnt=0;
 	int i;

@@ -727,11 +713,13 @@ spider_net_set_low_watermark(struct spid
 		descr = descr->next;
 
 	/* Set the new watermark, clear the old wtermark */
+	spin_lock_irqsave(&card->tx_chain.lock, flags);
 	descr->dmac_cmd_status |= SPIDER_NET_DESCR_TXDESFLG;
 	if (card->low_watermark && card->low_watermark != descr)
 		card->low_watermark->dmac_cmd_status =
 		     card->low_watermark->dmac_cmd_status & ~SPIDER_NET_DESCR_TXDESFLG;
 	card->low_watermark = descr;
+	spin_unlock_irqrestore(&card->tx_chain.lock, flags);
 }
 
 /**

@@ -750,22 +738,30 @@ static int
 spider_net_release_tx_chain(struct spider_net_card *card, int brutal)
 {
 	struct spider_net_descr_chain *chain = &card->tx_chain;
+	struct spider_net_descr *descr;
+	struct sk_buff *skb;
+	u32 buf_addr;
+	unsigned long flags;
 	int status;
 	int rc=0;
 
 	spider_net_read_reg(card, SPIDER_NET_GDTDMACCNTR);
 
 	while (chain->tail != chain->head) {
-		status = spider_net_get_descr_status(chain->tail);
+		spin_lock_irqsave(&chain->lock, flags);
+		descr = chain->tail;
+
+		status = spider_net_get_descr_status(descr);
 		switch (status) {
 		case SPIDER_NET_DESCR_COMPLETE:
 			card->netdev_stats.tx_packets++;
-			card->netdev_stats.tx_bytes += chain->tail->skb->len;
+			card->netdev_stats.tx_bytes += descr->skb->len;
 			break;
 
 		case SPIDER_NET_DESCR_CARDOWNED:
 			if (!brutal) {
 				rc = 1;
+				spin_unlock_irqrestore(&chain->lock, flags);
 				goto done;
 			}
 			/* fallthrough, if we release the descriptors

@@ -785,9 +781,19 @@ spider_net_release_tx_chain(struct spide
 		default:
 			card->netdev_stats.tx_dropped++;
 			rc = 1;
+			spin_unlock_irqrestore(&chain->lock, flags);
 			goto done;
 		}
-		spider_net_release_tx_descr(card);
+
+		chain->tail = chain->tail->next;
+		descr->dmac_cmd_status |= SPIDER_NET_DESCR_NOT_IN_USE;
+		skb = descr->skb;
+		buf_addr = descr->buf_addr;
+		spin_unlock_irqrestore(&chain->lock, flags);
+
+		/* unmap the skb */
+		pci_unmap_single(card->pdev, buf_addr, skb->len, PCI_DMA_TODEVICE);
+		dev_kfree_skb_any(skb);
 	}
 done:
 	if (rc == 1)

@@ -844,11 +850,8 @@ spider_net_xmit(struct sk_buff *skb, str
 	struct spider_net_card *card = netdev_priv(netdev);
 	struct spider_net_descr_chain *chain = &card->tx_chain;
 	struct spider_net_descr *descr = chain->head;
-	unsigned long flags;
 	int result;

:
- spin_lock_irqsave(&chain->lock, flags);
-
spider_net_release_tx_chain(card, 0);

if (chain->head->next == chain->tail->prev) {
@@ -869,12 +872,9 @@ spider_net_xmit(struct sk_buff *skb, str
}

result = NETDEV_TX_OK;
-
spider_net_kick_tx_dma(card);
- card->tx_chain.head = card->tx_chain.head->next;

out:
- spin_unlock_irqrestore(&chain->lock, flags);
netif_wake_queue(netdev);
return result;
}
@@ -891,17 +891,11 @@ out:
static void
spider_net_cleanup_tx_ring(struct spider_net_card *card) {
- unsigned long flags;
-
- spin_lock_irqsave(&card->tx_chain.lock, flags);
-
if ((spider_net_release_tx_chain(card, 0) != 0) &&
(card->netdev->flags & IFF_UP)) {
spider_net_kick_tx_dma(card); netif_wake_queue(card->netdev);
}
-
- spin_unlock_irqrestore(&card->tx_chain.lock, flags);
}

static void
@@ -1932,10 +1926,7 @@ spider_net_stop(struct net_device *netde
spider_net_disable_rxdmac(card);

/* release chains */
- if (spin_trylock(&card->tx_chain.lock)) {
- spider_net_release_tx_chain(card, 1);
- spin_unlock(&card->tx_chain.lock);
- }
+ spider_net_release_tx_chain(card, 1);

spider_net_free_chain(card, &card->tx_chain); spider_net_free_chain(card, &card->rx_chain);

Index: linux-2.6.18-rc3-mm2/drivers/net/spider_net.h
===================================================================
@@ -24,7 +24,7 @@
 #ifndef _SPIDER_NET_H
 #define _SPIDER_NET_H
 
-#define VERSION "1.1 B"
+#define VERSION "1.1 C"
 
 #include "sungem_phy.h"