virtio_net: fix oom handling on tx

PATCH In review Michael S. Tsirkin 4 participants 19 messages Jun 10, 2010 – Jun 21, 2010

Subsystems (4)

Networking Network drivers virtio core virtio net driver

lore

Share & tools

lore

Michael S. Tsirkin

author maintainer

2010-06-10 15:25 UTC #0 lore raw

PATCH 1/1 virtio_net: fix oom handling on tx

virtio net will never try to overflow the TX ring, so the only reason add_buf may fail is out of memory. Thus, we can not stop the device until some request completes - there's no guarantee anything at all is outstanding.

Make the error message clearer as well: error here does not indicate queue full.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/net/virtio_net.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

drivers/net/virtio_net.c +8 -7

index 85615a3..e48a06f 100644
@@ -563,7 +563,6 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct virtnet_info *vi = netdev_priv(dev);
 	int capacity;
 
-again:
 	/* Free up any pending old buffers before queueing new ones. */
 	free_old_xmit_skbs(vi);

@@ -572,12 +571,14 @@ again:
 
 	/* This can happen with OOM and indirect buffers. */
 	if (unlikely(capacity < 0)) {
-		netif_stop_queue(dev);
-		dev_warn(&dev->dev, "Unexpected full queue\n");
-		if (unlikely(!virtqueue_enable_cb(vi->svq))) {
-			virtqueue_disable_cb(vi->svq);
-			netif_start_queue(dev);
-			goto again;
+		if (net_ratelimit()) {
+			if (likely(capacity == -ENOMEM))
+				dev_warn(&dev->dev,
+					 "TX queue failure: out of memory\n");
+			else
+				dev_warn(&dev->dev,
+					 "Unexpected TX queue failure: %d\n",
+					 capacity);
 		}
 		return NETDEV_TX_BUSY;
 	}

Signature · 1 line

1.7.1.12.g42b7f

Sridhar Samudrala

2010-06-10 17:17 UTC #1 lore raw

Quoted 36 lines · Michael S. Tsirkin

virtio net will never try to overflow the TX ring, so the only reason add_buf may fail is out of memory. Thus, we can not stop the device until some request completes - there's no guarantee anything at all is outstanding.
Make the error message clearer as well: error here does not indicate queue full.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 drivers/net/virtio_net.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)
drivers/net/virtio_net.c +1 -7
index 85615a3..e48a06f 100644
@@ -563,7 +563,6 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct virtnet_info *vi = netdev_priv(dev);
 	int capacity;
 
-again:
 	/* Free up any pending old buffers before queueing new ones. */
 	free_old_xmit_skbs(vi);
 
@@ -572,12 +571,14 @@ again:
 
 	/* This can happen with OOM and indirect buffers. */
 	if (unlikely(capacity < 0)) {
-		netif_stop_queue(dev);
-		dev_warn(&dev->dev, "Unexpected full queue\n");
-		if (unlikely(!virtqueue_enable_cb(vi->svq))) {
-			virtqueue_disable_cb(vi->svq);
-			netif_start_queue(dev);
-			goto again;
+		if (net_ratelimit()) {

drivers/net/virtio_net.c again: · line 584

Quoted 10 lines

drivers/net/virtio_net.c +7 -0

+			if (likely(capacity == -ENOMEM))
+				dev_warn(&dev->dev,
+					 "TX queue failure: out of memory\n");
+			else
+				dev_warn(&dev->dev,
+					 "Unexpected TX queue failure: %d\n",
+					 capacity);
 		}
 		return NETDEV_TX_BUSY;
 	}

It is not clear to me how xmit_skb() can return -ENOMEM. xmit_skb() calls virtqueue_add_buf_gfp() which can return -ENOSPC. Even vring_add_indirect() doesn't return -ENOMEM on kmalloc failure.

Thanks
Sridhar

Sridhar Samudrala

2010-06-10 17:17 UTC #18 lore raw

In reply to Michael S. Tsirkin

Quoted 36 lines · Michael S. Tsirkin

virtio net will never try to overflow the TX ring, so the only reason add_buf may fail is out of memory. Thus, we can not stop the device until some request completes - there's no guarantee anything at all is outstanding.
Make the error message clearer as well: error here does not indicate queue full.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 drivers/net/virtio_net.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)
drivers/net/virtio_net.c +1 -7
index 85615a3..e48a06f 100644
@@ -563,7 +563,6 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct virtnet_info *vi = netdev_priv(dev);
 	int capacity;
 
-again:
 	/* Free up any pending old buffers before queueing new ones. */
 	free_old_xmit_skbs(vi);
 
@@ -572,12 +571,14 @@ again:
 
 	/* This can happen with OOM and indirect buffers. */
 	if (unlikely(capacity < 0)) {
-		netif_stop_queue(dev);
-		dev_warn(&dev->dev, "Unexpected full queue\n");
-		if (unlikely(!virtqueue_enable_cb(vi->svq))) {
-			virtqueue_disable_cb(vi->svq);
-			netif_start_queue(dev);
-			goto again;
+		if (net_ratelimit()) {

drivers/net/virtio_net.c again: · line 584

Quoted 10 lines

drivers/net/virtio_net.c +7 -0

+			if (likely(capacity == -ENOMEM))
+				dev_warn(&dev->dev,
+					 "TX queue failure: out of memory\n");
+			else
+				dev_warn(&dev->dev,
+					 "Unexpected TX queue failure: %d\n",
+					 capacity);
 		}
 		return NETDEV_TX_BUSY;
 	}

Thanks
Sridhar

Stephen Hemminger

2010-06-10 17:46 UTC #2 lore raw

In reply to Sridhar Samudrala

Quoted 43 lines · On Thu, 10 Jun 2010 10:17:07 -0700 Sridhar Samudrala <[email protected]> wrote:

Quoted 42 lines · Michael S. Tsirkin

Make the error message clearer as well: error here does not indicate queue full.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/net/virtio_net.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

drivers/net/virtio_net.c +7 -7

index 85615a3..e48a06f 100644
@@ -563,7 +563,6 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct virtnet_info *vi = netdev_priv(dev);
 	int capacity;
 
-again:
 	/* Free up any pending old buffers before queueing new ones. */
 	free_old_xmit_skbs(vi);

@@ -572,12 +571,14 @@ again:
 
 	/* This can happen with OOM and indirect buffers. */
 	if (unlikely(capacity < 0)) {
-		netif_stop_queue(dev);
-		dev_warn(&dev->dev, "Unexpected full queue\n");
-		if (unlikely(!virtqueue_enable_cb(vi->svq))) {
-			virtqueue_disable_cb(vi->svq);
-			netif_start_queue(dev);
-			goto again;
+		if (net_ratelimit()) {
+			if (likely(capacity == -ENOMEM))
+				dev_warn(&dev->dev,
+					 "TX queue failure: out of memory\n");
+			else
+				dev_warn(&dev->dev,
+					 "Unexpected TX queue failure: %d\n",

Quoted 8 lines

drivers/net/virtio_net.c again: · line 584
Quoted 4 lines
drivers/net/virtio_net.c +1 -0
+					 capacity);
 		}
 		return NETDEV_TX_BUSY;
 	}
It is not clear to me how xmit_skb() can return -ENOMEM. xmit_skb() calls virtqueue_add_buf_gfp() which can return -ENOSPC. Even vring_add_indirect() doesn't return -ENOMEM on kmalloc failure.

It makes more sense to have the device increment tx_droppped, and return NETDEV_TX_OK. Skip the message (or make it a pr_debug()). Network devices do not guarantee packet delivery, and if out of resources then holding more data in the
queue is going to hurt not help the situation.

Stephen Hemminger

2010-06-10 17:46 UTC #15 lore raw

In reply to Sridhar Samudrala

Quoted 43 lines · On Thu, 10 Jun 2010 10:17:07 -0700 Sridhar Samudrala <[email protected]> wrote:

Quoted 42 lines · Michael S. Tsirkin

Make the error message clearer as well: error here does not indicate queue full.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/net/virtio_net.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

drivers/net/virtio_net.c +7 -7

index 85615a3..e48a06f 100644
@@ -563,7 +563,6 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct virtnet_info *vi = netdev_priv(dev);
 	int capacity;
 
-again:
 	/* Free up any pending old buffers before queueing new ones. */
 	free_old_xmit_skbs(vi);

@@ -572,12 +571,14 @@ again:
 
 	/* This can happen with OOM and indirect buffers. */
 	if (unlikely(capacity < 0)) {
-		netif_stop_queue(dev);
-		dev_warn(&dev->dev, "Unexpected full queue\n");
-		if (unlikely(!virtqueue_enable_cb(vi->svq))) {
-			virtqueue_disable_cb(vi->svq);
-			netif_start_queue(dev);
-			goto again;
+		if (net_ratelimit()) {
+			if (likely(capacity == -ENOMEM))
+				dev_warn(&dev->dev,
+					 "TX queue failure: out of memory\n");
+			else
+				dev_warn(&dev->dev,
+					 "Unexpected TX queue failure: %d\n",

Quoted 8 lines

drivers/net/virtio_net.c again: · line 584
Quoted 4 lines
drivers/net/virtio_net.c +1 -0
+					 capacity);
 		}
 		return NETDEV_TX_BUSY;
 	}
It is not clear to me how xmit_skb() can return -ENOMEM. xmit_skb() calls virtqueue_add_buf_gfp() which can return -ENOSPC. Even vring_add_indirect() doesn't return -ENOMEM on kmalloc failure.

Michael S. Tsirkin

author maintainer

2010-06-10 18:57 UTC #16 lore raw

In reply to Sridhar Samudrala

Quoted 52 lines · Sridhar Samudrala

Quoted 36 lines · Michael S. Tsirkin
virtio net will never try to overflow the TX ring, so the only reason add_buf may fail is out of memory. Thus, we can not stop the device until some request completes - there's no guarantee anything at all is outstanding.
Make the error message clearer as well: error here does not indicate queue full.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 drivers/net/virtio_net.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)
drivers/net/virtio_net.c +1 -7
index 85615a3..e48a06f 100644
@@ -563,7 +563,6 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct virtnet_info *vi = netdev_priv(dev);
 	int capacity;
 
-again:
 	/* Free up any pending old buffers before queueing new ones. */
 	free_old_xmit_skbs(vi);
 
@@ -572,12 +571,14 @@ again:
 
 	/* This can happen with OOM and indirect buffers. */
 	if (unlikely(capacity < 0)) {
-		netif_stop_queue(dev);
-		dev_warn(&dev->dev, "Unexpected full queue\n");
-		if (unlikely(!virtqueue_enable_cb(vi->svq))) {
-			virtqueue_disable_cb(vi->svq);
-			netif_start_queue(dev);
-			goto again;
+		if (net_ratelimit()) {
drivers/net/virtio_net.c again: · line 584
Quoted 10 lines
drivers/net/virtio_net.c +7 -0
+			if (likely(capacity == -ENOMEM))
+				dev_warn(&dev->dev,
+					 "TX queue failure: out of memory\n");
+			else
+				dev_warn(&dev->dev,
+					 "Unexpected TX queue failure: %d\n",
+					 capacity);
 		}
 		return NETDEV_TX_BUSY;
 	}
It is not clear to me how xmit_skb() can return -ENOMEM. xmit_skb() calls virtqueue_add_buf_gfp() which can return -ENOSPC. Even vring_add_indirect() doesn't return -ENOMEM on kmalloc failure.

Quoted 2 lines

Thanks
Sridhar

A separate patch fixes vring_add_indirect to return -ENOMEM. -ENOSPC really means ring is full so nothing to do and no need to retry.

Signature · 1 line

MST

Michael S. Tsirkin

author maintainer

2010-06-10 18:57 UTC #17 lore raw

In reply to Sridhar Samudrala

Quoted 52 lines · Sridhar Samudrala

Quoted 36 lines · Michael S. Tsirkin
virtio net will never try to overflow the TX ring, so the only reason add_buf may fail is out of memory. Thus, we can not stop the device until some request completes - there's no guarantee anything at all is outstanding.
Make the error message clearer as well: error here does not indicate queue full.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 drivers/net/virtio_net.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)
drivers/net/virtio_net.c +1 -7
index 85615a3..e48a06f 100644
@@ -563,7 +563,6 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct virtnet_info *vi = netdev_priv(dev);
 	int capacity;
 
-again:
 	/* Free up any pending old buffers before queueing new ones. */
 	free_old_xmit_skbs(vi);
 
@@ -572,12 +571,14 @@ again:
 
 	/* This can happen with OOM and indirect buffers. */
 	if (unlikely(capacity < 0)) {
-		netif_stop_queue(dev);
-		dev_warn(&dev->dev, "Unexpected full queue\n");
-		if (unlikely(!virtqueue_enable_cb(vi->svq))) {
-			virtqueue_disable_cb(vi->svq);
-			netif_start_queue(dev);
-			goto again;
+		if (net_ratelimit()) {
drivers/net/virtio_net.c again: · line 584
Quoted 10 lines
drivers/net/virtio_net.c +7 -0
+			if (likely(capacity == -ENOMEM))
+				dev_warn(&dev->dev,
+					 "TX queue failure: out of memory\n");
+			else
+				dev_warn(&dev->dev,
+					 "Unexpected TX queue failure: %d\n",
+					 capacity);
 		}
 		return NETDEV_TX_BUSY;
 	}
It is not clear to me how xmit_skb() can return -ENOMEM. xmit_skb() calls virtqueue_add_buf_gfp() which can return -ENOSPC. Even vring_add_indirect() doesn't return -ENOMEM on kmalloc failure.

Quoted 2 lines

Thanks
Sridhar

A separate patch fixes vring_add_indirect to return -ENOMEM. -ENOSPC really means ring is full so nothing to do and no need to retry.

Signature · 1 line

MST

Michael S. Tsirkin

author maintainer

2010-06-10 19:03 UTC #3 lore raw

In reply to Stephen Hemminger

Quoted 55 lines · Stephen Hemminger

Quoted 43 lines · On Thu, 10 Jun 2010 10:17:07 -0700 Sridhar Samudrala <[email protected]> wrote:
Quoted 42 lines · Michael S. Tsirkin
virtio net will never try to overflow the TX ring, so the only reason add_buf may fail is out of memory. Thus, we can not stop the device until some request completes - there's no guarantee anything at all is outstanding.
Make the error message clearer as well: error here does not indicate queue full.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 drivers/net/virtio_net.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)
drivers/net/virtio_net.c +7 -7
index 85615a3..e48a06f 100644
@@ -563,7 +563,6 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct virtnet_info *vi = netdev_priv(dev);
 	int capacity;
 
-again:
 	/* Free up any pending old buffers before queueing new ones. */
 	free_old_xmit_skbs(vi);
 
@@ -572,12 +571,14 @@ again:
 
 	/* This can happen with OOM and indirect buffers. */
 	if (unlikely(capacity < 0)) {
-		netif_stop_queue(dev);
-		dev_warn(&dev->dev, "Unexpected full queue\n");
-		if (unlikely(!virtqueue_enable_cb(vi->svq))) {
-			virtqueue_disable_cb(vi->svq);
-			netif_start_queue(dev);
-			goto again;
+		if (net_ratelimit()) {
+			if (likely(capacity == -ENOMEM))
+				dev_warn(&dev->dev,
+					 "TX queue failure: out of memory\n");
+			else
+				dev_warn(&dev->dev,
+					 "Unexpected TX queue failure: %d\n",
Quoted 8 lines
drivers/net/virtio_net.c again: · line 584
Quoted 4 lines
drivers/net/virtio_net.c +1 -0
+					 capacity);
 		}
 		return NETDEV_TX_BUSY;
 	}
It is not clear to me how xmit_skb() can return -ENOMEM. xmit_skb() calls virtqueue_add_buf_gfp() which can return -ENOSPC. Even vring_add_indirect() doesn't return -ENOMEM on kmalloc failure.

Quoted 7 lines

It makes more sense to have the device increment tx_droppped, and return NETDEV_TX_OK. Skip the message (or make it a pr_debug()). Network devices do not guarantee packet delivery, and if out of resources then holding more data in the
queue is going to hurt not help the situation.

Well, I only keep the existing behaviour around. The changes you propose would be 2.6.36 material.
I have it on my todo list to look for a way to test performance under GFP_ATOMIC failure scenario. Any suggestions?

Signature · 1 line

MST

Michael S. Tsirkin

author maintainer

2010-06-10 19:09 UTC #4 lore raw

In reply to Stephen Hemminger

Quoted 55 lines · Stephen Hemminger

Quoted 43 lines · On Thu, 10 Jun 2010 10:17:07 -0700 Sridhar Samudrala <[email protected]> wrote:
Quoted 42 lines · Michael S. Tsirkin
virtio net will never try to overflow the TX ring, so the only reason add_buf may fail is out of memory. Thus, we can not stop the device until some request completes - there's no guarantee anything at all is outstanding.
Make the error message clearer as well: error here does not indicate queue full.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 drivers/net/virtio_net.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)
drivers/net/virtio_net.c +7 -7
index 85615a3..e48a06f 100644
@@ -563,7 +563,6 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct virtnet_info *vi = netdev_priv(dev);
 	int capacity;
 
-again:
 	/* Free up any pending old buffers before queueing new ones. */
 	free_old_xmit_skbs(vi);
 
@@ -572,12 +571,14 @@ again:
 
 	/* This can happen with OOM and indirect buffers. */
 	if (unlikely(capacity < 0)) {
-		netif_stop_queue(dev);
-		dev_warn(&dev->dev, "Unexpected full queue\n");
-		if (unlikely(!virtqueue_enable_cb(vi->svq))) {
-			virtqueue_disable_cb(vi->svq);
-			netif_start_queue(dev);
-			goto again;
+		if (net_ratelimit()) {
+			if (likely(capacity == -ENOMEM))
+				dev_warn(&dev->dev,
+					 "TX queue failure: out of memory\n");
+			else
+				dev_warn(&dev->dev,
+					 "Unexpected TX queue failure: %d\n",
Quoted 8 lines
drivers/net/virtio_net.c again: · line 584
Quoted 4 lines
drivers/net/virtio_net.c +1 -0
+					 capacity);
 		}
 		return NETDEV_TX_BUSY;
 	}
It is not clear to me how xmit_skb() can return -ENOMEM. xmit_skb() calls virtqueue_add_buf_gfp() which can return -ENOSPC. Even vring_add_indirect() doesn't return -ENOMEM on kmalloc failure.

Quoted 7 lines

It makes more sense to have the device increment tx_droppped, and return NETDEV_TX_OK. Skip the message (or make it a pr_debug()). Network devices do not guarantee packet delivery, and if out of resources then holding more data in the
queue is going to hurt not help the situation.

Signature · 1 line

MST

Rusty Russell

2010-06-15 04:23 UTC #5 lore raw

Quoted 6 lines · Michael S. Tsirkin

Quoted 5 lines · Stephen Hemminger
It makes more sense to have the device increment tx_droppped, and return NETDEV_TX_OK. Skip the message (or make it a pr_debug()). Network devices do not guarantee packet delivery, and if out of resources then holding more data in the
queue is going to hurt not help the situation.

Yes, actually oom should be a ratelimited message and TX_OK, the other case is a "should never happen" logic bug which warrants an error and I don't care what it returns (whatever's easiest).

Please fix both at once since you're touching it.

Thanks,
Rusty.

Rusty Russell

2010-06-15 04:23 UTC #6 lore raw

In reply to Michael S. Tsirkin

Quoted 6 lines · Michael S. Tsirkin

Quoted 5 lines · Stephen Hemminger
It makes more sense to have the device increment tx_droppped, and return NETDEV_TX_OK. Skip the message (or make it a pr_debug()). Network devices do not guarantee packet delivery, and if out of resources then holding more data in the
queue is going to hurt not help the situation.

Yes, actually oom should be a ratelimited message and TX_OK, the other case is a "should never happen" logic bug which warrants an error and I don't care what it returns (whatever's easiest).

Please fix both at once since you're touching it.

Thanks,
Rusty.

Rusty Russell

2010-06-21 02:43 UTC #7 lore raw

In reply to Michael S. Tsirkin

Quoted 18 lines · Michael S. Tsirkin

Quoted 18 lines

(quoted diff) +8 -6

@@ -572,12 +571,14 @@ again:
 
 	/* This can happen with OOM and indirect buffers. */
 	if (unlikely(capacity < 0)) {
-		netif_stop_queue(dev);
-		dev_warn(&dev->dev, "Unexpected full queue\n");
-		if (unlikely(!virtqueue_enable_cb(vi->svq))) {
-			virtqueue_disable_cb(vi->svq);
-			netif_start_queue(dev);
-			goto again;
+		if (net_ratelimit()) {
+			if (likely(capacity == -ENOMEM))
+				dev_warn(&dev->dev,
+					 "TX queue failure: out of memory\n");
+			else
+				dev_warn(&dev->dev,
+					 "Unexpected TX queue failure: %d\n",
+					 capacity);

...

Quoted 2 lines

Well, I only keep the existing behaviour around.

Actually, it *does* change behavior, as the comment indicates. So let's fix the whole thing. AFAICT wth TX_BUSY we'll get called again RSN, and that's not really useful for OOM.

This is what I have:

Subject: virtio_net: fix oom handling on tx
Date: Thu, 10 Jun 2010 18:20:41 +0300
From: "Michael S. Tsirkin" <[email protected]>

Make the error message clearer as well: error here does not indicate queue full.

Signed-off-by Michael S. Tsirkin <[email protected]>

Signed-off-by Rusty Russell <[email protected]> (...and avoid TX_BUSY)

Cc [email protected]

---
 drivers/net/virtio_net.c |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

drivers/net/virtio_net.c +10 -8

@@ -562,7 +562,6 @@ static netdev_tx_t start_xmit(struct sk_
 	struct virtnet_info *vi = netdev_priv(dev);
 	int capacity;
 
-again:
 	/* Free up any pending old buffers before queueing new ones. */
 	free_old_xmit_skbs(vi);

@@ -571,14 +570,17 @@ again:
 
 	/* This can happen with OOM and indirect buffers. */
 	if (unlikely(capacity < 0)) {
-		netif_stop_queue(dev);
-		dev_warn(&dev->dev, "Unexpected full queue\n");
-		if (unlikely(!virtqueue_enable_cb(vi->svq))) {
-			virtqueue_disable_cb(vi->svq);
-			netif_start_queue(dev);
-			goto again;
+		if (net_ratelimit()) {
+			if (likely(capacity == -ENOMEM))
+				dev_warn(&dev->dev,
+					 "TX queue failure: out of memory\n");
+			else
+				dev_warn(&dev->dev,
+					 "Unexpected TX queue failure: %d\n",
+					 capacity);
 		}
-		return NETDEV_TX_BUSY;
+		kfree_skb(skb);
+		return NETDEV_TX_OK;
 	}
 	virtqueue_kick(vi->svq);

Rusty Russell

2010-06-21 02:43 UTC #8 lore raw

In reply to Michael S. Tsirkin

Quoted 18 lines · Michael S. Tsirkin

Quoted 18 lines

(quoted diff) +8 -6

@@ -572,12 +571,14 @@ again:
 
 	/* This can happen with OOM and indirect buffers. */
 	if (unlikely(capacity < 0)) {
-		netif_stop_queue(dev);
-		dev_warn(&dev->dev, "Unexpected full queue\n");
-		if (unlikely(!virtqueue_enable_cb(vi->svq))) {
-			virtqueue_disable_cb(vi->svq);
-			netif_start_queue(dev);
-			goto again;
+		if (net_ratelimit()) {
+			if (likely(capacity == -ENOMEM))
+				dev_warn(&dev->dev,
+					 "TX queue failure: out of memory\n");
+			else
+				dev_warn(&dev->dev,
+					 "Unexpected TX queue failure: %d\n",
+					 capacity);

...

Quoted 2 lines

Well, I only keep the existing behaviour around.

Actually, it *does* change behavior, as the comment indicates. So let's fix the whole thing. AFAICT wth TX_BUSY we'll get called again RSN, and that's not really useful for OOM.

This is what I have:

Subject: virtio_net: fix oom handling on tx
Date: Thu, 10 Jun 2010 18:20:41 +0300
From: "Michael S. Tsirkin" <[email protected]>

Make the error message clearer as well: error here does not indicate queue full.

Signed-off-by Michael S. Tsirkin <[email protected]>

Signed-off-by Rusty Russell <[email protected]> (...and avoid TX_BUSY)

Cc [email protected]

---
 drivers/net/virtio_net.c |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

drivers/net/virtio_net.c +10 -8

@@ -562,7 +562,6 @@ static netdev_tx_t start_xmit(struct sk_
 	struct virtnet_info *vi = netdev_priv(dev);
 	int capacity;
 
-again:
 	/* Free up any pending old buffers before queueing new ones. */
 	free_old_xmit_skbs(vi);

@@ -571,14 +570,17 @@ again:
 
 	/* This can happen with OOM and indirect buffers. */
 	if (unlikely(capacity < 0)) {
-		netif_stop_queue(dev);
-		dev_warn(&dev->dev, "Unexpected full queue\n");
-		if (unlikely(!virtqueue_enable_cb(vi->svq))) {
-			virtqueue_disable_cb(vi->svq);
-			netif_start_queue(dev);
-			goto again;
+		if (net_ratelimit()) {
+			if (likely(capacity == -ENOMEM))
+				dev_warn(&dev->dev,
+					 "TX queue failure: out of memory\n");
+			else
+				dev_warn(&dev->dev,
+					 "Unexpected TX queue failure: %d\n",
+					 capacity);
 		}
-		return NETDEV_TX_BUSY;
+		kfree_skb(skb);
+		return NETDEV_TX_OK;
 	}
 	virtqueue_kick(vi->svq);

Michael S. Tsirkin

author maintainer

2010-06-21 08:33 UTC #9 lore raw

Quoted 71 lines · Rusty Russell

Quoted 18 lines · Michael S. Tsirkin
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
drivers/net/virtio_net.c +8 -6
@@ -572,12 +571,14 @@ again:
 
 	/* This can happen with OOM and indirect buffers. */
 	if (unlikely(capacity < 0)) {
-		netif_stop_queue(dev);
-		dev_warn(&dev->dev, "Unexpected full queue\n");
-		if (unlikely(!virtqueue_enable_cb(vi->svq))) {
-			virtqueue_disable_cb(vi->svq);
-			netif_start_queue(dev);
-			goto again;
+		if (net_ratelimit()) {
+			if (likely(capacity == -ENOMEM))
+				dev_warn(&dev->dev,
+					 "TX queue failure: out of memory\n");
+			else
+				dev_warn(&dev->dev,
+					 "Unexpected TX queue failure: %d\n",
+					 capacity);
...
Quoted 2 lines
Well, I only keep the existing behaviour around.
Actually, it *does* change behavior, as the comment indicates. So let's fix the whole thing. AFAICT wth TX_BUSY we'll get called again RSN, and that's not really useful for OOM.
This is what I have:
Subject: virtio_net: fix oom handling on tx
Date: Thu, 10 Jun 2010 18:20:41 +0300
From: "Michael S. Tsirkin" <[email protected]>
virtio net will never try to overflow the TX ring, so the only reason add_buf may fail is out of memory. Thus, we can not stop the device until some request completes - there's no guarantee anything at all is outstanding.
Make the error message clearer as well: error here does not indicate queue full.
Signed-off-by Michael S. Tsirkin <[email protected]>
Signed-off-by Rusty Russell <[email protected]> (...and avoid TX_BUSY)
Cc [email protected]
---
 drivers/net/virtio_net.c |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)
drivers/net/virtio_net.c +2 -7
@@ -562,7 +562,6 @@ static netdev_tx_t start_xmit(struct sk_
 	struct virtnet_info *vi = netdev_priv(dev);
 	int capacity;
 
-again:
 	/* Free up any pending old buffers before queueing new ones. */
 	free_old_xmit_skbs(vi);
 
@@ -571,14 +570,17 @@ again:
 
 	/* This can happen with OOM and indirect buffers. */
 	if (unlikely(capacity < 0)) {
-		netif_stop_queue(dev);
-		dev_warn(&dev->dev, "Unexpected full queue\n");
-		if (unlikely(!virtqueue_enable_cb(vi->svq))) {
-			virtqueue_disable_cb(vi->svq);
-			netif_start_queue(dev);
-			goto again;
+		if (net_ratelimit()) {
+			if (likely(capacity == -ENOMEM))

drivers/net/virtio_net.c again: · line 583

Quoted 10 lines

drivers/net/virtio_net.c +8 -1

+				dev_warn(&dev->dev,
+					 "TX queue failure: out of memory\n");
+			else
+				dev_warn(&dev->dev,
+					 "Unexpected TX queue failure: %d\n",
+					 capacity);
 		}
-		return NETDEV_TX_BUSY;
+		kfree_skb(skb);
+		return NETDEV_TX_OK;

If we do so, let's increment the dropped counter and/or error counter?

Quoted 3 lines

drivers/net/virtio_net.c +0 -0
 	}
 	virtqueue_kick(vi->svq);
 

Michael S. Tsirkin

author maintainer

2010-06-21 08:38 UTC #10 lore raw

In reply to Rusty Russell

Quoted 71 lines · Rusty Russell

Quoted 18 lines · Michael S. Tsirkin
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
drivers/net/virtio_net.c +8 -6
@@ -572,12 +571,14 @@ again:
 
 	/* This can happen with OOM and indirect buffers. */
 	if (unlikely(capacity < 0)) {
-		netif_stop_queue(dev);
-		dev_warn(&dev->dev, "Unexpected full queue\n");
-		if (unlikely(!virtqueue_enable_cb(vi->svq))) {
-			virtqueue_disable_cb(vi->svq);
-			netif_start_queue(dev);
-			goto again;
+		if (net_ratelimit()) {
+			if (likely(capacity == -ENOMEM))
+				dev_warn(&dev->dev,
+					 "TX queue failure: out of memory\n");
+			else
+				dev_warn(&dev->dev,
+					 "Unexpected TX queue failure: %d\n",
+					 capacity);
...
Quoted 2 lines
Well, I only keep the existing behaviour around.
Actually, it *does* change behavior, as the comment indicates. So let's fix the whole thing. AFAICT wth TX_BUSY we'll get called again RSN, and that's not really useful for OOM.
This is what I have:
Subject: virtio_net: fix oom handling on tx
Date: Thu, 10 Jun 2010 18:20:41 +0300
From: "Michael S. Tsirkin" <[email protected]>
virtio net will never try to overflow the TX ring, so the only reason add_buf may fail is out of memory. Thus, we can not stop the device until some request completes - there's no guarantee anything at all is outstanding.
Make the error message clearer as well: error here does not indicate queue full.
Signed-off-by Michael S. Tsirkin <[email protected]>
Signed-off-by Rusty Russell <[email protected]> (...and avoid TX_BUSY)
Cc [email protected]
---
 drivers/net/virtio_net.c |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)
drivers/net/virtio_net.c +2 -7
@@ -562,7 +562,6 @@ static netdev_tx_t start_xmit(struct sk_
 	struct virtnet_info *vi = netdev_priv(dev);
 	int capacity;
 
-again:
 	/* Free up any pending old buffers before queueing new ones. */
 	free_old_xmit_skbs(vi);
 
@@ -571,14 +570,17 @@ again:
 
 	/* This can happen with OOM and indirect buffers. */
 	if (unlikely(capacity < 0)) {
-		netif_stop_queue(dev);
-		dev_warn(&dev->dev, "Unexpected full queue\n");
-		if (unlikely(!virtqueue_enable_cb(vi->svq))) {
-			virtqueue_disable_cb(vi->svq);
-			netif_start_queue(dev);
-			goto again;
+		if (net_ratelimit()) {
+			if (likely(capacity == -ENOMEM))

drivers/net/virtio_net.c again: · line 583

Quoted 10 lines

drivers/net/virtio_net.c +8 -1

+				dev_warn(&dev->dev,
+					 "TX queue failure: out of memory\n");
+			else
+				dev_warn(&dev->dev,
+					 "Unexpected TX queue failure: %d\n",
+					 capacity);
 		}
-		return NETDEV_TX_BUSY;
+		kfree_skb(skb);
+		return NETDEV_TX_OK;

If we do so, let's increment the dropped counter and/or error counter?

Quoted 3 lines

drivers/net/virtio_net.c +0 -0
 	}
 	virtqueue_kick(vi->svq);
 

Rusty Russell

2010-06-21 10:23 UTC #11 lore raw

Quoted 6 lines · Michael S. Tsirkin

Review
Quoted 3 lines · Rusty Russell
(quoted diff) +2 -1
-		return NETDEV_TX_BUSY;
+		kfree_skb(skb);
+		return NETDEV_TX_OK;
If we do so, let's increment the dropped counter and/or error counter?

Yep, here's the extra change:

drivers/net/virtio_net.c +4 -2

@@ -571,14 +571,16 @@ static netdev_tx_t start_xmit(struct sk_
 	/* This can happen with OOM and indirect buffers. */
 	if (unlikely(capacity < 0)) {
 		if (net_ratelimit()) {
-			if (likely(capacity == -ENOMEM))
+			if (likely(capacity == -ENOMEM)) {
 				dev_warn(&dev->dev,
 					 "TX queue failure: out of memory\n");
-			else
+			} else {
+				dev->stats.tx_fifo_errors++;
 				dev_warn(&dev->dev,
 					 "Unexpected TX queue failure: %d\n",
 					 capacity);
 		}
+		dev->stats.tx_dropped++;
 		kfree_skb(skb);
 		return NETDEV_TX_OK;
 	}

Rusty Russell

2010-06-21 10:23 UTC #14 lore raw

In reply to Michael S. Tsirkin

Quoted 6 lines · Michael S. Tsirkin

Review
Quoted 3 lines · Rusty Russell
(quoted diff) +2 -1
-		return NETDEV_TX_BUSY;
+		kfree_skb(skb);
+		return NETDEV_TX_OK;
If we do so, let's increment the dropped counter and/or error counter?

Yep, here's the extra change:

drivers/net/virtio_net.c +4 -2

@@ -571,14 +571,16 @@ static netdev_tx_t start_xmit(struct sk_
 	/* This can happen with OOM and indirect buffers. */
 	if (unlikely(capacity < 0)) {
 		if (net_ratelimit()) {
-			if (likely(capacity == -ENOMEM))
+			if (likely(capacity == -ENOMEM)) {
 				dev_warn(&dev->dev,
 					 "TX queue failure: out of memory\n");
-			else
+			} else {
+				dev->stats.tx_fifo_errors++;
 				dev_warn(&dev->dev,
 					 "Unexpected TX queue failure: %d\n",
 					 capacity);
 		}
+		dev->stats.tx_dropped++;
 		kfree_skb(skb);
 		return NETDEV_TX_OK;
 	}

Michael S. Tsirkin

author maintainer

2010-06-21 10:53 UTC #12 lore raw

In reply to Rusty Russell

Quoted 9 lines · Rusty Russell

Quoted 6 lines · Michael S. Tsirkin
Review
Quoted 3 lines · Rusty Russell
(quoted diff) +2 -1
-		return NETDEV_TX_BUSY;
+		kfree_skb(skb);
+		return NETDEV_TX_OK;
If we do so, let's increment the dropped counter and/or error counter?
Yep, here's the extra change:

Looks good to me.

Quoted 22 lines

drivers/net/virtio_net.c +4 -2

@@ -571,14 +571,16 @@ static netdev_tx_t start_xmit(struct sk_
 	/* This can happen with OOM and indirect buffers. */
 	if (unlikely(capacity < 0)) {
 		if (net_ratelimit()) {
-			if (likely(capacity == -ENOMEM))
+			if (likely(capacity == -ENOMEM)) {
 				dev_warn(&dev->dev,
 					 "TX queue failure: out of memory\n");
-			else
+			} else {
+				dev->stats.tx_fifo_errors++;
 				dev_warn(&dev->dev,
 					 "Unexpected TX queue failure: %d\n",
 					 capacity);
 		}
+		dev->stats.tx_dropped++;
 		kfree_skb(skb);
 		return NETDEV_TX_OK;
 	}

Michael S. Tsirkin

author maintainer

2010-06-21 10:59 UTC #13 lore raw

In reply to Rusty Russell

Quoted 9 lines · Rusty Russell

Quoted 6 lines · Michael S. Tsirkin
Review
Quoted 3 lines · Rusty Russell
(quoted diff) +2 -1
-		return NETDEV_TX_BUSY;
+		kfree_skb(skb);
+		return NETDEV_TX_OK;
If we do so, let's increment the dropped counter and/or error counter?
Yep, here's the extra change:

Looks good to me.

Quoted 22 lines

drivers/net/virtio_net.c +4 -2

@@ -571,14 +571,16 @@ static netdev_tx_t start_xmit(struct sk_
 	/* This can happen with OOM and indirect buffers. */
 	if (unlikely(capacity < 0)) {
 		if (net_ratelimit()) {
-			if (likely(capacity == -ENOMEM))
+			if (likely(capacity == -ENOMEM)) {
 				dev_warn(&dev->dev,
 					 "TX queue failure: out of memory\n");
-			else
+			} else {
+				dev->stats.tx_fifo_errors++;
 				dev_warn(&dev->dev,
 					 "Unexpected TX queue failure: %d\n",
 					 capacity);
 		}
+		dev->stats.tx_dropped++;
 		kfree_skb(skb);
 		return NETDEV_TX_OK;
 	}