[PATCH] fix "ethtool -S" for tg3

[PATCH] fix "ethtool -S" for tg3

[Arthur Kepner]

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

Re: [PATCH] fix "ethtool -S" for tg3

[David S. Miller]

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.

Re: [PATCH] fix "ethtool -S" for tg3

[Arthur Kepner]

[-- 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)

Re: [PATCH] fix "ethtool -S" for tg3

[David S. Miller]

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.

[PATCH] "lockless loopback" patch for 2.6.6

[Arthur Kepner]

[-- 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:

Re: [PATCH] "lockless loopback" patch for 2.6.6

[Stephen Hemminger]

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:

Re: [PATCH] "lockless loopback" patch for 2.6.6

[David S. Miller]

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.

Re: [PATCH] "lockless loopback" patch for 2.6.6

[Andi Kleen]

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