* [PATCH] fix "ethtool -S" for tg3
@ 2004-05-11 19:57 Arthur Kepner
2004-05-12 19:08 ` David S. Miller
0 siblings, 1 reply; 8+ messages in thread
From: Arthur Kepner @ 2004-05-11 19:57 UTC (permalink / raw)
To: davem; +Cc: netdev
Here's a small mod I had to make to get "ethtool -S" to work with tg3 in
2.6.6:
8<-----------------------------------------------------------------------------
--- linux/drivers/net/tg3.c 2004-05-11 12:11:45.000000000 -0700
+++ linux.orig/drivers/net/tg3.c 2004-05-11 12:34:47.000000000 -0700
@@ -6288,7 +6288,7 @@
struct ethtool_stats *estats, u64 *tmp_stats)
{
struct tg3 *tp = dev->priv;
- memcpy(tmp_stats, &tp->estats, sizeof(tp->estats));
+ memcpy(tmp_stats, tg3_get_estats(tp), sizeof(tp->estats));
}
8<-----------------------------------------------------------------------------
--
Arthur
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] fix "ethtool -S" for tg3 2004-05-11 19:57 [PATCH] fix "ethtool -S" for tg3 Arthur Kepner @ 2004-05-12 19:08 ` David S. Miller 2004-05-12 20:00 ` Arthur Kepner 0 siblings, 1 reply; 8+ messages in thread From: David S. Miller @ 2004-05-12 19:08 UTC (permalink / raw) To: Arthur Kepner; +Cc: netdev On Tue, 11 May 2004 12:57:42 -0700 Arthur Kepner <akepner@sgi.com> wrote: > --- linux/drivers/net/tg3.c 2004-05-11 12:11:45.000000000 -0700 > +++ linux.orig/drivers/net/tg3.c 2004-05-11 12:34:47.000000000 -0700 Patch doesn't apply, your mail client turned all the tabs into spaces. Use attachments if necessary, thanks. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fix "ethtool -S" for tg3 2004-05-12 19:08 ` David S. Miller @ 2004-05-12 20:00 ` Arthur Kepner 2004-05-20 4:26 ` David S. Miller 2004-05-21 21:04 ` [PATCH] "lockless loopback" patch for 2.6.6 Arthur Kepner 0 siblings, 2 replies; 8+ messages in thread From: Arthur Kepner @ 2004-05-12 20:00 UTC (permalink / raw) To: David S. Miller; +Cc: netdev [-- Attachment #1: Type: TEXT/PLAIN, Size: 209 bytes --] On Wed, 12 May 2004, David S. Miller wrote: > ..... > Patch doesn't apply, your mail client turned all the tabs > into spaces. Use attachments if necessary, thanks. > OK. Attached is the patch. -- Arthur [-- Attachment #2: ethtool -S patch --] [-- Type: TEXT/PLAIN, Size: 447 bytes --] --- linux.orig/drivers/net/tg3.c 2004-05-12 12:41:58.000000000 -0700 +++ linux/drivers/net/tg3.c 2004-05-12 12:47:06.000000000 -0700 @@ -6284,7 +6284,7 @@ struct ethtool_stats *estats, u64 *tmp_stats) { struct tg3 *tp = dev->priv; - memcpy(tmp_stats, &tp->estats, sizeof(tp->estats)); + memcpy(tmp_stats, tg3_get_estats(tp), sizeof(tp->estats)); } static int tg3_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fix "ethtool -S" for tg3 2004-05-12 20:00 ` Arthur Kepner @ 2004-05-20 4:26 ` David S. Miller 2004-05-21 21:04 ` [PATCH] "lockless loopback" patch for 2.6.6 Arthur Kepner 1 sibling, 0 replies; 8+ messages in thread From: David S. Miller @ 2004-05-20 4:26 UTC (permalink / raw) To: Arthur Kepner; +Cc: netdev On Wed, 12 May 2004 13:00:24 -0700 Arthur Kepner <akepner@sgi.com> wrote: > On Wed, 12 May 2004, David S. Miller wrote: > > > ..... > > Patch doesn't apply, your mail client turned all the tabs > > into spaces. Use attachments if necessary, thanks. > > > > OK. Attached is the patch. Thanks a lot for following up on this Arthur. Patch applied. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] "lockless loopback" patch for 2.6.6 2004-05-12 20:00 ` Arthur Kepner 2004-05-20 4:26 ` David S. Miller @ 2004-05-21 21:04 ` Arthur Kepner 2004-05-21 21:45 ` Stephen Hemminger ` (2 more replies) 1 sibling, 3 replies; 8+ messages in thread From: Arthur Kepner @ 2004-05-21 21:04 UTC (permalink / raw) To: David S. Miller; +Cc: netdev [-- Attachment #1: Type: TEXT/PLAIN, Size: 1773 bytes --] Lock contention on the loopback device can lead to poor performance, even an essentially hung system, on systems with many processors. For the loopback device, the only purpose that locking serves is to protect the device statistics. The attached patch keeps per-cpu statistics for the loopback device and removes all locking. The patch is against 2.6.6. Worst-case loopback lock contention (2.4 kernel) ================================================ The following 'lockstat' data was taken with a 2.4 kernel (though I expect similar results with 2.6). The test scenario is that N transmitters simultaneously send to N receivers, via the loopback, on an N cpu system. (This was an attempt to simulate a hang seen at a customer site on a large system.) N SPINLOCKS HOLD WAIT UTIL CON MEAN( MAX ) MEAN( MAX )(% CPU) NOWAIT SPIN NAME ------------------------------------------------------------------------------ 2 6.0% 3.9% 48us(1397us) 74us(1172us)(0.18%) 96.1% 3.9% loopback_dev+0x1c0 8 17.0% 19.3% 137us( 117ms) 841us( 118ms)( 2.5%) 80.7% 19.3% loopback_dev+0x1c0 31* 72.2% 88.4% 1262us( 73ms) 25ms( 814ms)(39.7%) 11.6% 88.4% loopback_dev+0x1c0 64 74.7% 89.9% 2814us(1068ms) 85ms(4925ms)(31.5%) 10.1% 89.9% loopback_dev+0x1c0 (* yes, 31, one processor was down on a 32p system) In the 64p case, the system is essentially unusable for interactive processes when running this test case. Throughput on the loopback device also scales very poorly. In fact, if more than approximately 16 processors are used, throughput decreases as more processors are added. With (the 2.4 version of) the patch, the system remains quite usable during the same test scenario and throughput scales much better. -- Arthur [-- Attachment #2: lockless loopback patch --] [-- Type: TEXT/PLAIN, Size: 4981 bytes --] diff -Naur linux.orig/drivers/net/loopback.c linux/drivers/net/loopback.c --- linux.orig/drivers/net/loopback.c 2004-05-21 11:00:24.000000000 -0700 +++ linux/drivers/net/loopback.c 2004-05-21 11:04:00.000000000 -0700 @@ -123,7 +123,9 @@ */ static int loopback_xmit(struct sk_buff *skb, struct net_device *dev) { - struct net_device_stats *stats = dev->priv; + struct net_device_stats *global_stats = dev->priv; + struct net_device_stats *percpu_stats = 0; + struct net_device_stats *local_stats = 0; skb_orphan(skb); @@ -142,11 +144,13 @@ } dev->last_rx = jiffies; - if (likely(stats)) { - stats->rx_bytes+=skb->len; - stats->tx_bytes+=skb->len; - stats->rx_packets++; - stats->tx_packets++; + if (likely(global_stats)) { + percpu_stats = global_stats + 1; + local_stats = &percpu_stats[smp_processor_id()]; + local_stats->rx_bytes+=skb->len; + local_stats->tx_bytes+=skb->len; + local_stats->rx_packets++; + local_stats->tx_packets++; } netif_rx(skb); @@ -156,7 +160,22 @@ static struct net_device_stats *get_stats(struct net_device *dev) { - return (struct net_device_stats *)dev->priv; + int i; + struct net_device_stats *global_stats = dev->priv; + struct net_device_stats *percpu_stats = 0; + + if (global_stats) { + percpu_stats = global_stats + 1; + /* zero the system-wide stats */ + memset(global_stats, 0, sizeof(struct net_device_stats)); + for ( i = 0; i < NR_CPUS; i++ ) { + global_stats->rx_bytes += percpu_stats[i].rx_bytes; + global_stats->tx_bytes += percpu_stats[i].tx_bytes; + global_stats->rx_packets += percpu_stats[i].rx_packets; + global_stats->tx_packets += percpu_stats[i].tx_packets; + } + } + return global_stats; } struct net_device loopback_dev = { @@ -181,13 +200,19 @@ { struct net_device_stats *stats; +#define NR_LOOP_STATS (NR_CPUS + 1) + /* the first net_device_stats structure is for the whole system, the + * remaining NR_CPUS of them are for each cpu */ + /* Can survive without statistics */ - stats = kmalloc(sizeof(struct net_device_stats), GFP_KERNEL); + stats = kmalloc(NR_LOOP_STATS * sizeof(struct net_device_stats), GFP_KERNEL); + if (stats) { - memset(stats, 0, sizeof(struct net_device_stats)); + memset(stats, 0, NR_LOOP_STATS * sizeof(struct net_device_stats)); loopback_dev.priv = stats; loopback_dev.get_stats = &get_stats; } +#undef NR_LOOP_STATS return register_netdev(&loopback_dev); }; diff -Naur linux.orig/net/core/dev.c linux/net/core/dev.c --- linux.orig/net/core/dev.c 2004-05-21 11:01:01.000000000 -0700 +++ linux/net/core/dev.c 2004-05-21 11:03:44.000000000 -0700 @@ -1295,6 +1295,7 @@ struct net_device *dev = skb->dev; struct Qdisc *q; int rc = -ENOMEM; + int loopback = dev->flags & IFF_LOOPBACK; if (skb_shinfo(skb)->frag_list && !(dev->features & NETIF_F_FRAGLIST) && @@ -1322,17 +1323,19 @@ } /* Grab device queue */ - spin_lock_bh(&dev->queue_lock); - q = dev->qdisc; - if (q->enqueue) { - rc = q->enqueue(skb, q); + if (!loopback) { + spin_lock_bh(&dev->queue_lock); + q = dev->qdisc; + if (q->enqueue) { + rc = q->enqueue(skb, q); - qdisc_run(dev); + qdisc_run(dev); - spin_unlock_bh(&dev->queue_lock); - rc = rc == NET_XMIT_BYPASS ? NET_XMIT_SUCCESS : rc; - goto out; - } + spin_unlock_bh(&dev->queue_lock); + rc = rc == NET_XMIT_BYPASS ? NET_XMIT_SUCCESS : rc; + goto out; + } + } /* !loopback */ /* The device has no queue. Common case for software devices: loopback, all the sorts of tunnels... @@ -1354,11 +1357,13 @@ * The spin_lock effectivly does a preempt lock, but * we are about to drop that... */ - preempt_disable(); - spin_unlock(&dev->queue_lock); - spin_lock(&dev->xmit_lock); - dev->xmit_lock_owner = cpu; - preempt_enable(); + if (!loopback) { + preempt_disable(); + spin_unlock(&dev->queue_lock); + spin_lock(&dev->xmit_lock); + dev->xmit_lock_owner = cpu; + preempt_enable(); + } if (!netif_queue_stopped(dev)) { if (netdev_nit) @@ -1366,13 +1371,17 @@ rc = 0; if (!dev->hard_start_xmit(skb, dev)) { - dev->xmit_lock_owner = -1; - spin_unlock_bh(&dev->xmit_lock); + if (!loopback) { + dev->xmit_lock_owner = -1; + spin_unlock_bh(&dev->xmit_lock); + } goto out; } } - dev->xmit_lock_owner = -1; - spin_unlock_bh(&dev->xmit_lock); + if (!loopback) { + dev->xmit_lock_owner = -1; + spin_unlock_bh(&dev->xmit_lock); + } if (net_ratelimit()) printk(KERN_CRIT "Virtual device %s asks to " "queue packet!\n", dev->name); @@ -1385,7 +1394,8 @@ "%s, fix it urgently!\n", dev->name); } } - spin_unlock_bh(&dev->queue_lock); + if (!loopback) + spin_unlock_bh(&dev->queue_lock); out_enetdown: rc = -ENETDOWN; out_kfree_skb: ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] "lockless loopback" patch for 2.6.6 2004-05-21 21:04 ` [PATCH] "lockless loopback" patch for 2.6.6 Arthur Kepner @ 2004-05-21 21:45 ` Stephen Hemminger 2004-05-21 21:45 ` David S. Miller 2004-05-22 12:20 ` Andi Kleen 2 siblings, 0 replies; 8+ messages in thread From: Stephen Hemminger @ 2004-05-21 21:45 UTC (permalink / raw) To: Arthur Kepner; +Cc: David S. Miller, netdev On Fri, 21 May 2004 14:04:09 -0700 Arthur Kepner <akepner@sgi.com> wrote: > > Lock contention on the loopback device can lead to poor > performance, even an essentially hung system, on systems > with many processors. > > For the loopback device, the only purpose that locking serves > is to protect the device statistics. The attached patch > keeps per-cpu statistics for the loopback device and removes > all locking. The patch is against 2.6.6. The problem is more general than just loopback. This kind of special case hack is the right thing. Really fast network devices will have the same issues. > diff -Naur linux.orig/drivers/net/loopback.c linux/drivers/net/loopback.c > --- linux.orig/drivers/net/loopback.c 2004-05-21 11:00:24.000000000 -0700 > +++ linux/drivers/net/loopback.c 2004-05-21 11:04:00.000000000 -0700 > @@ -123,7 +123,9 @@ > */ > static int loopback_xmit(struct sk_buff *skb, struct net_device *dev) > { > - struct net_device_stats *stats = dev->priv; > + struct net_device_stats *global_stats = dev->priv; > + struct net_device_stats *percpu_stats = 0; > + struct net_device_stats *local_stats = 0; > Either do per-cpu stats or don't this seems confused with a mix of per-cpu and global. > skb_orphan(skb); > > @@ -142,11 +144,13 @@ > } > > dev->last_rx = jiffies; > - if (likely(stats)) { > - stats->rx_bytes+=skb->len; > - stats->tx_bytes+=skb->len; > - stats->rx_packets++; > - stats->tx_packets++; > + if (likely(global_stats)) { > + percpu_stats = global_stats + 1; > + local_stats = &percpu_stats[smp_processor_id()]; > + local_stats->rx_bytes+=skb->len; > + local_stats->tx_bytes+=skb->len; > + local_stats->rx_packets++; > + local_stats->tx_packets++; > } > > netif_rx(skb); > @@ -156,7 +160,22 @@ > > static struct net_device_stats *get_stats(struct net_device *dev) > { > - return (struct net_device_stats *)dev->priv; > + int i; > + struct net_device_stats *global_stats = dev->priv; > + struct net_device_stats *percpu_stats = 0; > + > + if (global_stats) { > + percpu_stats = global_stats + 1; > + /* zero the system-wide stats */ > + memset(global_stats, 0, sizeof(struct net_device_stats)); > + for ( i = 0; i < NR_CPUS; i++ ) { > + global_stats->rx_bytes += percpu_stats[i].rx_bytes; > + global_stats->tx_bytes += percpu_stats[i].tx_bytes; > + global_stats->rx_packets += percpu_stats[i].rx_packets; > + global_stats->tx_packets += percpu_stats[i].tx_packets; > + } > + } > + return global_stats; > } > > struct net_device loopback_dev = { > @@ -181,13 +200,19 @@ > { > struct net_device_stats *stats; > > +#define NR_LOOP_STATS (NR_CPUS + 1) > + /* the first net_device_stats structure is for the whole system, the > + * remaining NR_CPUS of them are for each cpu */ > + > /* Can survive without statistics */ > - stats = kmalloc(sizeof(struct net_device_stats), GFP_KERNEL); > + stats = kmalloc(NR_LOOP_STATS * sizeof(struct net_device_stats), GFP_KERNEL); > Use use per-cpu data rather than slots in an array. > if (stats) { > - memset(stats, 0, sizeof(struct net_device_stats)); > + memset(stats, 0, NR_LOOP_STATS * sizeof(struct net_device_stats)); > loopback_dev.priv = stats; > loopback_dev.get_stats = &get_stats; > } > +#undef NR_LOOP_STATS > > return register_netdev(&loopback_dev); > }; > diff -Naur linux.orig/net/core/dev.c linux/net/core/dev.c > --- linux.orig/net/core/dev.c 2004-05-21 11:01:01.000000000 -0700 > +++ linux/net/core/dev.c 2004-05-21 11:03:44.000000000 -0700 > @@ -1295,6 +1295,7 @@ > struct net_device *dev = skb->dev; > struct Qdisc *q; > int rc = -ENOMEM; > + int loopback = dev->flags & IFF_LOOPBACK; Boolean flags are for PASCAL programmers, test it where you use it. > > if (skb_shinfo(skb)->frag_list && > !(dev->features & NETIF_F_FRAGLIST) && > @@ -1322,17 +1323,19 @@ > } The real problem here is having a single queue, for the loopback device you don't want a a queue at all by default. That would get around a lot of the mess. > /* Grab device queue */ > - spin_lock_bh(&dev->queue_lock); > - q = dev->qdisc; > - if (q->enqueue) { > - rc = q->enqueue(skb, q); > + if (!loopback) { > + spin_lock_bh(&dev->queue_lock); > + q = dev->qdisc; > + if (q->enqueue) { > + rc = q->enqueue(skb, q); > > - qdisc_run(dev); > + qdisc_run(dev); > > - spin_unlock_bh(&dev->queue_lock); > - rc = rc == NET_XMIT_BYPASS ? NET_XMIT_SUCCESS : rc; > - goto out; > - } > + spin_unlock_bh(&dev->queue_lock); > + rc = rc == NET_XMIT_BYPASS ? NET_XMIT_SUCCESS : rc; > + goto out; > + } > + } /* !loopback */ > > /* The device has no queue. Common case for software devices: > loopback, all the sorts of tunnels... > @@ -1354,11 +1357,13 @@ > * The spin_lock effectivly does a preempt lock, but > * we are about to drop that... > */ > - preempt_disable(); > - spin_unlock(&dev->queue_lock); > - spin_lock(&dev->xmit_lock); > - dev->xmit_lock_owner = cpu; > - preempt_enable(); > + if (!loopback) { > + preempt_disable(); > + spin_unlock(&dev->queue_lock); > + spin_lock(&dev->xmit_lock); > + dev->xmit_lock_owner = cpu; > + preempt_enable(); > + } > > if (!netif_queue_stopped(dev)) { > if (netdev_nit) > @@ -1366,13 +1371,17 @@ > > rc = 0; > if (!dev->hard_start_xmit(skb, dev)) { > - dev->xmit_lock_owner = -1; > - spin_unlock_bh(&dev->xmit_lock); > + if (!loopback) { > + dev->xmit_lock_owner = -1; > + spin_unlock_bh(&dev->xmit_lock); > + } > goto out; > } > } > - dev->xmit_lock_owner = -1; > - spin_unlock_bh(&dev->xmit_lock); > + if (!loopback) { > + dev->xmit_lock_owner = -1; > + spin_unlock_bh(&dev->xmit_lock); > + } > if (net_ratelimit()) > printk(KERN_CRIT "Virtual device %s asks to " > "queue packet!\n", dev->name); > @@ -1385,7 +1394,8 @@ > "%s, fix it urgently!\n", dev->name); > } > } > - spin_unlock_bh(&dev->queue_lock); > + if (!loopback) > + spin_unlock_bh(&dev->queue_lock); > out_enetdown: > rc = -ENETDOWN; > out_kfree_skb: ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] "lockless loopback" patch for 2.6.6 2004-05-21 21:04 ` [PATCH] "lockless loopback" patch for 2.6.6 Arthur Kepner 2004-05-21 21:45 ` Stephen Hemminger @ 2004-05-21 21:45 ` David S. Miller 2004-05-22 12:20 ` Andi Kleen 2 siblings, 0 replies; 8+ messages in thread From: David S. Miller @ 2004-05-21 21:45 UTC (permalink / raw) To: Arthur Kepner; +Cc: netdev On Fri, 21 May 2004 14:04:09 -0700 Arthur Kepner <akepner@sgi.com> wrote: > Lock contention on the loopback device can lead to poor > performance, even an essentially hung system, on systems > with many processors. > > For the loopback device, the only purpose that locking serves > is to protect the device statistics. The attached patch > keeps per-cpu statistics for the loopback device and removes > all locking. The patch is against 2.6.6. It is legal to attach queueing disciplines to the loopback device but your patch makes that impossible. I don't think it is worth worrying about things like this especially if you're going to be adding ugly loopback special cases to the generic device handling code like this. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] "lockless loopback" patch for 2.6.6 2004-05-21 21:04 ` [PATCH] "lockless loopback" patch for 2.6.6 Arthur Kepner 2004-05-21 21:45 ` Stephen Hemminger 2004-05-21 21:45 ` David S. Miller @ 2004-05-22 12:20 ` Andi Kleen 2 siblings, 0 replies; 8+ messages in thread From: Andi Kleen @ 2004-05-22 12:20 UTC (permalink / raw) To: Arthur Kepner; +Cc: netdev On Fri, May 21, 2004 at 02:04:09PM -0700, Arthur Kepner wrote: > > Lock contention on the loopback device can lead to poor > performance, even an essentially hung system, on systems > with many processors. > > For the loopback device, the only purpose that locking serves > is to protect the device statistics. The attached patch > keeps per-cpu statistics for the loopback device and removes > all locking. The patch is against 2.6.6. [...] It looks quite ugly. How about you just create multiple loopback devices and use these preferably from specific CPUs ? One loopback device per CPU would be probably overkill, but maybe one per 8 CPUs. -Andi ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2004-05-22 12:20 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2004-05-11 19:57 [PATCH] fix "ethtool -S" for tg3 Arthur Kepner 2004-05-12 19:08 ` David S. Miller 2004-05-12 20:00 ` Arthur Kepner 2004-05-20 4:26 ` David S. Miller 2004-05-21 21:04 ` [PATCH] "lockless loopback" patch for 2.6.6 Arthur Kepner 2004-05-21 21:45 ` Stephen Hemminger 2004-05-21 21:45 ` David S. Miller 2004-05-22 12:20 ` Andi Kleen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).