Skip to contentSkip to conversation

xen-netfront: use napi_complete() correctly to prevent Rx stalling

PATCH Applied David Vrabel 6 participants 45 messages Dec 16, 2014 – Dec 24, 2014

Subsystems (4)
Networking Network drivers Xen hypervisor interface virtio net driver
lore
Share & tools
lore

David Vrabel

author
#0 lore raw

PATCH 1/1 xen-netfront: use napi_complete() correctly to prevent Rx stalling

After d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) the napi instance is removed from the per-cpu list prior to calling the n->poll(), and is only requeued if all of the budget was used. This inadvertently broke netfront because netfront does not use NAPI correctly.

If netfront had not used all of its budget it would do a final check for any Rx responses and avoid calling napi_complete() if there were more responses. It would still return under budget so it would never be rescheduled. The final check would also not re-enable the Rx interrupt.

Additionally, xenvif_poll() would also call napi_complete() /after/ enabling the interrupt. This resulted in a race between the napi_complete() and the napi_schedule() in the interrupt handler. The use of local_irq_save/restore() avoided by race iff the handler is running on the same CPU but not if it was running on a different CPU.

Fix both of these by always calling napi_compete() if the budget was not all used, and then calling napi_schedule() if the final checks says there's more work.

Signed-off-by David Vrabel <[email protected]>
Cc Eric Dumazet <[email protected]>
---
 drivers/net/xen-netfront.c |   11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)
drivers/net/xen-netfront.c +3 -8
index 2f0a9ce..22bcb4e 100644
@@ -977,7 +977,6 @@ static int xennet_poll(struct napi_struct *napi, int budget)
 	struct sk_buff_head rxq;
 	struct sk_buff_head errq;
 	struct sk_buff_head tmpq;
-	unsigned long flags;
 	int err;
 
 	spin_lock(&queue->rx_lock);

@@ -1050,15 +1049,11 @@ err:
 	if (work_done < budget) {
 		int more_to_do = 0;
 
-		napi_gro_flush(napi, false);
-
-		local_irq_save(flags);
+		napi_complete(napi);
 
 		RING_FINAL_CHECK_FOR_RESPONSES(&queue->rx, more_to_do);
-		if (!more_to_do)
-			__napi_complete(napi);
-
-		local_irq_restore(flags);
+		if (more_to_do)
+			napi_schedule(napi);
 	}
 
 	spin_unlock(&queue->rx_lock);
Signature · 1 line
1.7.10.4
maintainer Applied
#1 lore raw

From: David Vrabel <[email protected]>
Date: Tue, 16 Dec 2014 18:59:46 +0000

Quoted 18 lines

After d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) the napi instance is removed from the per-cpu list prior to calling the n->poll(), and is only requeued if all of the budget was used. This inadvertently broke netfront because netfront does not use NAPI correctly.

If netfront had not used all of its budget it would do a final check for any Rx responses and avoid calling napi_complete() if there were more responses. It would still return under budget so it would never be rescheduled. The final check would also not re-enable the Rx interrupt.

Additionally, xenvif_poll() would also call napi_complete() /after/ enabling the interrupt. This resulted in a race between the napi_complete() and the napi_schedule() in the interrupt handler. The use of local_irq_save/restore() avoided by race iff the handler is running on the same CPU but not if it was running on a different CPU.

Quoted 5 lines

Fix both of these by always calling napi_compete() if the budget was not all used, and then calling napi_schedule() if the final checks says there's more work.

Signed-off-by David Vrabel <[email protected]>

Applied, thanks.

maintainer Applied
#2 lore raw
In reply to David Vrabel

From: David Vrabel <[email protected]>
Date: Tue, 16 Dec 2014 18:59:46 +0000

Quoted 18 lines

After d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) the napi instance is removed from the per-cpu list prior to calling the n->poll(), and is only requeued if all of the budget was used. This inadvertently broke netfront because netfront does not use NAPI correctly.

If netfront had not used all of its budget it would do a final check for any Rx responses and avoid calling napi_complete() if there were more responses. It would still return under budget so it would never be rescheduled. The final check would also not re-enable the Rx interrupt.

Additionally, xenvif_poll() would also call napi_complete() /after/ enabling the interrupt. This resulted in a race between the napi_complete() and the napi_schedule() in the interrupt handler. The use of local_irq_save/restore() avoided by race iff the handler is running on the same CPU but not if it was running on a different CPU.

Quoted 5 lines

Fix both of these by always calling napi_compete() if the budget was not all used, and then calling napi_schedule() if the final checks says there's more work.

Signed-off-by David Vrabel <[email protected]>

Applied, thanks.

#3 lore raw

virtio_net: Fix napi poll list corruption

In reply to David Vrabel
Quoted 5 lines · David Vrabel

After d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) the napi instance is removed from the per-cpu list prior to calling the n->poll(), and is only requeued if all of the budget was used. This inadvertently broke netfront because netfront does not use NAPI correctly.

A similar bug exists in virtio_net.

-- >8 --
The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) breaks virtio_net in an insidious way.

It is now required that if the entire budget is consumed when poll returns, the napi poll_list must remain empty. However, like some other drivers virtio_net tries to do a last-ditch check and if there is more work it will call napi_schedule and then immediately process some of this new work. Should the entire budget be consumed while processing such new work then we will violate the new caller contract.

This patch fixes this by not touching any work when we reschedule in virtio_net.

The worst part of this bug is that the list corruption causes other napi users to be moved off-list. In my case I was chasing a stall in IPsec (IPsec uses netif_rx) and I only belatedly realised that it was virtio_net which caused the stall even though the virtio_net poll was still functioning perfectly after IPsec stalled.

Signed-off-by Herbert Xu <[email protected]>
drivers/net/virtio_net.c +0 -2
index b8bd719..5ca9771 100644
@@ -760,7 +760,6 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
 		container_of(napi, struct receive_queue, napi);
 	unsigned int r, received = 0;
 
-again:
 	received += virtnet_receive(rq, budget - received);
 
 	/* Out of packets? */

@@ -771,7 +770,6 @@ again:
 		    napi_schedule_prep(napi)) {
 			virtqueue_disable_cb(rq->vq);
 			__napi_schedule(napi);
-			goto again;
 		}
 	}

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#4 lore raw

net: Detect drivers that reschedule NAPI and exhaust budget

Quoted 2 lines · Herbert Xu

A similar bug exists in virtio_net.

In order to detect other drivers doing this we should add something like this.

-- >8 --
The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) required drivers to leave poll_list empty if the entire budget is consumed.

We have already had two broken drivers so let's add a check for this.

Signed-off-by Herbert Xu <[email protected]>
net/core/dev.c +5 -1
index f411c28..88f9725 100644
@@ -4620,7 +4620,11 @@ static void net_rx_action(struct softirq_action *h)
 					 */
 					napi_gro_flush(n, HZ >= 1000);
 				}
-				list_add_tail(&n->poll_list, &repoll);
+				/* Some drivers may have called napi_schedule
+				 * prior to exhausting their budget.
+				 */
+				if (!WARN_ON_ONCE(!list_empty(&n->poll_list)))
+					list_add_tail(&n->poll_list, &repoll);
 			}
 		}

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#5 lore raw

Re: net: Detect drivers that reschedule NAPI and exhaust budget

Quoted 24 lines · Herbert Xu
Quoted 2 lines · Herbert Xu

A similar bug exists in virtio_net.

In order to detect other drivers doing this we should add something like this.

-- >8 --
The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) required drivers to leave poll_list empty if the entire budget is consumed.

We have already had two broken drivers so let's add a check for this.

Signed-off-by Herbert Xu <[email protected]>
net/core/dev.c +0 -0
index f411c28..88f9725 100644
@@ -4620,7 +4620,11 @@ static void net_rx_action(struct softirq_action *h)
 					 */
 					napi_gro_flush(n, HZ >= 1000);

net/core/dev.c static void net_rx_action(struct softirq_action *h) · line 4630

Quoted 10 lines
net/core/dev.c +5 -1
 				}
-				list_add_tail(&n->poll_list, &repoll);
+				/* Some drivers may have called napi_schedule
+				 * prior to exhausting their budget.
+				 */
+				if (!WARN_ON_ONCE(!list_empty(&n->poll_list)))
+					list_add_tail(&n->poll_list, &repoll);
 			}
 		}

I do not think stack trace will point to the buggy driver ?

IMO it would be better to print a single line with the netdev name ?

maintainer
#6 lore raw

Re: net: Detect drivers that reschedule NAPI and exhaust budget

From: Eric Dumazet <[email protected]>
Date: Fri, 19 Dec 2014 17:34:48 -0800

Quoted 16 lines

net/core/dev.c static void net_rx_action(struct softirq_action *h) · line 4630

Quoted 13 lines
net/core/dev.c +5 -1
@@ -4620,7 +4620,11 @@ static void net_rx_action(struct softirq_action *h)
 					 */
 					napi_gro_flush(n, HZ >= 1000);
 				}
-				list_add_tail(&n->poll_list, &repoll);
+				/* Some drivers may have called napi_schedule
+				 * prior to exhausting their budget.
+				 */
+				if (!WARN_ON_ONCE(!list_empty(&n->poll_list)))
+					list_add_tail(&n->poll_list, &repoll);
 			}
 		}

I do not think stack trace will point to the buggy driver ?

Quoted 1 line

IMO it would be better to print a single line with the netdev name ?

Right, we are already back from the poll routine and will just end up seeing the call trace leading to the software interrupt.

#7 lore raw

Re: net: Detect drivers that reschedule NAPI and exhaust budget

Quoted 19 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Fri, 19 Dec 2014 17:34:48 -0800

Quoted 16 lines

Review static void net_rx_action(struct softirq_action *h) · line 4630

Quoted 13 lines
(quoted diff) +5 -1
@@ -4620,7 +4620,11 @@ static void net_rx_action(struct softirq_action *h)
 					 */
 					napi_gro_flush(n, HZ >= 1000);
 				}
-				list_add_tail(&n->poll_list, &repoll);
+				/* Some drivers may have called napi_schedule
+				 * prior to exhausting their budget.
+				 */
+				if (!WARN_ON_ONCE(!list_empty(&n->poll_list)))
+					list_add_tail(&n->poll_list, &repoll);
 			}
 		}

I do not think stack trace will point to the buggy driver ?

Quoted 4 lines
Quoted 1 line

IMO it would be better to print a single line with the netdev name ?

Right, we are already back from the poll routine and will just end up seeing the call trace leading to the software interrupt.

Good point Eric.

-- >8 --
The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) required drivers to leave poll_list empty if the entire budget is consumed.

We have already had two broken drivers so let's add a check for this.

Signed-off-by Herbert Xu <[email protected]>
net/core/dev.c +7 -1
index f411c28..47fdc5c 100644
@@ -4620,7 +4620,13 @@ static void net_rx_action(struct softirq_action *h)
 					 */
 					napi_gro_flush(n, HZ >= 1000);
 				}
-				list_add_tail(&n->poll_list, &repoll);
+				/* Some drivers may have called napi_schedule
+				 * prior to exhausting their budget.
+				 */
+				if (unlikely(!list_empty(&n->poll_list)))
+					pr_warn("%s: Budget exhausted after napi rescheduled\n", n->dev ? n->dev->name : "backlog");
+				else
+					list_add_tail(&n->poll_list, &repoll);
 			}
 		}

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#8 lore raw

Re: net: Detect drivers that reschedule NAPI and exhaust budget

Quoted 29 lines · Herbert Xu

-- >8 --
The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) required drivers to leave poll_list empty if the entire budget is consumed.

We have already had two broken drivers so let's add a check for this.

Signed-off-by Herbert Xu <[email protected]>
net/core/dev.c +7 -1
index f411c28..47fdc5c 100644
@@ -4620,7 +4620,13 @@ static void net_rx_action(struct softirq_action *h)
 					 */
 					napi_gro_flush(n, HZ >= 1000);
 				}
-				list_add_tail(&n->poll_list, &repoll);
+				/* Some drivers may have called napi_schedule
+				 * prior to exhausting their budget.
+				 */
+				if (unlikely(!list_empty(&n->poll_list)))
+					pr_warn("%s: Budget exhausted after napi rescheduled\n", n->dev ? n->dev->name : "backlog");
+				else
+					list_add_tail(&n->poll_list, &repoll);
 			}
 		}
Quoted 1 line

Thanks,

OK, but could you :
1) use pr_warn_once()
2) split the too long line

	pr_warn_once("%s: Budget exhausted after napi rescheduled\n",
		     n->dev ? n->dev->name : "backlog");

Thanks Herbert !

#9 lore raw

cover net: net_rx_action fixes and clean-ups

Quoted 6 lines · Eric Dumazet

OK, but could you :
1) use pr_warn_once()
2) split the too long line

	pr_warn_once("%s: Budget exhausted after napi rescheduled\n",
		     n->dev ? n->dev->name : "backlog");

Sure, I'll clean it up a bit too.

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#10 lore raw

PATCH 1/4 net: Move napi polling code out of net_rx_action

This patch creates a new function napi_poll and moves the napi polling code from net_rx_action into it.

Signed-off-by Herbert Xu <[email protected]>

---

 net/core/dev.c |   98 +++++++++++++++++++++++++++++++--------------------------
 1 file changed, 54 insertions(+), 44 deletions(-)
net/core/dev.c +54 -44
index f411c28..f7c4f4e 100644
@@ -4557,6 +4557,59 @@ void netif_napi_del(struct napi_struct *napi)
 }
 EXPORT_SYMBOL(netif_napi_del);
 
+static int napi_poll(struct napi_struct *n, struct list_head *repoll)
+{
+	void *have;
+	int work, weight;
+
+	list_del_init(&n->poll_list);
+
+	have = netpoll_poll_lock(n);
+
+	weight = n->weight;
+
+	/* This NAPI_STATE_SCHED test is for avoiding a race
+	 * with netpoll's poll_napi().  Only the entity which
+	 * obtains the lock and sees NAPI_STATE_SCHED set will
+	 * actually make the ->poll() call.  Therefore we avoid
+	 * accidentally calling ->poll() when NAPI is not scheduled.
+	 */
+	work = 0;
+	if (test_bit(NAPI_STATE_SCHED, &n->state)) {
+		work = n->poll(n, weight);
+		trace_napi_poll(n);
+	}
+
+	WARN_ON_ONCE(work > weight);
+
+	if (likely(work < weight))
+		goto out_unlock;
+
+	/* Drivers must not modify the NAPI state if they
+	 * consume the entire weight.  In such cases this code
+	 * still "owns" the NAPI instance and therefore can
+	 * move the instance around on the list at-will.
+	 */
+	if (unlikely(napi_disable_pending(n))) {
+		napi_complete(n);
+		goto out_unlock;
+	}
+
+	if (n->gro_list) {
+		/* flush too old packets
+		 * If HZ < 1000, flush all packets.
+		 */
+		napi_gro_flush(n, HZ >= 1000);
+	}
+
+	list_add_tail(&n->poll_list, repoll);
+
+out_unlock:
+	netpoll_poll_unlock(have);
+
+	return work;
+}
+
 static void net_rx_action(struct softirq_action *h)
 {
 	struct softnet_data *sd = this_cpu_ptr(&softnet_data);

@@ -4564,7 +4617,6 @@ static void net_rx_action(struct softirq_action *h)
 	int budget = netdev_budget;
 	LIST_HEAD(list);
 	LIST_HEAD(repoll);
-	void *have;
 
 	local_irq_disable();
 	list_splice_init(&sd->poll_list, &list);

@@ -4572,7 +4624,6 @@ static void net_rx_action(struct softirq_action *h)
 
 	while (!list_empty(&list)) {
 		struct napi_struct *n;
-		int work, weight;
 
 		/* If softirq window is exhausted then punt.
 		 * Allow this to run for 2 jiffies since which will allow

@@ -4583,48 +4634,7 @@ static void net_rx_action(struct softirq_action *h)
 
 
 		n = list_first_entry(&list, struct napi_struct, poll_list);
-		list_del_init(&n->poll_list);
-
-		have = netpoll_poll_lock(n);
-
-		weight = n->weight;
-
-		/* This NAPI_STATE_SCHED test is for avoiding a race
-		 * with netpoll's poll_napi().  Only the entity which
-		 * obtains the lock and sees NAPI_STATE_SCHED set will
-		 * actually make the ->poll() call.  Therefore we avoid
-		 * accidentally calling ->poll() when NAPI is not scheduled.
-		 */
-		work = 0;
-		if (test_bit(NAPI_STATE_SCHED, &n->state)) {
-			work = n->poll(n, weight);
-			trace_napi_poll(n);
-		}
-
-		WARN_ON_ONCE(work > weight);
-
-		budget -= work;
-
-		/* Drivers must not modify the NAPI state if they
-		 * consume the entire weight.  In such cases this code
-		 * still "owns" the NAPI instance and therefore can
-		 * move the instance around on the list at-will.
-		 */
-		if (unlikely(work == weight)) {
-			if (unlikely(napi_disable_pending(n))) {
-				napi_complete(n);
-			} else {
-				if (n->gro_list) {
-					/* flush too old packets
-					 * If HZ < 1000, flush all packets.
-					 */
-					napi_gro_flush(n, HZ >= 1000);
-				}
-				list_add_tail(&n->poll_list, &repoll);
-			}
-		}
-
-		netpoll_poll_unlock(have);
+		budget -= napi_poll(n, &repoll);
 	}
 
 	if (!sd_has_rps_ipi_waiting(sd) &&
maintainer Applied
#11 lore raw

Re: [PATCH 1/4] net: Move napi polling code out of net_rx_action

From: Herbert Xu <[email protected]>
Date: Sun, 21 Dec 2014 07:16:21 +1100

Quoted 4 lines

This patch creates a new function napi_poll and moves the napi polling code from net_rx_action into it.

Signed-off-by Herbert Xu <[email protected]>

Applied.

#13 lore raw

PATCH 1/4 net: Move napi polling code out of net_rx_action

In reply to Herbert Xu

This patch creates a new function napi_poll and moves the napi polling code from net_rx_action into it.

Signed-off-by Herbert Xu <[email protected]>

---

 net/core/dev.c |   98 +++++++++++++++++++++++++++++++--------------------------
 1 file changed, 54 insertions(+), 44 deletions(-)
net/core/dev.c +54 -44
index f411c28..f7c4f4e 100644
@@ -4557,6 +4557,59 @@ void netif_napi_del(struct napi_struct *napi)
 }
 EXPORT_SYMBOL(netif_napi_del);
 
+static int napi_poll(struct napi_struct *n, struct list_head *repoll)
+{
+	void *have;
+	int work, weight;
+
+	list_del_init(&n->poll_list);
+
+	have = netpoll_poll_lock(n);
+
+	weight = n->weight;
+
+	/* This NAPI_STATE_SCHED test is for avoiding a race
+	 * with netpoll's poll_napi().  Only the entity which
+	 * obtains the lock and sees NAPI_STATE_SCHED set will
+	 * actually make the ->poll() call.  Therefore we avoid
+	 * accidentally calling ->poll() when NAPI is not scheduled.
+	 */
+	work = 0;
+	if (test_bit(NAPI_STATE_SCHED, &n->state)) {
+		work = n->poll(n, weight);
+		trace_napi_poll(n);
+	}
+
+	WARN_ON_ONCE(work > weight);
+
+	if (likely(work < weight))
+		goto out_unlock;
+
+	/* Drivers must not modify the NAPI state if they
+	 * consume the entire weight.  In such cases this code
+	 * still "owns" the NAPI instance and therefore can
+	 * move the instance around on the list at-will.
+	 */
+	if (unlikely(napi_disable_pending(n))) {
+		napi_complete(n);
+		goto out_unlock;
+	}
+
+	if (n->gro_list) {
+		/* flush too old packets
+		 * If HZ < 1000, flush all packets.
+		 */
+		napi_gro_flush(n, HZ >= 1000);
+	}
+
+	list_add_tail(&n->poll_list, repoll);
+
+out_unlock:
+	netpoll_poll_unlock(have);
+
+	return work;
+}
+
 static void net_rx_action(struct softirq_action *h)
 {
 	struct softnet_data *sd = this_cpu_ptr(&softnet_data);

@@ -4564,7 +4617,6 @@ static void net_rx_action(struct softirq_action *h)
 	int budget = netdev_budget;
 	LIST_HEAD(list);
 	LIST_HEAD(repoll);
-	void *have;
 
 	local_irq_disable();
 	list_splice_init(&sd->poll_list, &list);

@@ -4572,7 +4624,6 @@ static void net_rx_action(struct softirq_action *h)
 
 	while (!list_empty(&list)) {
 		struct napi_struct *n;
-		int work, weight;
 
 		/* If softirq window is exhausted then punt.
 		 * Allow this to run for 2 jiffies since which will allow

@@ -4583,48 +4634,7 @@ static void net_rx_action(struct softirq_action *h)
 
 
 		n = list_first_entry(&list, struct napi_struct, poll_list);
-		list_del_init(&n->poll_list);
-
-		have = netpoll_poll_lock(n);
-
-		weight = n->weight;
-
-		/* This NAPI_STATE_SCHED test is for avoiding a race
-		 * with netpoll's poll_napi().  Only the entity which
-		 * obtains the lock and sees NAPI_STATE_SCHED set will
-		 * actually make the ->poll() call.  Therefore we avoid
-		 * accidentally calling ->poll() when NAPI is not scheduled.
-		 */
-		work = 0;
-		if (test_bit(NAPI_STATE_SCHED, &n->state)) {
-			work = n->poll(n, weight);
-			trace_napi_poll(n);
-		}
-
-		WARN_ON_ONCE(work > weight);
-
-		budget -= work;
-
-		/* Drivers must not modify the NAPI state if they
-		 * consume the entire weight.  In such cases this code
-		 * still "owns" the NAPI instance and therefore can
-		 * move the instance around on the list at-will.
-		 */
-		if (unlikely(work == weight)) {
-			if (unlikely(napi_disable_pending(n))) {
-				napi_complete(n);
-			} else {
-				if (n->gro_list) {
-					/* flush too old packets
-					 * If HZ < 1000, flush all packets.
-					 */
-					napi_gro_flush(n, HZ >= 1000);
-				}
-				list_add_tail(&n->poll_list, &repoll);
-			}
-		}
-
-		netpoll_poll_unlock(have);
+		budget -= napi_poll(n, &repoll);
 	}
 
 	if (!sd_has_rps_ipi_waiting(sd) &&
#14 lore raw

PATCH 2/4 net: Detect drivers that reschedule NAPI and exhaust budget

In reply to Herbert Xu

The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) required drivers to leave poll_list empty if the entire budget is consumed.

We have already had two broken drivers so let's add a check for this.

Signed-off-by Herbert Xu <[email protected]>

---

 net/core/dev.c |    9 +++++++++
 1 file changed, 9 insertions(+)
net/core/dev.c +9 -0
index f7c4f4e..4a9c424 100644
@@ -4602,6 +4602,15 @@ static int napi_poll(struct napi_struct *n, struct list_head *repoll)
 		napi_gro_flush(n, HZ >= 1000);
 	}
 
+	/* Some drivers may have called napi_schedule
+	 * prior to exhausting their budget.
+	 */
+	if (unlikely(!list_empty(&n->poll_list))) {
+		pr_warn_once("%s: Budget exhausted after napi rescheduled\n",
+			     n->dev ? n->dev->name : "backlog");
+		goto out_unlock;
+	}
+
 	list_add_tail(&n->poll_list, repoll);
 
 out_unlock:
maintainer Applied
#15 lore raw

Re: [PATCH 2/4] net: Detect drivers that reschedule NAPI and exhaust budget

From: Herbert Xu <[email protected]>
Date: Sun, 21 Dec 2014 07:16:22 +1100

Quoted 8 lines

The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) required drivers to leave poll_list empty if the entire budget is consumed.

We have already had two broken drivers so let's add a check for this.

Signed-off-by Herbert Xu <[email protected]>

Applied.

maintainer Applied
#16 lore raw

Re: [PATCH 2/4] net: Detect drivers that reschedule NAPI and exhaust budget

In reply to Herbert Xu

From: Herbert Xu <[email protected]>
Date: Sun, 21 Dec 2014 07:16:22 +1100

Quoted 8 lines

The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) required drivers to leave poll_list empty if the entire budget is consumed.

We have already had two broken drivers so let's add a check for this.

Signed-off-by Herbert Xu <[email protected]>

Applied.

#17 lore raw

PATCH 2/4 net: Detect drivers that reschedule NAPI and exhaust budget

In reply to Herbert Xu

The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) required drivers to leave poll_list empty if the entire budget is consumed.

We have already had two broken drivers so let's add a check for this.

Signed-off-by Herbert Xu <[email protected]>

---

 net/core/dev.c |    9 +++++++++
 1 file changed, 9 insertions(+)
net/core/dev.c +9 -0
index f7c4f4e..4a9c424 100644
@@ -4602,6 +4602,15 @@ static int napi_poll(struct napi_struct *n, struct list_head *repoll)
 		napi_gro_flush(n, HZ >= 1000);
 	}
 
+	/* Some drivers may have called napi_schedule
+	 * prior to exhausting their budget.
+	 */
+	if (unlikely(!list_empty(&n->poll_list))) {
+		pr_warn_once("%s: Budget exhausted after napi rescheduled\n",
+			     n->dev ? n->dev->name : "backlog");
+		goto out_unlock;
+	}
+
 	list_add_tail(&n->poll_list, repoll);
 
 out_unlock:
#18 lore raw

PATCH 3/4 net: Always poll at least one device in net_rx_action

In reply to Herbert Xu

We should only perform the softnet_break check after we have polled at least one device in net_rx_action. Otherwise a zero or negative setting of netdev_budget can lock up the whole system.

Signed-off-by Herbert Xu <[email protected]>

---

 net/core/dev.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
net/core/dev.c +3 -4
index 4a9c424..22b0fb2 100644
@@ -4634,16 +4634,15 @@ static void net_rx_action(struct softirq_action *h)
 	while (!list_empty(&list)) {
 		struct napi_struct *n;
 
+		n = list_first_entry(&list, struct napi_struct, poll_list);
+		budget -= napi_poll(n, &repoll);
+
 		/* If softirq window is exhausted then punt.
 		 * Allow this to run for 2 jiffies since which will allow
 		 * an average latency of 1.5/HZ.
 		 */
 		if (unlikely(budget <= 0 || time_after_eq(jiffies, time_limit)))
 			goto softnet_break;
-
-
-		n = list_first_entry(&list, struct napi_struct, poll_list);
-		budget -= napi_poll(n, &repoll);
 	}
 
 	if (!sd_has_rps_ipi_waiting(sd) &&
maintainer Applied
#19 lore raw

Re: [PATCH 3/4] net: Always poll at least one device in net_rx_action

From: Herbert Xu <[email protected]>
Date: Sun, 21 Dec 2014 07:16:24 +1100

Quoted 5 lines

We should only perform the softnet_break check after we have polled at least one device in net_rx_action. Otherwise a zero or negative setting of netdev_budget can lock up the whole system.

Signed-off-by Herbert Xu <[email protected]>

Applied.

maintainer Applied
#20 lore raw

Re: [PATCH 3/4] net: Always poll at least one device in net_rx_action

In reply to Herbert Xu

From: Herbert Xu <[email protected]>
Date: Sun, 21 Dec 2014 07:16:24 +1100

Quoted 5 lines

We should only perform the softnet_break check after we have polled at least one device in net_rx_action. Otherwise a zero or negative setting of netdev_budget can lock up the whole system.

Signed-off-by Herbert Xu <[email protected]>

Applied.

#21 lore raw

PATCH 3/4 net: Always poll at least one device in net_rx_action

In reply to Herbert Xu

We should only perform the softnet_break check after we have polled at least one device in net_rx_action. Otherwise a zero or negative setting of netdev_budget can lock up the whole system.

Signed-off-by Herbert Xu <[email protected]>

---

 net/core/dev.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
net/core/dev.c +3 -4
index 4a9c424..22b0fb2 100644
@@ -4634,16 +4634,15 @@ static void net_rx_action(struct softirq_action *h)
 	while (!list_empty(&list)) {
 		struct napi_struct *n;
 
+		n = list_first_entry(&list, struct napi_struct, poll_list);
+		budget -= napi_poll(n, &repoll);
+
 		/* If softirq window is exhausted then punt.
 		 * Allow this to run for 2 jiffies since which will allow
 		 * an average latency of 1.5/HZ.
 		 */
 		if (unlikely(budget <= 0 || time_after_eq(jiffies, time_limit)))
 			goto softnet_break;
-
-
-		n = list_first_entry(&list, struct napi_struct, poll_list);
-		budget -= napi_poll(n, &repoll);
 	}
 
 	if (!sd_has_rps_ipi_waiting(sd) &&
#22 lore raw

PATCH 4/4 net: Rearrange loop in net_rx_action

In reply to Herbert Xu

This patch rearranges the loop in net_rx_action to reduce the amount of jumping back and forth when reading the code.

Signed-off-by Herbert Xu <[email protected]>

---

 net/core/dev.c |   26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)
net/core/dev.c +12 -14
index 22b0fb2..dd565a5 100644
@@ -4631,9 +4631,15 @@ static void net_rx_action(struct softirq_action *h)
 	list_splice_init(&sd->poll_list, &list);
 	local_irq_enable();
 
-	while (!list_empty(&list)) {
+	for (;;) {
 		struct napi_struct *n;
 
+		if (list_empty(&list)) {
+			if (!sd_has_rps_ipi_waiting(sd) && list_empty(&repoll))
+				return;
+			break;
+		}
+
 		n = list_first_entry(&list, struct napi_struct, poll_list);
 		budget -= napi_poll(n, &repoll);
 

@@ -4641,15 +4647,13 @@ static void net_rx_action(struct softirq_action *h)
 		 * Allow this to run for 2 jiffies since which will allow
 		 * an average latency of 1.5/HZ.
 		 */
-		if (unlikely(budget <= 0 || time_after_eq(jiffies, time_limit)))
-			goto softnet_break;
+		if (unlikely(budget <= 0 ||
+			     time_after_eq(jiffies, time_limit))) {
+			sd->time_squeeze++;
+			break;
+		}
 	}
 
-	if (!sd_has_rps_ipi_waiting(sd) &&
-	    list_empty(&list) &&
-	    list_empty(&repoll))
-		return;
-out:
 	local_irq_disable();
 
 	list_splice_tail_init(&sd->poll_list, &list);

@@ -4659,12 +4663,6 @@ out:
 		__raise_softirq_irqoff(NET_RX_SOFTIRQ);
 
 	net_rps_action_and_irq_enable(sd);
-
-	return;
-
-softnet_break:
-	sd->time_squeeze++;
-	goto out;
 }
 
 struct netdev_adjacent {
maintainer Applied
#23 lore raw

Re: [PATCH 4/4] net: Rearrange loop in net_rx_action

From: Herbert Xu <[email protected]>
Date: Sun, 21 Dec 2014 07:16:25 +1100

Quoted 4 lines

This patch rearranges the loop in net_rx_action to reduce the amount of jumping back and forth when reading the code.

Signed-off-by Herbert Xu <[email protected]>

Applied.

#25 lore raw

PATCH 4/4 net: Rearrange loop in net_rx_action

In reply to Herbert Xu

This patch rearranges the loop in net_rx_action to reduce the amount of jumping back and forth when reading the code.

Signed-off-by Herbert Xu <[email protected]>

---

 net/core/dev.c |   26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)
net/core/dev.c +12 -14
index 22b0fb2..dd565a5 100644
@@ -4631,9 +4631,15 @@ static void net_rx_action(struct softirq_action *h)
 	list_splice_init(&sd->poll_list, &list);
 	local_irq_enable();
 
-	while (!list_empty(&list)) {
+	for (;;) {
 		struct napi_struct *n;
 
+		if (list_empty(&list)) {
+			if (!sd_has_rps_ipi_waiting(sd) && list_empty(&repoll))
+				return;
+			break;
+		}
+
 		n = list_first_entry(&list, struct napi_struct, poll_list);
 		budget -= napi_poll(n, &repoll);
 

@@ -4641,15 +4647,13 @@ static void net_rx_action(struct softirq_action *h)
 		 * Allow this to run for 2 jiffies since which will allow
 		 * an average latency of 1.5/HZ.
 		 */
-		if (unlikely(budget <= 0 || time_after_eq(jiffies, time_limit)))
-			goto softnet_break;
+		if (unlikely(budget <= 0 ||
+			     time_after_eq(jiffies, time_limit))) {
+			sd->time_squeeze++;
+			break;
+		}
 	}
 
-	if (!sd_has_rps_ipi_waiting(sd) &&
-	    list_empty(&list) &&
-	    list_empty(&repoll))
-		return;
-out:
 	local_irq_disable();
 
 	list_splice_tail_init(&sd->poll_list, &list);

@@ -4659,12 +4663,6 @@ out:
 		__raise_softirq_irqoff(NET_RX_SOFTIRQ);
 
 	net_rps_action_and_irq_enable(sd);
-
-	return;
-
-softnet_break:
-	sd->time_squeeze++;
-	goto out;
 }
 
 struct netdev_adjacent {
#26 lore raw

cover net: net_rx_action fixes and clean-ups

In reply to Eric Dumazet
Quoted 6 lines · Eric Dumazet

OK, but could you :
1) use pr_warn_once()
2) split the too long line

	pr_warn_once("%s: Budget exhausted after napi rescheduled\n",
		     n->dev ? n->dev->name : "backlog");

Sure, I'll clean it up a bit too.

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#27 lore raw

Re: net: Detect drivers that reschedule NAPI and exhaust budget

In reply to Herbert Xu
Quoted 29 lines · Herbert Xu

-- >8 --
The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) required drivers to leave poll_list empty if the entire budget is consumed.

We have already had two broken drivers so let's add a check for this.

Signed-off-by Herbert Xu <[email protected]>
net/core/dev.c +7 -1
index f411c28..47fdc5c 100644
@@ -4620,7 +4620,13 @@ static void net_rx_action(struct softirq_action *h)
 					 */
 					napi_gro_flush(n, HZ >= 1000);
 				}
-				list_add_tail(&n->poll_list, &repoll);
+				/* Some drivers may have called napi_schedule
+				 * prior to exhausting their budget.
+				 */
+				if (unlikely(!list_empty(&n->poll_list)))
+					pr_warn("%s: Budget exhausted after napi rescheduled\n", n->dev ? n->dev->name : "backlog");
+				else
+					list_add_tail(&n->poll_list, &repoll);
 			}
 		}
Quoted 1 line

Thanks,

OK, but could you :
1) use pr_warn_once()
2) split the too long line

	pr_warn_once("%s: Budget exhausted after napi rescheduled\n",
		     n->dev ? n->dev->name : "backlog");

Thanks Herbert !

#28 lore raw

Re: net: Detect drivers that reschedule NAPI and exhaust budget

In reply to David Miller
Quoted 19 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Fri, 19 Dec 2014 17:34:48 -0800

Quoted 16 lines

Review static void net_rx_action(struct softirq_action *h) · line 4630

Quoted 13 lines
(quoted diff) +5 -1
@@ -4620,7 +4620,11 @@ static void net_rx_action(struct softirq_action *h)
 					 */
 					napi_gro_flush(n, HZ >= 1000);
 				}
-				list_add_tail(&n->poll_list, &repoll);
+				/* Some drivers may have called napi_schedule
+				 * prior to exhausting their budget.
+				 */
+				if (!WARN_ON_ONCE(!list_empty(&n->poll_list)))
+					list_add_tail(&n->poll_list, &repoll);
 			}
 		}

I do not think stack trace will point to the buggy driver ?

Quoted 4 lines
Quoted 1 line

IMO it would be better to print a single line with the netdev name ?

Right, we are already back from the poll routine and will just end up seeing the call trace leading to the software interrupt.

Good point Eric.

-- >8 --
The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) required drivers to leave poll_list empty if the entire budget is consumed.

We have already had two broken drivers so let's add a check for this.

Signed-off-by Herbert Xu <[email protected]>
net/core/dev.c +7 -1
index f411c28..47fdc5c 100644
@@ -4620,7 +4620,13 @@ static void net_rx_action(struct softirq_action *h)
 					 */
 					napi_gro_flush(n, HZ >= 1000);
 				}
-				list_add_tail(&n->poll_list, &repoll);
+				/* Some drivers may have called napi_schedule
+				 * prior to exhausting their budget.
+				 */
+				if (unlikely(!list_empty(&n->poll_list)))
+					pr_warn("%s: Budget exhausted after napi rescheduled\n", n->dev ? n->dev->name : "backlog");
+				else
+					list_add_tail(&n->poll_list, &repoll);
 			}
 		}

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
maintainer
#29 lore raw

Re: net: Detect drivers that reschedule NAPI and exhaust budget

In reply to Eric Dumazet

From: Eric Dumazet <[email protected]>
Date: Fri, 19 Dec 2014 17:34:48 -0800

Quoted 16 lines

net/core/dev.c static void net_rx_action(struct softirq_action *h) · line 4630

Quoted 13 lines
net/core/dev.c +5 -1
@@ -4620,7 +4620,11 @@ static void net_rx_action(struct softirq_action *h)
 					 */
 					napi_gro_flush(n, HZ >= 1000);
 				}
-				list_add_tail(&n->poll_list, &repoll);
+				/* Some drivers may have called napi_schedule
+				 * prior to exhausting their budget.
+				 */
+				if (!WARN_ON_ONCE(!list_empty(&n->poll_list)))
+					list_add_tail(&n->poll_list, &repoll);
 			}
 		}

I do not think stack trace will point to the buggy driver ?

Quoted 1 line

IMO it would be better to print a single line with the netdev name ?

Right, we are already back from the poll routine and will just end up seeing the call trace leading to the software interrupt.

#30 lore raw

Re: net: Detect drivers that reschedule NAPI and exhaust budget

In reply to Herbert Xu
Quoted 24 lines · Herbert Xu
Quoted 2 lines · Herbert Xu

A similar bug exists in virtio_net.

In order to detect other drivers doing this we should add something like this.

-- >8 --
The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) required drivers to leave poll_list empty if the entire budget is consumed.

We have already had two broken drivers so let's add a check for this.

Signed-off-by Herbert Xu <[email protected]>
net/core/dev.c +0 -0
index f411c28..88f9725 100644
@@ -4620,7 +4620,11 @@ static void net_rx_action(struct softirq_action *h)
 					 */
 					napi_gro_flush(n, HZ >= 1000);

net/core/dev.c static void net_rx_action(struct softirq_action *h) · line 4630

Quoted 10 lines
net/core/dev.c +5 -1
 				}
-				list_add_tail(&n->poll_list, &repoll);
+				/* Some drivers may have called napi_schedule
+				 * prior to exhausting their budget.
+				 */
+				if (!WARN_ON_ONCE(!list_empty(&n->poll_list)))
+					list_add_tail(&n->poll_list, &repoll);
 			}
 		}

I do not think stack trace will point to the buggy driver ?

IMO it would be better to print a single line with the netdev name ?

#31 lore raw

net: Detect drivers that reschedule NAPI and exhaust budget

In reply to Herbert Xu
Quoted 2 lines · Herbert Xu

A similar bug exists in virtio_net.

In order to detect other drivers doing this we should add something like this.

-- >8 --
The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) required drivers to leave poll_list empty if the entire budget is consumed.

We have already had two broken drivers so let's add a check for this.

Signed-off-by Herbert Xu <[email protected]>
net/core/dev.c +5 -1
index f411c28..88f9725 100644
@@ -4620,7 +4620,11 @@ static void net_rx_action(struct softirq_action *h)
 					 */
 					napi_gro_flush(n, HZ >= 1000);
 				}
-				list_add_tail(&n->poll_list, &repoll);
+				/* Some drivers may have called napi_schedule
+				 * prior to exhausting their budget.
+				 */
+				if (!WARN_ON_ONCE(!list_empty(&n->poll_list)))
+					list_add_tail(&n->poll_list, &repoll);
 			}
 		}

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Acked-by
#32 lore raw

Re: virtio_net: Fix napi poll list corruption

In reply to Herbert Xu
Quoted 51 lines · Herbert Xu
Quoted 5 lines · David Vrabel

After d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) the napi instance is removed from the per-cpu list prior to calling the n->poll(), and is only requeued if all of the budget was used. This inadvertently broke netfront because netfront does not use NAPI correctly.

A similar bug exists in virtio_net.

-- >8 --
The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) breaks virtio_net in an insidious way.

It is now required that if the entire budget is consumed when poll returns, the napi poll_list must remain empty. However, like some other drivers virtio_net tries to do a last-ditch check and if there is more work it will call napi_schedule and then immediately process some of this new work. Should the entire budget be consumed while processing such new work then we will violate the new caller contract.

This patch fixes this by not touching any work when we reschedule in virtio_net.

The worst part of this bug is that the list corruption causes other napi users to be moved off-list. In my case I was chasing a stall in IPsec (IPsec uses netif_rx) and I only belatedly realised that it was virtio_net which caused the stall even though the virtio_net poll was still functioning perfectly after IPsec stalled.

Signed-off-by Herbert Xu <[email protected]>
drivers/net/virtio_net.c +0 -2
index b8bd719..5ca9771 100644
@@ -760,7 +760,6 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
 		container_of(napi, struct receive_queue, napi);
 	unsigned int r, received = 0;
 
-again:
 	received += virtnet_receive(rq, budget - received);
 
 	/* Out of packets? */

@@ -771,7 +770,6 @@ again:
 		    napi_schedule_prep(napi)) {
 			virtqueue_disable_cb(rq->vq);
 			__napi_schedule(napi);
-			goto again;
 		}
 	}
Quoted 1 line

Cheers,

Acked-by Jason Wang <[email protected]>

btw, looks like at least caif_virtio has the same issue.

#33 lore raw

caif: Fix napi poll list corruption

Quoted 2 lines · Jason Wang

btw, looks like at least caif_virtio has the same issue.

Good catch.

-- >8 --
The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) breaks caif.

It is now required that if the entire budget is consumed when poll returns, the napi poll_list must remain empty. However, like some other drivers caif tries to do a last-ditch check and if there is more work it will call napi_schedule and then immediately process some of this new work. Should the entire budget be consumed while processing such new work then we will violate the new caller contract.

This patch fixes this by not touching any work when we reschedule in caif.

Signed-off-by Herbert Xu <[email protected]>
drivers/net/caif/caif_virtio.c +0 -2
index a5fefb9..b306210 100644
@@ -257,7 +257,6 @@ static int cfv_rx_poll(struct napi_struct *napi, int quota)
 	struct vringh_kiov *riov = &cfv->ctx.riov;
 	unsigned int skb_len;
 
-again:
 	do {
 		skb = NULL;
 

@@ -322,7 +321,6 @@ exit:
 		    napi_schedule_prep(napi)) {
 			vringh_notify_disable_kern(cfv->vr_rx);
 			__napi_schedule(napi);
-			goto again;
 		}
 		break;

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
maintainer Applied
#34 lore raw

Re: caif: Fix napi poll list corruption

From: Herbert Xu <[email protected]>
Date: Mon, 22 Dec 2014 20:35:25 +1100

Quoted 11 lines

The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) breaks caif.

It is now required that if the entire budget is consumed when poll returns, the napi poll_list must remain empty. However, like some other drivers caif tries to do a last-ditch check and if there is more work it will call napi_schedule and then immediately process some of this new work. Should the entire budget be consumed while processing such new work then we will violate the new caller contract.

Quoted 4 lines

This patch fixes this by not touching any work when we reschedule in caif.

Signed-off-by Herbert Xu <[email protected]>

Applied, thanks.

maintainer Applied
#35 lore raw

Re: caif: Fix napi poll list corruption

In reply to Herbert Xu

From: Herbert Xu <[email protected]>
Date: Mon, 22 Dec 2014 20:35:25 +1100

Quoted 11 lines

The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) breaks caif.

It is now required that if the entire budget is consumed when poll returns, the napi poll_list must remain empty. However, like some other drivers caif tries to do a last-ditch check and if there is more work it will call napi_schedule and then immediately process some of this new work. Should the entire budget be consumed while processing such new work then we will violate the new caller contract.

Quoted 4 lines

This patch fixes this by not touching any work when we reschedule in caif.

Signed-off-by Herbert Xu <[email protected]>

Applied, thanks.

Acked-by
#36 lore raw

Re: caif: Fix napi poll list corruption

In reply to Herbert Xu
Quoted 18 lines · Herbert Xu
Quoted 2 lines · Jason Wang

btw, looks like at least caif_virtio has the same issue.

Good catch.

-- >8 --
The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) breaks caif.

It is now required that if the entire budget is consumed when poll returns, the napi poll_list must remain empty. However, like some other drivers caif tries to do a last-ditch check and if there is more work it will call napi_schedule and then immediately process some of this new work. Should the entire budget be consumed while processing such new work then we will violate the new caller contract.

Quoted 4 lines

This patch fixes this by not touching any work when we reschedule in caif.

Signed-off-by Herbert Xu <[email protected]>
Acked-by Jason Wang <[email protected]>

Thanks.

Quoted 29 lines
drivers/net/caif/caif_virtio.c +0 -2
index a5fefb9..b306210 100644
@@ -257,7 +257,6 @@ static int cfv_rx_poll(struct napi_struct *napi, 
 int quota)
 	struct vringh_kiov *riov = &cfv->ctx.riov;
 	unsigned int skb_len;
 
-again:
 	do {
 		skb = NULL;

@@ -322,7 +321,6 @@ exit:
 		    napi_schedule_prep(napi)) {
 			vringh_notify_disable_kern(cfv->vr_rx);
 			__napi_schedule(napi);
-			goto again;
 		}
 		break;

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Acked-by
#37 lore raw

Re: caif: Fix napi poll list corruption

In reply to Herbert Xu
Quoted 18 lines · Herbert Xu
Quoted 2 lines · Jason Wang

btw, looks like at least caif_virtio has the same issue.

Good catch.

-- >8 --
The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) breaks caif.

It is now required that if the entire budget is consumed when poll returns, the napi poll_list must remain empty. However, like some other drivers caif tries to do a last-ditch check and if there is more work it will call napi_schedule and then immediately process some of this new work. Should the entire budget be consumed while processing such new work then we will violate the new caller contract.

Quoted 4 lines

This patch fixes this by not touching any work when we reschedule in caif.

Signed-off-by Herbert Xu <[email protected]>
Acked-by Jason Wang <[email protected]>

Thanks.

Quoted 29 lines
drivers/net/caif/caif_virtio.c +0 -2
index a5fefb9..b306210 100644
@@ -257,7 +257,6 @@ static int cfv_rx_poll(struct napi_struct *napi, 
 int quota)
 	struct vringh_kiov *riov = &cfv->ctx.riov;
 	unsigned int skb_len;
 
-again:
 	do {
 		skb = NULL;

@@ -322,7 +321,6 @@ exit:
 		    napi_schedule_prep(napi)) {
 			vringh_notify_disable_kern(cfv->vr_rx);
 			__napi_schedule(napi);
-			goto again;
 		}
 		break;

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#38 lore raw

caif: Fix napi poll list corruption

In reply to Jason Wang
Quoted 2 lines · Jason Wang

btw, looks like at least caif_virtio has the same issue.

Good catch.

-- >8 --
The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) breaks caif.

It is now required that if the entire budget is consumed when poll returns, the napi poll_list must remain empty. However, like some other drivers caif tries to do a last-ditch check and if there is more work it will call napi_schedule and then immediately process some of this new work. Should the entire budget be consumed while processing such new work then we will violate the new caller contract.

This patch fixes this by not touching any work when we reschedule in caif.

Signed-off-by Herbert Xu <[email protected]>
drivers/net/caif/caif_virtio.c +0 -2
index a5fefb9..b306210 100644
@@ -257,7 +257,6 @@ static int cfv_rx_poll(struct napi_struct *napi, int quota)
 	struct vringh_kiov *riov = &cfv->ctx.riov;
 	unsigned int skb_len;
 
-again:
 	do {
 		skb = NULL;
 

@@ -322,7 +321,6 @@ exit:
 		    napi_schedule_prep(napi)) {
 			vringh_notify_disable_kern(cfv->vr_rx);
 			__napi_schedule(napi);
-			goto again;
 		}
 		break;

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Acked-by
#39 lore raw

Re: virtio_net: Fix napi poll list corruption

In reply to Herbert Xu
Quoted 51 lines · Herbert Xu
Quoted 5 lines · David Vrabel

After d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) the napi instance is removed from the per-cpu list prior to calling the n->poll(), and is only requeued if all of the budget was used. This inadvertently broke netfront because netfront does not use NAPI correctly.

A similar bug exists in virtio_net.

-- >8 --
The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) breaks virtio_net in an insidious way.

It is now required that if the entire budget is consumed when poll returns, the napi poll_list must remain empty. However, like some other drivers virtio_net tries to do a last-ditch check and if there is more work it will call napi_schedule and then immediately process some of this new work. Should the entire budget be consumed while processing such new work then we will violate the new caller contract.

This patch fixes this by not touching any work when we reschedule in virtio_net.

The worst part of this bug is that the list corruption causes other napi users to be moved off-list. In my case I was chasing a stall in IPsec (IPsec uses netif_rx) and I only belatedly realised that it was virtio_net which caused the stall even though the virtio_net poll was still functioning perfectly after IPsec stalled.

Signed-off-by Herbert Xu <[email protected]>
drivers/net/virtio_net.c +0 -2
index b8bd719..5ca9771 100644
@@ -760,7 +760,6 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
 		container_of(napi, struct receive_queue, napi);
 	unsigned int r, received = 0;
 
-again:
 	received += virtnet_receive(rq, budget - received);
 
 	/* Out of packets? */

@@ -771,7 +770,6 @@ again:
 		    napi_schedule_prep(napi)) {
 			virtqueue_disable_cb(rq->vq);
 			__napi_schedule(napi);
-			goto again;
 		}
 	}
Quoted 1 line

Cheers,

Acked-by Jason Wang <[email protected]>

btw, looks like at least caif_virtio has the same issue.

#40 lore raw

Re: virtio_net: Fix napi poll list corruption

In reply to Herbert Xu
Quoted 21 lines · Herbert Xu
Quoted 5 lines · David Vrabel

After d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) the napi instance is removed from the per-cpu list prior to calling the n->poll(), and is only requeued if all of the budget was used. This inadvertently broke netfront because netfront does not use NAPI correctly.

A similar bug exists in virtio_net.

-- >8 --
The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) breaks virtio_net in an insidious way.

It is now required that if the entire budget is consumed when poll returns, the napi poll_list must remain empty. However, like some other drivers virtio_net tries to do a last-ditch check and if there is more work it will call napi_schedule and then immediately process some of this new work. Should the entire budget be consumed while processing such new work then we will violate the new caller contract.

Quoted 8 lines

This patch fixes this by not touching any work when we reschedule in virtio_net.

The worst part of this bug is that the list corruption causes other napi users to be moved off-list. In my case I was chasing a stall in IPsec (IPsec uses netif_rx) and I only belatedly realised that it was virtio_net which caused the stall even though the virtio_net poll was still functioning perfectly after IPsec stalled.

Thanks for finding/fixing this, Herbert. I was debugging this one too. In my case, vxlan interface was getting stuck.

Marcelo

#41 lore raw

Re: virtio_net: Fix napi poll list corruption

In reply to Herbert Xu
Quoted 21 lines · Herbert Xu
Quoted 5 lines · David Vrabel

After d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) the napi instance is removed from the per-cpu list prior to calling the n->poll(), and is only requeued if all of the budget was used. This inadvertently broke netfront because netfront does not use NAPI correctly.

A similar bug exists in virtio_net.

-- >8 --
The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) breaks virtio_net in an insidious way.

It is now required that if the entire budget is consumed when poll returns, the napi poll_list must remain empty. However, like some other drivers virtio_net tries to do a last-ditch check and if there is more work it will call napi_schedule and then immediately process some of this new work. Should the entire budget be consumed while processing such new work then we will violate the new caller contract.

Quoted 8 lines

This patch fixes this by not touching any work when we reschedule in virtio_net.

The worst part of this bug is that the list corruption causes other napi users to be moved off-list. In my case I was chasing a stall in IPsec (IPsec uses netif_rx) and I only belatedly realised that it was virtio_net which caused the stall even though the virtio_net poll was still functioning perfectly after IPsec stalled.

Thanks for finding/fixing this, Herbert. I was debugging this one too. In my case, vxlan interface was getting stuck.

Marcelo

maintainer Applied
#42 lore raw

Re: virtio_net: Fix napi poll list corruption

In reply to Herbert Xu

From: Herbert Xu <[email protected]>
Date: Sat, 20 Dec 2014 11:23:27 +1100

Quoted 14 lines

The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) breaks virtio_net in an insidious way.

It is now required that if the entire budget is consumed when poll returns, the napi poll_list must remain empty. However, like some other drivers virtio_net tries to do a last-ditch check and if there is more work it will call napi_schedule and then immediately process some of this new work. Should the entire budget be consumed while processing such new work then we will violate the new caller contract.

This patch fixes this by not touching any work when we reschedule in virtio_net.

Quoted 7 lines

The worst part of this bug is that the list corruption causes other napi users to be moved off-list. In my case I was chasing a stall in IPsec (IPsec uses netif_rx) and I only belatedly realised that it was virtio_net which caused the stall even though the virtio_net poll was still functioning perfectly after IPsec stalled.

Signed-off-by Herbert Xu <[email protected]>

Applied, thanks Herbert.

maintainer Applied
#43 lore raw

Re: virtio_net: Fix napi poll list corruption

In reply to Herbert Xu

From: Herbert Xu <[email protected]>
Date: Sat, 20 Dec 2014 11:23:27 +1100

Quoted 14 lines

The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) breaks virtio_net in an insidious way.

It is now required that if the entire budget is consumed when poll returns, the napi poll_list must remain empty. However, like some other drivers virtio_net tries to do a last-ditch check and if there is more work it will call napi_schedule and then immediately process some of this new work. Should the entire budget be consumed while processing such new work then we will violate the new caller contract.

This patch fixes this by not touching any work when we reschedule in virtio_net.

Quoted 7 lines

The worst part of this bug is that the list corruption causes other napi users to be moved off-list. In my case I was chasing a stall in IPsec (IPsec uses netif_rx) and I only belatedly realised that it was virtio_net which caused the stall even though the virtio_net poll was still functioning perfectly after IPsec stalled.

Signed-off-by Herbert Xu <[email protected]>

Applied, thanks Herbert.

#44 lore raw

virtio_net: Fix napi poll list corruption

In reply to David Vrabel
Quoted 5 lines · David Vrabel

After d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) the napi instance is removed from the per-cpu list prior to calling the n->poll(), and is only requeued if all of the budget was used. This inadvertently broke netfront because netfront does not use NAPI correctly.

A similar bug exists in virtio_net.

-- >8 --
The commit d75b1ade567ffab085e8adbbdacf0092d10cd09c (net: less interrupt masking in NAPI) breaks virtio_net in an insidious way.

It is now required that if the entire budget is consumed when poll returns, the napi poll_list must remain empty. However, like some other drivers virtio_net tries to do a last-ditch check and if there is more work it will call napi_schedule and then immediately process some of this new work. Should the entire budget be consumed while processing such new work then we will violate the new caller contract.

This patch fixes this by not touching any work when we reschedule in virtio_net.

The worst part of this bug is that the list corruption causes other napi users to be moved off-list. In my case I was chasing a stall in IPsec (IPsec uses netif_rx) and I only belatedly realised that it was virtio_net which caused the stall even though the virtio_net poll was still functioning perfectly after IPsec stalled.

Signed-off-by Herbert Xu <[email protected]>
drivers/net/virtio_net.c +0 -2
index b8bd719..5ca9771 100644
@@ -760,7 +760,6 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
 		container_of(napi, struct receive_queue, napi);
 	unsigned int r, received = 0;
 
-again:
 	received += virtnet_receive(rq, budget - received);
 
 	/* Out of packets? */

@@ -771,7 +770,6 @@ again:
 		    napi_schedule_prep(napi)) {
 			virtqueue_disable_cb(rq->vq);
 			__napi_schedule(napi);
-			goto again;
 		}
 	}

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt