netpoll: various bugfixes
Subsystems (2)
cover netpoll: various bugfixes
This patch series cleans up a few outstanding bugs in netpoll:
- two bugfixes from Jeff Moyer's netpoll bonding
- a tweak to e1000's netpoll stub
- timeout handling for e1000 with carrier loss
- prefilling SKBs at init
- a fix-up for a race discovered in initialization
- an unused variable warning
This patch set was tested over repeated rebooting with both tg3 and e1000 and random cable disconnection, with and without SMP and preempt. Please apply.
PATCH 1/8 netpoll: rx_flags bugfix
Initialize npinfo->rx_flags. The way it stands now, this will have random garbage, and so will incur a locking penalty even when an rx_hook isn't registered and we are not active in the netpoll polling code.
net/core/netpoll.c renamed from net/core/netpoll.c.orig +1 -0
@@ -639,6 +639,7 @@ int netpoll_setup(struct netpoll *np)if (!npinfo) goto release; + npinfo->rx_flags =0 ; npinfo->rx_np =NULL ; npinfo->poll_lock =SPIN_LOCK_UNLOCKED ; npinfo->poll_owner =-1 ;
PATCH 2/8 netpoll: deadlock bugfix
This fixes an obvious deadlock in the netpoll code. netpoll_rx takes the npinfo->rx_lock. netpoll_rx is also the only caller of arp_reply (through __netpoll_rx). As such, it is not necessary to take this lock.
net/core/netpoll.c +0 -3
@@ -353,11 +353,8 @@ static void arp_reply(struct sk_buff *skstruct sk_buff *send_skb;struct netpoll *np =NULL ; -spin_lock_irqsave (&npinfo->rx_lock, flags);if (npinfo->rx_np && npinfo->rx_np->dev == skb->dev) np = npinfo->rx_np; -spin_unlock_irqrestore (&npinfo->rx_lock, flags); -if (!np)return ;
PATCH 3/8 netpoll: e1000 netpoll tweak
Suggested by Steven Rostedt, matches his patch included in e100.
drivers/net/e1000/e1000_main.c +1 -0
@@ -3789,6 +3789,7 @@ e1000_netpoll(struct net_device *netdev)struct e1000_adapter *adapter =netdev_priv (netdev);disable_irq (adapter->pdev->irq);e1000_intr (adapter->pdev->irq, netdev,NULL ); +e1000_clean_tx_irq (adapter); enable_irq(adapter->pdev->irq); }#endif
PATCH 4/8 netpoll: netpoll_send_skb simplify
Minor netpoll_send_skb restructuring
Restructure to avoid confusing goto and move some bits out of the retry loop.
net/core/netpoll.c +22 -20
@@ -248,14 +248,14 @@ static void netpoll_send_skb(struct netpint status;struct netpoll_info *npinfo; -repeat: -if (!np || !np->dev || !netif_running (np->dev)) { +if (!np || !np->dev || !netif_running (np->dev)) {__kfree_skb (skb);return ; } -/* avoid recursion */ npinfo = np->dev->npinfo; + +/* avoid recursion */ if (npinfo->poll_owner ==smp_processor_id () || np->dev->xmit_lock_owner ==smp_processor_id ()) {if (np->drop)
@@ -265,29 +265,31 @@ repeat:return ; } -spin_lock (&np->dev->xmit_lock); - np->dev->xmit_lock_owner =smp_processor_id (); +while (1 ) { +spin_lock (&np->dev->xmit_lock); + np->dev->xmit_lock_owner =smp_processor_id (); + +/* +* network drivers do not expect to be called if the queue is +* stopped. +*/ +if (netif_queue_stopped (np->dev)) { + np->dev->xmit_lock_owner =-1 ; +spin_unlock (&np->dev->xmit_lock); +netpoll_poll (np); +continue ; + } -/* -* network drivers do not expect to be called if the queue is -* stopped. -*/ -if (netif_queue_stopped (np->dev)) { + status = np->dev->hard_start_xmit (skb, np->dev); np->dev->xmit_lock_owner =-1 ;spin_unlock (&np->dev->xmit_lock); -netpoll_poll (np); - goto repeat; - } - - status = np->dev->hard_start_xmit (skb, np->dev); - np->dev->xmit_lock_owner =-1 ; -spin_unlock (&np->dev->xmit_lock); +/* success */ +if (!status) +return ; -/* transmit busy */ -if (status) { +/* transmit busy */ netpoll_poll (np); - goto repeat; } }
PATCH 5/8 netpoll: add retry timeout
Add limited retry logic to netpoll_send_skb
Each time we attempt to send, decrement our per-device retry counter. On every successful send, we reset the counter.
We delay 50us between attempts with up to 20000 retries for a total of 1 second. After we've exhausted our retries, subsequent failed attempts will try only once until reset by success.
net/core/netpoll.c +10 -3
@@ -33,6 +33,7 @@#define MAX_UDP_CHUNK 1460#define MAX_SKBS 32#define MAX_QUEUE_DEPTH (MAX_SKBS / 2) +#define MAX_RETRIES 20000static DEFINE_SPINLOCK (skb_list_lock);static int nr_skbs;
@@ -265,7 +266,8 @@ static void netpoll_send_skb(struct netpreturn ; } -while (1 ) { + do { + npinfo->tries--; spin_lock(&np->dev->xmit_lock); np->dev->xmit_lock_owner = smp_processor_id();
@@ -277,6 +279,7 @@ static void netpoll_send_skb(struct netp np->dev->xmit_lock_owner =-1 ;spin_unlock (&np->dev->xmit_lock);netpoll_poll (np); +udelay (50 );continue ; }
@@ -285,12 +288,15 @@ static void netpoll_send_skb(struct netpspin_unlock (&np->dev->xmit_lock);/* success */ -if (!status) +if (!status) { + npinfo->tries =MAX_RETRIES ;/* reset */ return ; + }/* transmit busy */ netpoll_poll (np); - } +udelay (50 ); + }while (npinfo->tries >0 ); }void netpoll_send_udp (struct netpoll *np,const char *msg,int len)
@@ -642,6 +648,7 @@ int netpoll_setup(struct netpoll *np) npinfo->rx_np =NULL ; npinfo->poll_lock =SPIN_LOCK_UNLOCKED ; npinfo->poll_owner =-1 ; + npinfo->tries =MAX_RETRIES ; npinfo->rx_lock =SPIN_LOCK_UNLOCKED ; }else npinfo = ndev->npinfo;
include/linux/netpoll.h +1 -0
@@ -26,6 +26,7 @@ struct netpoll {struct netpoll_info {spinlock_t poll_lock;int poll_owner; +int tries;int rx_flags;spinlock_t rx_lock;struct netpoll *rx_np;/* netpoll that registered an rx_hook */
PATCH 6/8 netpoll: pre-fill skb pool
we could do one thing (see the patch below): i think it would be useful to fill up the netlogging skb queue straight at initialization time. Especially if netpoll is used for dumping alone, the system might not be in a situation to fill up the queue at the point of crash, so better be a bit more prepared and keep the pipeline filled.
Ingo
I've modified this to be called earlier - mpm
net/core/netpoll.c +4 -0
@@ -724,6 +724,10 @@ int netpoll_setup(struct netpoll *np) npinfo->rx_np = np;spin_unlock_irqrestore (&npinfo->rx_lock, flags); } + +/* fill up the skb queue */ +refill_skbs (); +/* last thing to do is link it to the net device structure */ ndev->npinfo = npinfo;
PATCH 7/8 netpoll: fix initialization/NAPI race
This fixes a race during initialization with the NAPI softirq processing by using an RCU approach.
This race was discovered when refill_skbs() was added to the setup code.
net/core/netpoll.c +3 -0
@@ -731,6 +731,9 @@ int netpoll_setup(struct netpoll *np)/* last thing to do is link it to the net device structure */ ndev->npinfo = npinfo; +/* avoid racing with NAPI reading npinfo */ +synchronize_rcu (); +return 0 ; release:
include/linux/netpoll.h +13 -6
@@ -9,6 +9,7 @@#include <linux/netdevice.h> #include <linux/interrupt.h> +#include <linux/rcupdate.h> #include <linux/list.h> struct netpoll ;
@@ -61,25 +62,31 @@ static inline int netpoll_rx(struct sk_b return ret; } -static inline void netpoll_poll_lock (struct net_device *dev) +static inline void *netpoll_poll_lock (struct net_device *dev) { +rcu_read_lock ();/* deal with race on ->npinfo */ if (dev->npinfo) {spin_lock (&dev->npinfo->poll_lock); dev->npinfo->poll_owner =smp_processor_id (); +return dev->npinfo; } +return NULL ; } -static inline void netpoll_poll_unlock (struct net_device *dev) +static inline void netpoll_poll_unlock (void *have) { -if (dev->npinfo) { - dev->npinfo->poll_owner =-1 ; -spin_unlock (&dev->npinfo->poll_lock); +struct netpoll_info *npi = have; + +if (npi) { + npi->poll_owner =-1 ; +spin_unlock (&npi->poll_lock); } +rcu_read_unlock (); }#else #define netpoll_rx (a) 0 -#define netpoll_poll_lock (a) +#define netpoll_poll_lock (a) 0#define netpoll_poll_unlock (a)#endif
net/core/dev.c +5 -4
@@ -1696,7 +1696,8 @@ static void net_rx_action(struct softirqstruct softnet_data *queue = &__get_cpu_var (softnet_data);unsigned long start_time = jiffies;int budget = netdev_budget; - +void *have; +local_irq_disable ();while (!list_empty (&queue->poll_list)) {
@@ -1709,10 +1710,10 @@ static void net_rx_action(struct softirq dev =list_entry (queue->poll_list.next, struct net_device, poll_list); -netpoll_poll_lock (dev); + have =netpoll_poll_lock (dev);if (dev->quota <=0 || dev->poll (dev, &budget)) { -netpoll_poll_unlock (dev); +netpoll_poll_unlock (have);local_irq_disable ();list_del (&dev->poll_list);list_add_tail (&dev->poll_list, &queue->poll_list);
@@ -1721,7 +1722,7 @@ static void net_rx_action(struct softirqelse dev->quota = dev->weight; }else { -netpoll_poll_unlock (dev); +netpoll_poll_unlock (have);dev_put (dev);local_irq_disable (); }
PATCH 8/8 netpoll: remove unused variable
Remove unused variable
net/core/netpoll.c +0 -1
@@ -356,7 +356,6 @@ static void arp_reply(struct sk_buff *skunsigned char *arp_ptr;int size, type =ARPOP_REPLY , ptype =ETH_P_ARP ;u32 sip, tip; -unsigned long flags;struct sk_buff *send_skb;struct netpoll *np =NULL ;
Re: [PATCH 3/8] netpoll: e1000 netpoll tweak
In reply to Matt MackallSorry this reply was to go to the whole list but only made it to Matt.
The e1000_intr() routine already calls e1000_clean_tx_irq(). So what's the point of this patch? Am I missing something?
Quoted 18 lines · Matt Mackall
Suggested by Steven Rostedt, matches his patch included in e100.
Signed-off-by Matt Mackall <[email protected]>Index:
l/drivers/net/e1000/e1000_main.c
===================================================================drivers/net/e1000/e1000_main.c 2005-08-06 17:55:01.000000000 -0500 renamed from drivers/net/e1000/e1000_main.c 2005-08-06 17:36:32.000000000 -0500 +1
-0@@ -3789,6 +3789,7 @@ e1000_netpoll(struct net_device *netdev) struct e1000_adapter *adapter = netdev_priv(netdev); disable_irq(adapter->pdev->irq); e1000_intr(adapter->pdev->irq, netdev, NULL); + e1000_clean_tx_irq(adapter); enable_irq(adapter->pdev->irq); } #endif
Signature · 2 lines
Cheers, John
Re: [PATCH 3/8] netpoll: e1000 netpoll tweak
In reply to Matt MackallSorry this reply was to go to the whole list but only made it to Matt.
The e1000_intr() routine already calls e1000_clean_tx_irq(). So what's the point of this patch? Am I missing something?
Quoted 18 lines · Matt Mackall
Suggested by Steven Rostedt, matches his patch included in e100.
Signed-off-by Matt Mackall <[email protected]>Index:
l/drivers/net/e1000/e1000_main.c
===================================================================drivers/net/e1000/e1000_main.c 2005-08-06 17:55:01.000000000 -0500 renamed from drivers/net/e1000/e1000_main.c 2005-08-06 17:36:32.000000000 -0500 +1
-0@@ -3789,6 +3789,7 @@ e1000_netpoll(struct net_device *netdev) struct e1000_adapter *adapter = netdev_priv(netdev); disable_irq(adapter->pdev->irq); e1000_intr(adapter->pdev->irq, netdev, NULL); + e1000_clean_tx_irq(adapter); enable_irq(adapter->pdev->irq); } #endif
Signature · 2 lines
Cheers, John
David S. Miller
maintainerRe: [PATCH 3/8] netpoll: e1000 netpoll tweak
From: John Ronciak <[email protected]>
Subject: Re: [PATCH 3/8] netpoll: e1000 netpoll tweak
Date: Fri, 12 Aug 2005 12:02:03 -0700
Quoted 4 lines
Sorry this reply was to go to the whole list but only made it to Matt.
The
e1000_intr()routine already callse1000_clean_tx_irq(). So what's the point of this patch? Am I missing something?
e1000_intr() does not call e1000_clean_tx_irq() when NAPI is enabled.
Re: [PATCH 3/8] netpoll: e1000 netpoll tweak
In reply to John Ronciak[corrected akpm's address]
Quoted 4 lines · John Ronciak
Sorry this reply was to go to the whole list but only made it to Matt.
The
e1000_intr()routine already callse1000_clean_tx_irq(). So what's the point of this patch? Am I missing something?
Here is Steven's original analysis:
http://lkml.org/lkml/2005/8/5/116
It looked plausible, but I didn't dig much deeper.
Quoted 12 lines
Quoted 12 lines
Index:
l/drivers/net/e1000/e1000_main.c
===================================================================drivers/net/e1000/e1000_main.c 2005-08-06 17:55:01.000000000 -0500 renamed from drivers/net/e1000/e1000_main.c 2005-08-06 17:36:32.000000000 -0500 +1
-0@@ -3789,6 +3789,7 @@ e1000_netpoll(struct net_device *netdev) struct e1000_adapter *adapter = netdev_priv(netdev); disable_irq(adapter->pdev->irq); e1000_intr(adapter->pdev->irq, netdev, NULL); + e1000_clean_tx_irq(adapter); enable_irq(adapter->pdev->irq); } #endif
Signature · 1 line
Mathematics is the supreme nostalgia of our time.
Re: [PATCH 3/8] netpoll: e1000 netpoll tweak
In reply to John Ronciak[corrected akpm's address]
Quoted 4 lines · John Ronciak
Sorry this reply was to go to the whole list but only made it to Matt.
The
e1000_intr()routine already callse1000_clean_tx_irq(). So what's the point of this patch? Am I missing something?
Here is Steven's original analysis:
http://lkml.org/lkml/2005/8/5/116
It looked plausible, but I didn't dig much deeper.
Quoted 12 lines
Quoted 12 lines
Index:
l/drivers/net/e1000/e1000_main.c
===================================================================drivers/net/e1000/e1000_main.c 2005-08-06 17:55:01.000000000 -0500 renamed from drivers/net/e1000/e1000_main.c 2005-08-06 17:36:32.000000000 -0500 +1
-0@@ -3789,6 +3789,7 @@ e1000_netpoll(struct net_device *netdev) struct e1000_adapter *adapter = netdev_priv(netdev); disable_irq(adapter->pdev->irq); e1000_intr(adapter->pdev->irq, netdev, NULL); + e1000_clean_tx_irq(adapter); enable_irq(adapter->pdev->irq); } #endif
Signature · 1 line
Mathematics is the supreme nostalgia of our time.
David S. Miller
maintainerFrom: Matt Mackall <[email protected]>
Date: Thu, 11 Aug 2005 21:18:28 -0500
Quoted 9 lines
This patch series cleans up a few outstanding bugs in netpoll:
- two bugfixes from Jeff Moyer's netpoll bonding
- a tweak to e1000's netpoll stub
- timeout handling for e1000 with carrier loss
- prefilling SKBs at init
- a fix-up for a race discovered in initialization
- an unused variable warning
Quoted 3 lines
This patch set was tested over repeated rebooting with both tg3 and e1000 and random cable disconnection, with and without SMP and preempt. Please apply.
All applied, thanks a lot for putting this patch set together.
I'll push this to Linus after some smoke testing.
Quoted 8 lines · Matt Mackall
This patch series cleans up a few outstanding bugs in netpoll:
- two bugfixes from Jeff Moyer's netpoll bonding
- a tweak to e1000's netpoll stub
- timeout handling for e1000 with carrier loss
- prefilling SKBs at init
- a fix-up for a race discovered in initialization
- an unused variable warning
Matt, I have tested them, the sender doesnt lockup anymore. But a task dump doesnt work, I get only the first task. This is on a 3GHz xeon with tg3 card.
Quoted 8 lines · Matt Mackall
This patch series cleans up a few outstanding bugs in netpoll:
- two bugfixes from Jeff Moyer's netpoll bonding
- a tweak to e1000's netpoll stub
- timeout handling for e1000 with carrier loss
- prefilling SKBs at init
- a fix-up for a race discovered in initialization
- an unused variable warning
Matt, I have tested them, the sender doesnt lockup anymore. But a task dump doesnt work, I get only the first task. This is on a 3GHz xeon with tg3 card.
[corrected akpm's address]
Quoted 11 lines · Olaf Hering
Quoted 8 lines · Matt Mackall
This patch series cleans up a few outstanding bugs in netpoll:
- two bugfixes from Jeff Moyer's netpoll bonding
- a tweak to e1000's netpoll stub
- timeout handling for e1000 with carrier loss
- prefilling SKBs at init
- a fix-up for a race discovered in initialization
- an unused variable warning
Quoted 3 lines
Matt, I have tested them, the sender doesnt lockup anymore. But a task dump doesnt work, I get only the first task. This is on a 3GHz xeon with tg3 card.
Does the task dump work without patch 5/8 (add retry timeout)? I'll try testing it here.
Signature · 1 line
Mathematics is the supreme nostalgia of our time.
Quoted 2 lines · Matt Mackall
Does the task dump work without patch 5/8 (add retry timeout)? I'll try testing it here.
I spoke to soon, worked once, after reboot not anymore. Will try to play with individual patches. Does the task dump work for you, at least?
Quoted 2 lines · Matt Mackall
Does the task dump work without patch 5/8 (add retry timeout)? I'll try testing it here.
I spoke to soon, worked once, after reboot not anymore. Will try to play with individual patches. Does the task dump work for you, at least?
Quoted 7 lines · Olaf Hering
Quoted 2 lines · Matt Mackall
Does the task dump work without patch 5/8 (add retry timeout)? I'll try testing it here.
I spoke to soon, worked once, after reboot not anymore. Will try to play with individual patches. Does the task dump work for you, at least?
Works flawlessly on e1000. Works on tg3 with serial console, but seems to cause trouble without. Haven't had time to dig deeper yet.
Signature · 1 line
Mathematics is the supreme nostalgia of our time.
Quoted 11 lines · Matt Mackall
Quoted 7 lines · Olaf Hering
Quoted 2 lines · Matt Mackall
Does the task dump work without patch 5/8 (add retry timeout)? I'll try testing it here.
I spoke to soon, worked once, after reboot not anymore. Will try to play with individual patches. Does the task dump work for you, at least?
Works flawlessly on e1000. Works on tg3 with serial console, but seems to cause trouble without. Haven't had time to dig deeper yet.
Can you send me your .config off-list?
I'm using ftp.suse.com/pub/projects/kernel/kotd/i386/HEAD/kernel-smp.i586.rpm
will check if that nmi_watchdog thing shows anything.