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(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 85615a3..e48a06f 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -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;
}
--
1.7.1.12.g42b7f
On Thu, 2010-06-10 at 18:20 +0300, Michael S. Tsirkin wrote:
> 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(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 85615a3..e48a06f 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -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;
> }
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
On Thu, 10 Jun 2010 10:17:07 -0700
Sridhar Samudrala <[email protected]> wrote:
> On Thu, 2010-06-10 at 18:20 +0300, Michael S. Tsirkin wrote:
> > 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(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 85615a3..e48a06f 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -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;
> > }
>
> 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.
--
On Thu, Jun 10, 2010 at 10:17:07AM -0700, Sridhar Samudrala wrote:
> On Thu, 2010-06-10 at 18:20 +0300, Michael S. Tsirkin wrote:
> > 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(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 85615a3..e48a06f 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -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;
> > }
>
> 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
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.
--
MST
On Thu, Jun 10, 2010 at 10:46:53AM -0700, Stephen Hemminger wrote:
> On Thu, 10 Jun 2010 10:17:07 -0700
> Sridhar Samudrala <[email protected]> wrote:
>
> > On Thu, 2010-06-10 at 18:20 +0300, Michael S. Tsirkin wrote:
> > > 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(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 85615a3..e48a06f 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -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;
> > > }
> >
> > 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.
>
> --
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?
--
MST
On Fri, 11 Jun 2010 04:33:43 am Michael S. Tsirkin wrote:
> On Thu, Jun 10, 2010 at 10:46:53AM -0700, Stephen Hemminger wrote:
> > 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.
On Fri, 11 Jun 2010 04:33:43 am Michael S. Tsirkin wrote:
> > > > @@ -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);
...
>
> 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(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -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);
On Mon, Jun 21, 2010 at 12:13:49PM +0930, Rusty Russell wrote:
> On Fri, 11 Jun 2010 04:33:43 am Michael S. Tsirkin wrote:
> > > > > @@ -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);
> ...
> >
> > 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(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -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;
If we do so, let's increment the dropped counter and/or error counter?
> }
> virtqueue_kick(vi->svq);
>
On Mon, 21 Jun 2010 06:03:16 pm Michael S. Tsirkin wrote:
> On Mon, Jun 21, 2010 at 12:13:49PM +0930, Rusty Russell wrote:
> > - 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:
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -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;
}
On Mon, Jun 21, 2010 at 07:53:43PM +0930, Rusty Russell wrote:
> On Mon, 21 Jun 2010 06:03:16 pm Michael S. Tsirkin wrote:
> > On Mon, Jun 21, 2010 at 12:13:49PM +0930, Rusty Russell wrote:
> > > - 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.
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -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;
> }