* Re: [PATCH for-2.6.35] virtio_net: fix oom handling on tx [not found] <20100610152041.GA3480@redhat.com> @ 2010-06-10 17:17 ` Sridhar Samudrala 2010-06-10 17:46 ` Stephen Hemminger 2010-06-10 18:57 ` Michael S. Tsirkin 0 siblings, 2 replies; 10+ messages in thread From: Sridhar Samudrala @ 2010-06-10 17:17 UTC (permalink / raw) To: Michael S. Tsirkin Cc: virtualization, Rusty Russell, Jiri Pirko, Shirley Ma, netdev, linux-kernel 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 <mst@redhat.com> > --- > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH for-2.6.35] virtio_net: fix oom handling on tx 2010-06-10 17:17 ` [PATCH for-2.6.35] virtio_net: fix oom handling on tx Sridhar Samudrala @ 2010-06-10 17:46 ` Stephen Hemminger 2010-06-10 19:03 ` Michael S. Tsirkin 2010-06-10 18:57 ` Michael S. Tsirkin 1 sibling, 1 reply; 10+ messages in thread From: Stephen Hemminger @ 2010-06-10 17:46 UTC (permalink / raw) To: Sridhar Samudrala Cc: Michael S. Tsirkin, virtualization, Rusty Russell, Jiri Pirko, Shirley Ma, netdev, linux-kernel On Thu, 10 Jun 2010 10:17:07 -0700 Sridhar Samudrala <sri@us.ibm.com> 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 <mst@redhat.com> > > --- > > 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. -- ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH for-2.6.35] virtio_net: fix oom handling on tx 2010-06-10 17:46 ` Stephen Hemminger @ 2010-06-10 19:03 ` Michael S. Tsirkin 2010-06-15 4:23 ` Rusty Russell 2010-06-21 2:43 ` Rusty Russell 0 siblings, 2 replies; 10+ messages in thread From: Michael S. Tsirkin @ 2010-06-10 19:03 UTC (permalink / raw) To: Stephen Hemminger Cc: Sridhar Samudrala, virtualization, Rusty Russell, Jiri Pirko, Shirley Ma, netdev, linux-kernel On Thu, Jun 10, 2010 at 10:46:53AM -0700, Stephen Hemminger wrote: > On Thu, 10 Jun 2010 10:17:07 -0700 > Sridhar Samudrala <sri@us.ibm.com> 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 <mst@redhat.com> > > > --- > > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH for-2.6.35] virtio_net: fix oom handling on tx 2010-06-10 19:03 ` Michael S. Tsirkin @ 2010-06-15 4:23 ` Rusty Russell 2010-06-21 2:43 ` Rusty Russell 1 sibling, 0 replies; 10+ messages in thread From: Rusty Russell @ 2010-06-15 4:23 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Stephen Hemminger, Sridhar Samudrala, virtualization, Jiri Pirko, Shirley Ma, netdev, linux-kernel 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH for-2.6.35] virtio_net: fix oom handling on tx 2010-06-10 19:03 ` Michael S. Tsirkin 2010-06-15 4:23 ` Rusty Russell @ 2010-06-21 2:43 ` Rusty Russell 2010-06-21 8:33 ` Michael S. Tsirkin 1 sibling, 1 reply; 10+ messages in thread From: Rusty Russell @ 2010-06-21 2:43 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Stephen Hemminger, Sridhar Samudrala, virtualization, Jiri Pirko, Shirley Ma, netdev, linux-kernel 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" <mst@redhat.com> 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 <mst@redhat.com> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> (...and avoid TX_BUSY) Cc: stable@kernel.org --- 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); ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH for-2.6.35] virtio_net: fix oom handling on tx 2010-06-21 2:43 ` Rusty Russell @ 2010-06-21 8:33 ` Michael S. Tsirkin 2010-06-21 10:23 ` Rusty Russell 0 siblings, 1 reply; 10+ messages in thread From: Michael S. Tsirkin @ 2010-06-21 8:33 UTC (permalink / raw) To: Rusty Russell Cc: Stephen Hemminger, Sridhar Samudrala, virtualization, Jiri Pirko, Shirley Ma, netdev, linux-kernel 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" <mst@redhat.com> > > 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 <mst@redhat.com> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> (...and avoid TX_BUSY) > Cc: stable@kernel.org > --- > 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); > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH for-2.6.35] virtio_net: fix oom handling on tx 2010-06-21 8:33 ` Michael S. Tsirkin @ 2010-06-21 10:23 ` Rusty Russell 2010-06-21 10:53 ` Michael S. Tsirkin 0 siblings, 1 reply; 10+ messages in thread From: Rusty Russell @ 2010-06-21 10:23 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Stephen Hemminger, Sridhar Samudrala, virtualization, Jiri Pirko, Shirley Ma, netdev, linux-kernel 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; } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH for-2.6.35] virtio_net: fix oom handling on tx 2010-06-21 10:23 ` Rusty Russell @ 2010-06-21 10:53 ` Michael S. Tsirkin 0 siblings, 0 replies; 10+ messages in thread From: Michael S. Tsirkin @ 2010-06-21 10:53 UTC (permalink / raw) To: Rusty Russell Cc: Stephen Hemminger, Sridhar Samudrala, virtualization, Jiri Pirko, Shirley Ma, netdev, linux-kernel 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; > } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH for-2.6.35] virtio_net: fix oom handling on tx 2010-06-10 17:17 ` [PATCH for-2.6.35] virtio_net: fix oom handling on tx Sridhar Samudrala 2010-06-10 17:46 ` Stephen Hemminger @ 2010-06-10 18:57 ` Michael S. Tsirkin 1 sibling, 0 replies; 10+ messages in thread From: Michael S. Tsirkin @ 2010-06-10 18:57 UTC (permalink / raw) To: Sridhar Samudrala Cc: virtualization, Rusty Russell, Jiri Pirko, Shirley Ma, netdev, linux-kernel 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 <mst@redhat.com> > > --- > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH for-2.6.35] virtio_net: fix oom handling on tx @ 2010-06-10 15:20 Michael S. Tsirkin 0 siblings, 0 replies; 10+ messages in thread From: Michael S. Tsirkin @ 2010-06-10 15:20 UTC (permalink / raw) To: virtualization, Rusty Russell, Michael S. Tsirkin, Jiri Pirko, Shirley Ma 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 <mst@redhat.com> --- 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2010-06-21 10:59 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20100610152041.GA3480@redhat.com>
2010-06-10 17:17 ` [PATCH for-2.6.35] virtio_net: fix oom handling on tx Sridhar Samudrala
2010-06-10 17:46 ` Stephen Hemminger
2010-06-10 19:03 ` Michael S. Tsirkin
2010-06-15 4:23 ` Rusty Russell
2010-06-21 2:43 ` Rusty Russell
2010-06-21 8:33 ` Michael S. Tsirkin
2010-06-21 10:23 ` Rusty Russell
2010-06-21 10:53 ` Michael S. Tsirkin
2010-06-10 18:57 ` Michael S. Tsirkin
2010-06-10 15:20 Michael S. Tsirkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).