[PATCH 0/8] netpoll: various bugfixes

STALE7678d

12 messages, 4 authors, 2005-08-12 · open the first message on its own page

[PATCH 0/8] netpoll: various bugfixes

From: Matt Mackall <hidden>
Date: 2005-08-12 02:20:58

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 8/8] netpoll: remove unused variable

From: Matt Mackall <hidden>
Date: 2005-08-12 02:19:46

Remove unused variable

Signed-off-by: Matt Mackall <redacted>

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;

[PATCH 7/8] netpoll: fix initialization/NAPI race

From: Matt Mackall <hidden>
Date: 2005-08-12 02:19:46

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

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 5/8] netpoll: add retry timeout

From: Matt Mackall <hidden>
Date: 2005-08-12 02:20:13

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

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

From: Matt Mackall <hidden>
Date: 2005-08-12 02:20:15

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

I've modified this to be called earlier - mpm

Signed-off-by: Matt Mackall <redacted>

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 4/8] netpoll: netpoll_send_skb simplify

From: Matt Mackall <hidden>
Date: 2005-08-12 02:20:58

Minor netpoll_send_skb restructuring

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

Signed-off-by: Matt Mackall <redacted>

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 3/8] netpoll: e1000 netpoll tweak

From: Matt Mackall <hidden>
Date: 2005-08-12 02:20:58

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

Signed-off-by: Matt Mackall <redacted>

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 2/8] netpoll: deadlock bugfix

From: Matt Mackall <hidden>
Date: 2005-08-12 02:21:48

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 <redacted>
Signed-off-by: Matt Mackall <redacted>

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 1/8] netpoll: rx_flags bugfix

From: Matt Mackall <hidden>
Date: 2005-08-12 02:21:48

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 <redacted>
Signed-off-by: Matt Mackall <redacted>

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

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

From: "David S. Miller" <davem@davemloft.net>
Date: 2005-08-12 02:42:17

From: Matt Mackall <redacted>
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

From: Olaf Hering <hidden>
Date: 2005-08-12 17:21:51

 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 3/8] netpoll: e1000 netpoll tweak

From: John Ronciak <hidden>
Date: 2005-08-12 19:02:03

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 [off-list ref] wrote:

quoted hunk

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

Signed-off-by: Matt Mackall <redacted>

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help