[PATCH 0/8] netpoll: various bugfixes

[PATCH 0/8] netpoll: various bugfixes

[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

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

[Matt Mackall]

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.

Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
Signed-off-by: Matt Mackall <mpm@selenic.com>

--- linux-2.6.12/net/core/netpoll.c.orig	2005-07-01 14:02:56.039174635 -0400
+++ linux-2.6.12/net/core/netpoll.c	2005-07-01 14:03:16.688739508 -0400
@@ -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

[Matt Mackall]

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.

Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
Signed-off-by: Matt Mackall <mpm@selenic.com>

Index: l/net/core/netpoll.c
===================================================================
--- l.orig/net/core/netpoll.c	2005-08-06 17:47:48.000000000 -0500
+++ l/net/core/netpoll.c	2005-08-06 17:47:49.000000000 -0500
@@ -353,11 +353,8 @@ static void arp_reply(struct sk_buff *sk
 	struct 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

[Matt Mackall]

Suggested by Steven Rostedt, matches his patch included in e100.

Signed-off-by: Matt Mackall <mpm@selenic.com>

Index: l/drivers/net/e1000/e1000_main.c
===================================================================
--- l.orig/drivers/net/e1000/e1000_main.c	2005-08-06 17:36:32.000000000 -0500
+++ l/drivers/net/e1000/e1000_main.c	2005-08-06 17:55:01.000000000 -0500
@@ -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

[Matt Mackall]

Minor netpoll_send_skb restructuring

Restructure to avoid confusing goto and move some bits out of the
retry loop.

Signed-off-by: Matt Mackall <mpm@selenic.com>

Index: l/net/core/netpoll.c
===================================================================
--- l.orig/net/core/netpoll.c	2005-08-07 15:15:48.000000000 -0500
+++ l/net/core/netpoll.c	2005-08-07 16:59:27.000000000 -0500
@@ -248,14 +248,14 @@ static void netpoll_send_skb(struct netp
 	int 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

[Matt Mackall]

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.

Signed-off-by: Matt Mackall <mpm@selenic.com>

Index: lhg/net/core/netpoll.c
===================================================================
--- lhg.orig/net/core/netpoll.c	2005-08-11 16:17:45.000000000 -0700
+++ lhg/net/core/netpoll.c	2005-08-11 16:45:20.000000000 -0700
@@ -33,6 +33,7 @@
 #define MAX_UDP_CHUNK 1460
 #define MAX_SKBS 32
 #define MAX_QUEUE_DEPTH (MAX_SKBS / 2)
+#define MAX_RETRIES 20000
 
 static DEFINE_SPINLOCK(skb_list_lock);
 static int nr_skbs;
@@ -265,7 +266,8 @@ static void netpoll_send_skb(struct netp
 		return;
 	}
 
-	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 netp
 		spin_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;
Index: lhg/include/linux/netpoll.h
===================================================================
--- lhg.orig/include/linux/netpoll.h	2005-08-11 15:40:27.000000000 -0700
+++ lhg/include/linux/netpoll.h	2005-08-11 16:17:56.000000000 -0700
@@ -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

[Matt Mackall]

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

Signed-off-by: Ingo Molnar <mingo@elte.hu>

I've modified this to be called earlier - mpm

Signed-off-by: Matt Mackall <mpm@selenic.com>

Index: l/net/core/netpoll.c
===================================================================
--- l.orig/net/core/netpoll.c	2005-08-08 23:00:48.000000000 -0500
+++ l/net/core/netpoll.c	2005-08-11 01:50:31.000000000 -0500
@@ -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

[Matt Mackall]

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.

Signed-off-by: Matt Mackall <mpm@selenic.com>

Index: l/net/core/netpoll.c
===================================================================
--- l.orig/net/core/netpoll.c	2005-08-09 00:56:23.000000000 -0500
+++ l/net/core/netpoll.c	2005-08-11 01:50:24.000000000 -0500
@@ -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:
Index: l/include/linux/netpoll.h
===================================================================
--- l.orig/include/linux/netpoll.h	2005-08-09 00:56:23.000000000 -0500
+++ l/include/linux/netpoll.h	2005-08-11 01:33:42.000000000 -0500
@@ -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
 
Index: l/net/core/dev.c
===================================================================
--- l.orig/net/core/dev.c	2005-08-09 00:56:23.000000000 -0500
+++ l/net/core/dev.c	2005-08-11 01:34:08.000000000 -0500
@@ -1696,7 +1696,8 @@ static void net_rx_action(struct softirq
 	struct 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 softirq
 			else
 				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

[Matt Mackall]

Remove unused variable

Signed-off-by: Matt Mackall <mpm@selenic.com>

Index: l/net/core/netpoll.c
===================================================================
--- l.orig/net/core/netpoll.c	2005-08-11 01:32:01.000000000 -0500
+++ l/net/core/netpoll.c	2005-08-11 01:49:37.000000000 -0500
@@ -356,7 +356,6 @@ static void arp_reply(struct sk_buff *sk
 	unsigned 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

[John Ronciak]

Sorry 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?

On 8/11/05, Matt Mackall <mpm@selenic.com> wrote:
> Suggested by Steven Rostedt, matches his patch included in e100.
> 
> Signed-off-by: Matt Mackall <mpm@selenic.com>
> 
> Index: l/drivers/net/e1000/e1000_main.c
> ===================================================================
> --- l.orig/drivers/net/e1000/e1000_main.c       2005-08-06 17:36:32.000000000 -0500
> +++ l/drivers/net/e1000/e1000_main.c    2005-08-06 17:55:01.000000000 -0500
> @@ -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
> 
> 


-- 
Cheers,
John

Re: [PATCH 3/8] netpoll: e1000 netpoll tweak

[David S. Miller]

From: John Ronciak <john.ronciak@gmail.com>
Subject: Re: [PATCH 3/8] netpoll: e1000 netpoll tweak
Date: Fri, 12 Aug 2005 12:02:03 -0700

> Sorry 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?

e1000_intr() does not call e1000_clean_tx_irq() when NAPI
is enabled.

Re: [PATCH 3/8] netpoll: e1000 netpoll tweak

[Matt Mackall]

[corrected akpm's address]

On Fri, Aug 12, 2005 at 12:02:03PM -0700, John Ronciak wrote:
> Sorry 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?

Here is Steven's original analysis:

http://lkml.org/lkml/2005/8/5/116

It looked plausible, but I didn't dig much deeper.

> > Index: l/drivers/net/e1000/e1000_main.c
> > ===================================================================
> > --- l.orig/drivers/net/e1000/e1000_main.c       2005-08-06 17:36:32.000000000 -0500
> > +++ l/drivers/net/e1000/e1000_main.c    2005-08-06 17:55:01.000000000 -0500
> > @@ -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

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH 0/8] netpoll: various bugfixes

[David S. Miller]

From: Matt Mackall <mpm@selenic.com>
Date: Thu, 11 Aug 2005 21:18:28 -0500

> 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.

All applied, thanks a lot for putting this patch set together.

I'll push this to Linus after some smoke testing.

Re: [PATCH 0/8] netpoll: various bugfixes

[Olaf Hering]

 On Thu, Aug 11, Matt Mackall wrote:

> 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.

Re: [PATCH 0/8] netpoll: various bugfixes

[Matt Mackall]

[corrected akpm's address]

On Fri, Aug 12, 2005 at 07:21:51PM +0200, Olaf Hering wrote:
>  On Thu, Aug 11, Matt Mackall wrote:
> 
> > 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.

Does the task dump work without patch 5/8 (add retry timeout)? I'll
try testing it here.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH 0/8] netpoll: various bugfixes

[Olaf Hering]

 On Fri, Aug 12, Matt Mackall wrote:

> 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?

Re: [PATCH 0/8] netpoll: various bugfixes

[Matt Mackall]

On Fri, Aug 12, 2005 at 09:31:09PM +0200, Olaf Hering wrote:
>  On Fri, Aug 12, Matt Mackall wrote:
> 
> > 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.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH 0/8] netpoll: various bugfixes

[Olaf Hering]

 On Sun, Aug 14, Matt Mackall wrote:

> On Fri, Aug 12, 2005 at 09:31:09PM +0200, Olaf Hering wrote:
> >  On Fri, Aug 12, Matt Mackall wrote:
> > 
> > > 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.