Re: do_IRQ: stack overflow: 872..

• Previous message: Alan Cox: "Re: Make pipe data structure be a circular list of pages, rather than"
• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

To: Andi Kleen <ak@suse.de>
Date:	Fri, 07 Jan 2005 17:05:59 +0000

On Sat, 2004-12-18 at 08:50 +0100, Andi Kleen wrote:
> It's not really an oops, just a warning that stack space got quiet
> tight.
>
> The problem seems to be that the br netfilter code is nesting far too
> deeply and recursing several times. Looks like a design bug to me,
> it shouldn't do that.

I don't think it's recursing -- I think the stack trace is just a bit
noisy. The problem is that the bridge code, especially with br_netfilter
in the equation, is implicated in code paths which are just _too_ deep.
This happens when you're bridging packets received in an interrupt while
you were deep in journalling code, and it's also been seen with a call
trace something like nfs->sunrpc->ip->bridge->br_netfilter.

One option might be to make br_dev_xmit() just queue the packet rather
than trying to deliver it to all the slave devices immediately. Then the
actual retransmission can be handled from a context where we're _not_
short of stack; perhaps from a dedicated kernel thread.

Unfortunately that approach would introduce a lot of latency on all
packets we pass. Another option would be to have all architectures
provide a stack_available() function and for br_dev_xmit() to queue the
packet only if we're short of stack, while still sending most packets
immediately.

Proof of concept below; obviously the stack_available() is an evil hack
and would need to be done more sanely. Comments?

===== net/bridge/br_device.c 1.17 vs edited =====
--- 1.17/net/bridge/br_device.c 2004-07-29 22:40:51 +01:00
+++ edited/net/bridge/br_device.c 2005-01-07 16:54:26 +00:00
@@ -19,6 +19,13 @@
 #include <asm/uaccess.h>
 #include "br_private.h"
 
+static inline unsigned long stack_available(void)
+{
+ unsigned long esp;
+ asm volatile("movl %%esp,%0" : "=r"(esp));
+ return esp - (unsigned long)current - sizeof(struct thread_info);
+}
+
 static struct net_device_stats *br_dev_get_stats(struct net_device *dev)
 {
         struct net_bridge *br;
@@ -34,6 +41,14 @@
         const unsigned char *dest = skb->data;
         struct net_bridge_fdb_entry *dst;
 
+ if (stack_available() < THREAD_SIZE/2) {
+ if (net_ratelimit()) {
+ printk(KERN_DEBUG "Bridge device %s queues packet due to stack shortage\n",
+ dev->name);
+ }
+ return NETDEV_TX_BUSY;
+ }
+
         br->statistics.tx_packets++;
         br->statistics.tx_bytes += skb->len;
 
@@ -104,7 +119,7 @@
         SET_MODULE_OWNER(dev);
         dev->stop = br_dev_stop;
         dev->accept_fastpath = br_dev_accept_fastpath;
- dev->tx_queue_len = 0;
+ dev->tx_queue_len = 5;
         dev->set_mac_address = NULL;
         dev->priv_flags = IFF_EBRIDGE;
 }

-- 
dwmw2
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

• Previous message: Alan Cox: "Re: Make pipe data structure be a circular list of pages, rather than"
• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]