* Re: [NET]: Lockless loopback patch (version 2). [not found] <200406210510.i5L5A340018849@hera.kernel.org> @ 2004-06-21 10:57 ` Andrew Morton 2004-06-21 15:09 ` Arthur Kepner ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Andrew Morton @ 2004-06-21 10:57 UTC (permalink / raw) To: netdev; +Cc: David S. Miller, akepner 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); }; _ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [NET]: Lockless loopback patch (version 2). 2004-06-21 10:57 ` [NET]: Lockless loopback patch (version 2) Andrew Morton @ 2004-06-21 15:09 ` Arthur Kepner 2004-06-21 16:39 ` David S. Miller 2004-06-22 16:24 ` [PATCH] preempt count regression w/ lockless loopback patch Arthur Kepner 2 siblings, 0 replies; 8+ messages in thread From: Arthur Kepner @ 2004-06-21 15:09 UTC (permalink / raw) To: Andrew Morton; +Cc: netdev, David S. Miller 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [NET]: Lockless loopback patch (version 2). 2004-06-21 10:57 ` [NET]: Lockless loopback patch (version 2) Andrew Morton 2004-06-21 15:09 ` Arthur Kepner @ 2004-06-21 16:39 ` David S. Miller 2004-06-22 16:24 ` [PATCH] preempt count regression w/ lockless loopback patch Arthur Kepner 2 siblings, 0 replies; 8+ messages in thread From: David S. Miller @ 2004-06-21 16:39 UTC (permalink / raw) To: Andrew Morton; +Cc: netdev, akepner 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] preempt count regression w/ lockless loopback patch 2004-06-21 10:57 ` [NET]: Lockless loopback patch (version 2) Andrew Morton 2004-06-21 15:09 ` Arthur Kepner 2004-06-21 16:39 ` David S. Miller @ 2004-06-22 16:24 ` Arthur Kepner 2004-06-22 19:39 ` David S. Miller 2 siblings, 1 reply; 8+ messages in thread From: Arthur Kepner @ 2004-06-22 16:24 UTC (permalink / raw) To: Andrew Morton; +Cc: netdev, David S. Miller, Chris Wright, Bob Gill [-- 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); }; ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] preempt count regression w/ lockless loopback patch 2004-06-22 16:24 ` [PATCH] preempt count regression w/ lockless loopback patch Arthur Kepner @ 2004-06-22 19:39 ` David S. Miller 2004-06-22 20:46 ` Arthur Kepner 0 siblings, 1 reply; 8+ messages in thread From: David S. Miller @ 2004-06-22 19:39 UTC (permalink / raw) To: Arthur Kepner; +Cc: akpm, netdev, chrisw, gillb4 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] preempt count regression w/ lockless loopback patch 2004-06-22 19:39 ` David S. Miller @ 2004-06-22 20:46 ` Arthur Kepner 2004-06-22 21:01 ` David S. Miller 0 siblings, 1 reply; 8+ messages in thread From: Arthur Kepner @ 2004-06-22 20:46 UTC (permalink / raw) To: David S. Miller; +Cc: akpm, netdev, chrisw, gillb4 [-- 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; ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] preempt count regression w/ lockless loopback patch 2004-06-22 20:46 ` Arthur Kepner @ 2004-06-22 21:01 ` David S. Miller 2004-06-23 5:21 ` Bob Gill 0 siblings, 1 reply; 8+ messages in thread From: David S. Miller @ 2004-06-22 21:01 UTC (permalink / raw) To: Arthur Kepner; +Cc: akpm, netdev, chrisw, gillb4 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] preempt count regression w/ lockless loopback patch 2004-06-22 21:01 ` David S. Miller @ 2004-06-23 5:21 ` Bob Gill 0 siblings, 0 replies; 8+ messages in thread From: Bob Gill @ 2004-06-23 5:21 UTC (permalink / raw) To: David S. Miller, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2004-06-23 5:16 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <200406210510.i5L5A340018849@hera.kernel.org>
2004-06-21 10:57 ` [NET]: Lockless loopback patch (version 2) Andrew Morton
2004-06-21 15:09 ` Arthur Kepner
2004-06-21 16:39 ` David S. Miller
2004-06-22 16:24 ` [PATCH] preempt count regression w/ lockless loopback patch Arthur Kepner
2004-06-22 19:39 ` David S. Miller
2004-06-22 20:46 ` Arthur Kepner
2004-06-22 21:01 ` David S. Miller
2004-06-23 5:21 ` Bob Gill
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.