Please apply and forward upstream as appropriate.
Although these patches have not been baking in
any -mm tree, they have been tested and are
generally available as a part of the Cell SDK 2.0
overseen by Arnd Bergmann. (Arnd, if you want
to lend a voice of authority here, or to correct
me, please do so...)
The following sequence of six patches implement a
series of changes to the transmit side of the
spidernet ethernet device driver, significantly
improving performance for large packets.
This series of patches is almost identical to
those previously mailed on 18-20 August, with one
critical change: NAPI polling is used instead of
homegrown polling.
Although these patches improve things, I am not
satisfied with how this driver behaves, and so
plan to do additional work next week.
Signed-off-by: Linas Vepstas <[email protected]>
Signed-off-by: James K Lewis <[email protected]>
Cc: Arnd Bergmann <[email protected]>
--linas
This patch increases the Burst Address alignment from 64 to 1024 in the
Spidernet driver. This improves transmit performance for large packets.
From: James K Lewis <[email protected]>
Signed-off-by: James K Lewis <[email protected]>
Signed-off-by: Linas Vepstas <[email protected]>
Cc: Arnd Bergmann <[email protected]>
----
drivers/net/spider_net.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-2.6.18-mm2/drivers/net/spider_net.h
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net.h 2006-09-29 14:11:21.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net.h 2006-09-29 16:33:49.000000000 -0500
@@ -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.
Hardware low-watermarks allow a properly configured kernel
to continously stream data to a device and not have to handle
any interrupts at all in doing so. Correct zero-interrupt
operation can be actually observed for this driver, when the
socket buffer is made large enough.
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 the 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.
If the queue drains down to the low waterark, then an interrupt
will be generated. However, if the kernel/driver continues
to add enough packets to keep the queue partially filled,
no interrupt will actually be generated, and the hardware
can continue streaming packets indefinitely in this mode.
The impelmentation is done 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 the flag
at about 1/4 from "empty".
Signed-off-by: Linas Vepstas <[email protected]>
Signed-off-by: James K Lewis <[email protected]>
Cc: Arnd Bergmann <[email protected]>
----
drivers/net/spider_net.c | 56 ++++++++++++++++++++++++++++++++++++++++++-----
drivers/net/spider_net.h | 6 +++--
2 files changed, 55 insertions(+), 7 deletions(-)
Index: linux-2.6.18-mm2/drivers/net/spider_net.c
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net.c 2006-09-29 14:11:20.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net.c 2006-09-29 16:33:46.000000000 -0500
@@ -703,6 +703,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
@@ -720,6 +753,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);
@@ -732,8 +766,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) */
@@ -750,12 +786,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;
}
/**
@@ -1460,6 +1499,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);
@@ -1621,6 +1664,9 @@ spider_net_open(struct net_device *netde
if (spider_net_init_chain(card, &card->tx_chain, card->descr,
PCI_DMA_TODEVICE, card->tx_desc))
goto alloc_tx_failed;
+
+ card->low_watermark = NULL;
+
if (spider_net_init_chain(card, &card->rx_chain,
card->descr + card->rx_desc,
PCI_DMA_FROMDEVICE, card->rx_desc))
Index: linux-2.6.18-mm2/drivers/net/spider_net.h
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net.h 2006-09-29 14:47:50.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net.h 2006-09-29 16:33:43.000000000 -0500
@@ -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 */
@@ -433,6 +434,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;
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.
Signed-off-by: Linas Vepstas <[email protected]>
Signed-off-by: James K Lewis <[email protected]>
Cc: Arnd Bergmann <[email protected]>
----
drivers/net/spider_net.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
Index: linux-2.6.18-mm2/drivers/net/spider_net.c
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net.c 2006-09-29 15:01:55.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net.c 2006-09-29 16:33:43.000000000 -0500
@@ -1282,12 +1282,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;
@@ -1346,9 +1349,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 */
@@ -1462,8 +1466,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.
From: James K Lewis <[email protected]>
Signed-off-by: James K Lewis <[email protected]>
Signed-off-by: Linas Vepstas <[email protected]>
Cc: Arnd Bergmann <[email protected]>
----
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-mm2/drivers/net/spider_net.c
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net.c 2006-09-29 15:05:05.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net.c 2006-09-29 16:33:39.000000000 -0500
@@ -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;
@@ -2303,6 +2304,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-mm2/drivers/net/spider_net.h
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net.h 2006-09-29 15:01:55.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net.h 2006-09-29 15:18:12.000000000 -0500
@@ -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-mm2/drivers/net/spider_net_ethtool.c
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net_ethtool.c 2006-09-29 14:11:18.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net_ethtool.c 2006-09-29 15:18:12.000000000 -0500
@@ -76,7 +76,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);
}
Jim, as the official maintainer, you should explicitly ack this patch.
--linas
This patch moves transmit queue cleanup code out of the
interrupt context, and into the NAPI polling routine.
Signed-off-by: Linas Vepstas <[email protected]>
Cc: James K Lewis <[email protected]>
Cc: Arnd Bergmann <[email protected]>
----
drivers/net/spider_net.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
Index: linux-2.6.18-mm2/drivers/net/spider_net.c
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net.c 2006-09-29 17:01:39.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net.c 2006-09-29 17:39:06.000000000 -0500
@@ -887,9 +887,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 the NAPI polling routine.
+ * 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)
@@ -1093,6 +1094,7 @@ 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) {
@@ -1505,10 +1507,8 @@ 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_TXINT)
+ netif_rx_schedule(netdev);
if (status_reg & SPIDER_NET_ERRINT )
spider_net_handle_error_irq(card, status_reg);
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. One lock is used to protect
three locations: the queue head and tail pointers, and the
queue low-watermark location.
Signed-off-by: Linas Vepstas <[email protected]>
Cc: James K Lewis <[email protected]>
Cc: Arnd Bergmann <[email protected]>
----
drivers/net/spider_net.c | 77 ++++++++++++++++++++---------------------------
1 file changed, 34 insertions(+), 43 deletions(-)
Index: linux-2.6.18-mm2/drivers/net/spider_net.c
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net.c 2006-09-29 17:39:06.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net.c 2006-09-29 17:41:19.000000000 -0500
@@ -646,8 +646,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 (pci_dma_mapping_error(buf)) {
@@ -658,6 +659,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;
@@ -666,6 +671,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:
@@ -676,37 +683,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;
@@ -730,11 +716,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);
}
/**
@@ -753,22 +741,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
@@ -788,9 +784,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)
@@ -847,11 +853,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) {
@@ -873,12 +876,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;
}
@@ -895,15 +895,9 @@ 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);
-
- spin_unlock_irqrestore(&card->tx_chain.lock, flags);
}
/**
@@ -1930,10 +1924,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);
On Fri, 29 Sep 2006 18:29:11 -0500
[email protected] (Linas Vepstas) wrote:
> 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. One lock is used to protect
> three locations: the queue head and tail pointers, and the
> queue low-watermark location.
You have spider_net_set_low_watermark() walking the tx_chain outside
tx_chain.lock. Are you sure about that?
Am Saturday 30 September 2006 01:17 schrieb Linas Vepstas:
> Implement basic low-watermark support for the transmit queue.
> Hardware low-watermarks allow a properly configured kernel
> to continously stream data to a device and not have to handle
> any interrupts at all in doing so. Correct zero-interrupt
> operation can be actually observed for this driver, when the
> socket buffer is made large enough.
Acked-by: Arnd Bergmann <[email protected]>
Am Saturday 30 September 2006 01:15 schrieb Linas Vepstas:
> This patch increases the Burst Address alignment from 64 to 1024 in the
> Spidernet driver. This improves transmit performance for large packets.
>
> From: James K Lewis <[email protected]>
> Signed-off-by: James K Lewis <[email protected]>
> Signed-off-by: Linas Vepstas <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
Acked-by: Arnd Bergmann <[email protected]>
Am Saturday 30 September 2006 01:21 schrieb Linas Vepstas:
> This patch adds version information as reported by
> ethtool -i to the Spidernet driver.
>
> From: James K Lewis <[email protected]>
> Signed-off-by: James K Lewis <[email protected]>
> Signed-off-by: Linas Vepstas <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
Same comment as last time this was submitted:
> @@ -2303,6 +2304,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",
This is missing a printk level and a space character. More importantly,
you should not print the presence of the driver to the syslog, but
the presence of a device driven by it.
Arnd <><
Am Saturday 30 September 2006 01:26 schrieb Linas Vepstas:
> This patch moves transmit queue cleanup code out of the
> interrupt context, and into the NAPI polling routine.
>
> Signed-off-by: Linas Vepstas <[email protected]>
> Cc: James K Lewis <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
The subject of this mail should be "spidernet: use NAPI poll for
TX interrupts", otherwise
Acked-by: Arnd Bergmann <[email protected]>
Am Saturday 30 September 2006 01:19 schrieb Linas Vepstas:
> 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.
>
> Signed-off-by: Linas Vepstas <[email protected]>
> Signed-off-by: James K Lewis <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
Acked-by: Arnd Bergmann <[email protected]>
Am Saturday 30 September 2006 01:05 schrieb Linas Vepstas:
> Although these patches have not been baking in
> any -mm tree, they have been tested and are
> generally available as a part of the Cell SDK 2.0
> overseen by Arnd Bergmann. (Arnd, if you want
> to lend a voice of authority here, or to correct
> me, please do so...)
>
> The following sequence of six patches implement a
> series of changes to the transmit side of the
> spidernet ethernet device driver, significantly
> improving performance for large packets.
>
> This series of patches is almost identical to
> those previously mailed on 18-20 August, with one
> critical change: NAPI polling is used instead of
> homegrown polling.
>
> Although these patches improve things, I am not
> satisfied with how this driver behaves, and so
> plan to do additional work next week.
>
I'm not sure if I have missed a patch in here, but I
don't see anything reintroducing the 'netif_stop_queue'
that is missing from the transmit path.
Do you have a extra patch for that?
Arnd <><
On Sat, Sep 30, 2006 at 12:40:00PM +0200, Arnd Bergmann wrote:
> Am Saturday 30 September 2006 01:05 schrieb Linas Vepstas:
> >
> > Although these patches improve things, I am not
> > satisfied with how this driver behaves, and so
> > plan to do additional work next week.
> >
>
> I'm not sure if I have missed a patch in here, but I
> don't see anything reintroducing the 'netif_stop_queue'
> that is missing from the transmit path.
>
> Do you have a extra patch for that?
Unfinished. There are several ways in which the current
spider-net driver doesn't do things the way Greg KH's, etal
book on device drivers recommends. I was planning on combing
through these this week.
--linas
On Monday 02 October 2006 18:27, Linas Vepstas wrote:
> >
> > I'm not sure if I have missed a patch in here, but I
> > don't see anything reintroducing the 'netif_stop_queue'
> > that is missing from the transmit path.
> >
> > Do you have a extra patch for that?
>
> Unfinished. ?There are several ways in which the current
> spider-net driver doesn't do things the way Greg KH's, etal
> book on device drivers recommends. I was planning on combing
> through these this week.
Ok, that's good. However, removing the netif_stop_queue
was an obvious oversight that happened during the cleanup
last year.
Putting that one line back in should be a really safe fix for
the problem of overly high system load we sometimes see.
Arnd <><
On Sat, Sep 30, 2006 at 12:33:45PM +0200, Arnd Bergmann wrote:
> Am Saturday 30 September 2006 01:21 schrieb Linas Vepstas:
> >
> > ?static int __init spider_net_init(void)
> > ?{
> > +???????printk("spidernet Version %s.\n",VERSION);
> > +
>
> This is missing a printk level and a space character.
Revised patch below.
> More importantly,
> you should not print the presence of the driver to the syslog, but
> the presence of a device driven by it.
Most device drivers seem to print version numbers in the module_init
routines (i.e. right before calling pci_register_driver), rather than
in the probe routines; so this patch follows this convention.
--linas
This patch adds version information as reported by
ethtool -i to the Spidernet driver.
From: James K Lewis <[email protected]>
Signed-off-by: James K Lewis <[email protected]>
Signed-off-by: Linas Vepstas <[email protected]>
Acked-by: Arnd Bergmann <[email protected]>
----
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-mm2/drivers/net/spider_net.c
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net.c 2006-09-29 16:44:17.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net.c 2006-10-02 12:00:15.000000000 -0500
@@ -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;
@@ -2303,6 +2304,8 @@ static struct pci_driver spider_net_driv
*/
static int __init spider_net_init(void)
{
+ printk(KERN_INFO "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-mm2/drivers/net/spider_net.h
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net.h 2006-09-29 16:44:17.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net.h 2006-10-02 11:57:32.000000000 -0500
@@ -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-mm2/drivers/net/spider_net_ethtool.c
===================================================================
--- linux-2.6.18-mm2.orig/drivers/net/spider_net_ethtool.c 2006-09-29 16:33:43.000000000 -0500
+++ linux-2.6.18-mm2/drivers/net/spider_net_ethtool.c 2006-09-29 16:44:18.000000000 -0500
@@ -76,7 +76,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);
}
On Mon, Oct 02, 2006 at 06:50:39PM +0200, Arnd Bergmann wrote:
> On Monday 02 October 2006 18:27, Linas Vepstas wrote:
> > >
> > > I'm not sure if I have missed a patch in here, but I
> > > don't see anything reintroducing the 'netif_stop_queue'
> > > that is missing from the transmit path.
> > >
> > > Do you have a extra patch for that?
> >
> > Unfinished. ?There are several ways in which the current
> > spider-net driver doesn't do things the way Greg KH's, etal
> > book on device drivers recommends. I was planning on combing
> > through these this week.
>
> Ok, that's good. However, removing the netif_stop_queue
> was an obvious oversight that happened during the cleanup
> last year.
>
> Putting that one line back in should be a really safe fix for
> the problem of overly high system load we sometimes see.
Hmm. I have a patch from 5 weeks ago that seems to insert a bunch
of these. I'm not sure why it hadn't been mailed before, I'll
test and post as soon as I can.
--linas
On Fri, Sep 29, 2006 at 07:47:52PM -0700, Andrew Morton wrote:
> On Fri, 29 Sep 2006 18:29:11 -0500
> [email protected] (Linas Vepstas) wrote:
>
> > 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. One lock is used to protect
> > three locations: the queue head and tail pointers, and the
> > queue low-watermark location.
>
> You have spider_net_set_low_watermark() walking the tx_chain outside
> tx_chain.lock. Are you sure about that?
Yes. Its making an approximate count of the queue length, and I figured
that if its approximate to begin with, an unlocked version should be
just fine.
--linas