Re: [NET]: Lockless loopback patch (version 2).

Re: [NET]: Lockless loopback patch (version 2).

[Andrew Morton]

Linux Kernel Mailing List <linux-kernel@vger.kernel.org> wrote:
>
> 	[NET]: Lockless loopback patch (version 2).

The loopback_stats handling looks wrong:

+	if (likely(loopback_stats)) {
+		get_cpu_ptr(loopback_stats)->rx_bytes += skb->len;
+		get_cpu_ptr(loopback_stats)->tx_bytes += skb->len;
+		get_cpu_ptr(loopback_stats)->rx_packets++;
+		get_cpu_ptr(loopback_stats)->tx_packets++;
+		put_cpu_ptr(loopback_stats);

each get_cpu_ptr() does get_cpu(), which increments preempt_count().  But
there is only a single put_cpu_ptr() in there.


Still, I don't see why we need to use alloc_percpu() - why not
statically allocate it?

This compiles, but does need runtime testing.




Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 25-akpm/drivers/net/loopback.c |   35 ++++++++++++++++++-----------------
 1 files changed, 18 insertions(+), 17 deletions(-)

diff -puN drivers/net/loopback.c~loopback-percpu-fix drivers/net/loopback.c
--- 25/drivers/net/loopback.c~loopback-percpu-fix	2004-06-21 03:39:33.265306016 -0700
+++ 25-akpm/drivers/net/loopback.c	2004-06-21 03:50:58.327160784 -0700
@@ -55,8 +55,9 @@
 #include <linux/if_arp.h>	/* For ARPHRD_ETHER */
 #include <linux/ip.h>
 #include <linux/tcp.h>
+#include <linux/percpu.h>
 
-static struct net_device_stats *loopback_stats;
+static DEFINE_PER_CPU(struct net_device_stats, loopback_stats);
 
 #define LOOPBACK_OVERHEAD (128 + MAX_HEADER + 16 + 16)
 
@@ -124,6 +125,7 @@ static void emulate_large_send_offload(s
  */
 static int loopback_xmit(struct sk_buff *skb, struct net_device *dev)
 {
+	struct net_device_stats *lb_stats;
 
 	skb_orphan(skb);
 
@@ -142,13 +144,13 @@ static int loopback_xmit(struct sk_buff 
 	}
 
 	dev->last_rx = jiffies;
-	if (likely(loopback_stats)) {
-		get_cpu_ptr(loopback_stats)->rx_bytes += skb->len;
-		get_cpu_ptr(loopback_stats)->tx_bytes += skb->len;
-		get_cpu_ptr(loopback_stats)->rx_packets++;
-		get_cpu_ptr(loopback_stats)->tx_packets++;
-		put_cpu_ptr(loopback_stats);
-	}
+
+	lb_stats = &per_cpu(loopback_stats, get_cpu());
+	lb_stats->rx_bytes += skb->len;
+	lb_stats->tx_bytes += skb->len;
+	lb_stats->rx_packets++;
+	lb_stats->tx_packets++;
+	put_cpu();
 
 	netif_rx(skb);
 
@@ -165,17 +167,18 @@ static struct net_device_stats *get_stat
 	}
 
 	memset(stats, 0, sizeof(struct net_device_stats));
-	if (!loopback_stats) {
-		return stats;
-	}
 
 	for (i=0; i < NR_CPUS; i++) {
+		struct net_device_stats *lb_stats;
+
 		if (!cpu_possible(i)) 
 			continue;
-		stats->rx_bytes   += per_cpu_ptr(loopback_stats, i)->rx_bytes;
-		stats->tx_bytes   += per_cpu_ptr(loopback_stats, i)->tx_bytes;
-		stats->rx_packets += per_cpu_ptr(loopback_stats, i)->rx_packets;
-		stats->tx_packets += per_cpu_ptr(loopback_stats, i)->tx_packets;
+		lb_stats = &per_cpu(loopback_stats, get_cpu());
+		stats->rx_bytes   += lb_stats->rx_bytes;
+		stats->tx_bytes   += lb_stats->tx_bytes;
+		stats->rx_packets += lb_stats->rx_packets;
+		stats->tx_packets += lb_stats->tx_packets;
+		put_cpu();
 	}
 				
 	return stats;
@@ -211,8 +214,6 @@ int __init loopback_init(void)
 		loopback_dev.priv = stats;
 		loopback_dev.get_stats = &get_stats;
 	}
-
-	loopback_stats = alloc_percpu(struct net_device_stats);
 	
 	return register_netdev(&loopback_dev);
 };
_

Re: [NET]: Lockless loopback patch (version 2).

[Arthur Kepner]

On Mon, 21 Jun 2004, Andrew Morton wrote:

> Linux Kernel Mailing List <linux-kernel@vger.kernel.org> wrote:
> >
> > 	[NET]: Lockless loopback patch (version 2).
>
> The loopback_stats handling looks wrong:
>
> ....
> each get_cpu_ptr() does get_cpu(), which increments preempt_count().  But
> there is only a single put_cpu_ptr() in there.
>
>
> Still, I don't see why we need to use alloc_percpu() - why not
> statically allocate it?
>
> This compiles, but does need runtime testing.

Thanks, Andrew. I'll do some basic testing of this change and
post results (probably by tomorrow.)

--

Arthur

Re: [NET]: Lockless loopback patch (version 2).

[David S. Miller]

On Mon, 21 Jun 2004 03:57:02 -0700
Andrew Morton <akpm@osdl.org> wrote:

> Still, I don't see why we need to use alloc_percpu() - why not
> statically allocate it?
> 
> This compiles, but does need runtime testing.

Looks good to me, applied.

[PATCH] preempt count regression w/ lockless loopback patch

[Arthur Kepner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 548 bytes --]


On Mon, 21 Jun 2004, Andrew Morton wrote:

> The loopback_stats handling looks wrong:
> .....
>
> each get_cpu_ptr() does get_cpu(), which increments preempt_count().  But
> there is only a single put_cpu_ptr() in there.

Correct. At least one person has tripped over this already. (See "Re:
[2.6.7-bk] NFS-related kernel panic" on lkml.)

> .....
>
> This compiles, but does need runtime testing.
>

I made one small change in get_stats() and tested with a preemptible
kernel on a 32p system. Looks good. The tested patch is attached.

--

Arthur

[-- Attachment #2: patch for preempt count bug --]
[-- Type: TEXT/PLAIN, Size: 2162 bytes --]

--- linux.tmp/drivers/net/loopback.c	2004-06-22 09:00:41.000000000 -0700
+++ linux/drivers/net/loopback.c	2004-06-22 09:06:41.000000000 -0700
@@ -55,8 +55,9 @@
 #include <linux/if_arp.h>	/* For ARPHRD_ETHER */
 #include <linux/ip.h>
 #include <linux/tcp.h>
+#include <linux/percpu.h>
 
-static struct net_device_stats *loopback_stats;
+static DEFINE_PER_CPU(struct net_device_stats, loopback_stats);
 
 #define LOOPBACK_OVERHEAD (128 + MAX_HEADER + 16 + 16)
 
@@ -124,6 +125,7 @@
  */
 static int loopback_xmit(struct sk_buff *skb, struct net_device *dev)
 {
+	struct net_device_stats *lb_stats;
 
 	skb_orphan(skb);
 
@@ -142,13 +144,12 @@
 	}
 
 	dev->last_rx = jiffies;
-	if (likely(loopback_stats)) {
-		get_cpu_ptr(loopback_stats)->rx_bytes += skb->len;
-		get_cpu_ptr(loopback_stats)->tx_bytes += skb->len;
-		get_cpu_ptr(loopback_stats)->rx_packets++;
-		get_cpu_ptr(loopback_stats)->tx_packets++;
-		put_cpu_ptr(loopback_stats);
-	}
+	lb_stats = &per_cpu(loopback_stats, get_cpu());
+	lb_stats->rx_bytes += skb->len;
+	lb_stats->tx_bytes += skb->len;
+	lb_stats->rx_packets++;
+	lb_stats->tx_packets++;
+	put_cpu();
 
 	netif_rx(skb);
 
@@ -165,19 +166,19 @@
 	}
 
 	memset(stats, 0, sizeof(struct net_device_stats));
-	if (!loopback_stats) {
-		return stats;
-	}
 
 	for (i=0; i < NR_CPUS; i++) {
+		struct net_device_stats *lb_stats;
+
 		if (!cpu_possible(i)) 
 			continue;
-		stats->rx_bytes   += per_cpu_ptr(loopback_stats, i)->rx_bytes;
-		stats->tx_bytes   += per_cpu_ptr(loopback_stats, i)->tx_bytes;
-		stats->rx_packets += per_cpu_ptr(loopback_stats, i)->rx_packets;
-		stats->tx_packets += per_cpu_ptr(loopback_stats, i)->tx_packets;
+		lb_stats = &per_cpu(loopback_stats, i);
+		stats->rx_bytes   += lb_stats->rx_bytes;
+		stats->tx_bytes   += lb_stats->tx_bytes;
+		stats->rx_packets += lb_stats->rx_packets;
+		stats->tx_packets += lb_stats->tx_packets;
 	}
-				
+
 	return stats;
 }
 
@@ -212,8 +213,6 @@
 		loopback_dev.get_stats = &get_stats;
 	}
 
-	loopback_stats = alloc_percpu(struct net_device_stats);
-	
 	return register_netdev(&loopback_dev);
 };
 

Re: [PATCH] preempt count regression w/ lockless loopback patch

[David S. Miller]

On Tue, 22 Jun 2004 09:24:40 -0700
Arthur Kepner <akepner@sgi.com> wrote:

> I made one small change in get_stats() and tested with a preemptible
> kernel on a 32p system. Looks good. The tested patch is attached.

Can you instead give a patch relative to Andrew's?  His is
applied already and in fact pushed into Linus's tree.

Re: [PATCH] preempt count regression w/ lockless loopback patch

[Arthur Kepner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 213 bytes --]

On Tue, 22 Jun 2004, David S. Miller wrote:

> ....
> Can you instead give a patch relative to Andrew's?  His is
> applied already and in fact pushed into Linus's tree.
>

Yes. It is in the attachment.

--

Arthur

[-- Attachment #2: patch relative to akpm --]
[-- Type: TEXT/PLAIN, Size: 537 bytes --]

--- linux.akpm/drivers/net/loopback.c	2004-06-22 13:35:13.000000000 -0700
+++ linux/drivers/net/loopback.c	2004-06-22 13:42:27.000000000 -0700
@@ -173,12 +173,11 @@
 
 		if (!cpu_possible(i)) 
 			continue;
-		lb_stats = &per_cpu(loopback_stats, get_cpu());
+		lb_stats = &per_cpu(loopback_stats, i);
 		stats->rx_bytes   += lb_stats->rx_bytes;
 		stats->tx_bytes   += lb_stats->tx_bytes;
 		stats->rx_packets += lb_stats->rx_packets;
 		stats->tx_packets += lb_stats->tx_packets;
-		put_cpu();
 	}
 				
 	return stats;

Re: [PATCH] preempt count regression w/ lockless loopback patch

[David S. Miller]

On Tue, 22 Jun 2004 13:46:07 -0700
Arthur Kepner <akepner@sgi.com> wrote:

> On Tue, 22 Jun 2004, David S. Miller wrote:
> 
> > ....
> > Can you instead give a patch relative to Andrew's?  His is
> > applied already and in fact pushed into Linus's tree.
> >
> 
> Yes. It is in the attachment.

Thanks a lot Arthur, patch applied.