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.

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

[Bob Gill]

On Tue, 2004-06-22 at 15:01, David S. Miller wrote:

> Thanks a lot Arthur, patch applied.


Hi.  Sorry for the late reply.  I applied the patch (so now my
drivers/net/loopback.c around line 173 looks like:

for (i=0; i < NR_CPUS; i++) {
                if (!cpu_possible(i))
                        continue;
                lb_stats = &per_cpu(loopback_stats, i);
                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;        }
                                 
        return stats;

(and if that is the patch, then I get --with extra debugging in the
kernel):

CPU:    0
EIP:    0060:[<co11f349>]    Not tainted
EFLAGS: 00010246   (2.6.7-bk5)
EIP is at __mod_timer+0x170/0x17d
eax: 00000000   ebx: 00000000   ecx: f7ba8a20   edx: f7ba8aa4
esi: f7ba8aa4   edi: 0000e000   ebp: f7af7e80   esp: f7af7e38
ds: 007b   es: 007b   ss: 0068
Process ifconfig (pid: 1535, threadinfo=f7af6000 task
Stack: 00000000 00001200 00000000 00000000 f7ba8800 0000e000 f7af7e80
f8aa6cb9
       f7ba8aa4 fffc163b 00000001 f7ba8800 f7ba8800 f7ba8a20 90326f48
f770f658
       c0328460 f7ba8800 00000000 c0256344 f7ba8800 f770f658 0000004c
f7fd2238
Call Trace:
 [<f8aa6cb9>] sis900_open+0x15f/0x166 [sis900]
 [<c0256344>] neigh_parms_alloc+0xa9/0xb9
 [<c0288a17>] inetdev_init+0x89/0x16e
 [<c028907e>] inet_set_ifa+0x9f/0x105
 [<c02888b6>] inet_alloc_ifa+0x22/0x48
 [<c028982b>] devinet_ioctl+0x2f3/0x560
 [<c028bbbd>] inet_ioctl+0x5e/0x9e
 [<c0248dc5>] sock_ioctl+0xed/0x28b
 [<c0248cd8>] sock_ioctl+0x0/0x28b
 [<c015c9e7>] sys_ioctl+0xf7/0x24c
 [<c0103eb1>] sysenter_past_esp+0x52/0x71
Code: 0f 0b 9b 00 4b 42 2b c0 e9 a1 fe ff ff 83 ec 14 89 7c 24 10


I will try (any patches), but unfortunately, my turnaround time will
(likely) be delayed about 20 hours from this post (working long hours
lately).

Thanks for all replies.
Bob