Message ID | 20100606201258.GA21196@redhat.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
On Sun, Jun 06, 2010 at 11:13:00PM +0300, Michael S. Tsirkin wrote: > > Actually this code looks strange: > Note that add_buf inicates out of memory > condition with a positive return value, and ring full > (which is not an error!) with -ENOSPC. Indeed, this ultimately came from commit 9ab86bbcf8be755256f0a5e994e0b38af6b4d399 Author: Shirley Ma <mashirle@us.ibm.com> Date: Fri Jan 29 03:20:04 2010 +0000 virtio_net: Defer skb allocation in receive path Date: Wed, 13 Jan 2010 12:53:38 -0800 (Greg, please don't apply this even though I've just given you the upstream commit ID that you were asking for :) where it confuses a memory allocation error with an add_buf failure. > Possibly the right thing to do is to > 1. handle ENOMEM specially > 2. fix add_buf to return ENOMEM on error I think we should make it so that only a memory allocation error is returned as before. There is no need for returning the add_buf error unless add_buf is now doing an allocation itself that needs to be retried. Thanks,
On Mon, 7 Jun 2010 05:43:00 am Michael S. Tsirkin wrote: > On Fri, Jun 04, 2010 at 10:28:56AM +0930, Rusty Russell wrote: > > This patch is a subset of an already upstream patch, but this portion > > is useful in earlier releases. > > > > Please consider for the 2.6.32 and 2.6.33 stable trees. > > > > If the add_buf operation fails, indicate failure to the caller. > > > > Signed-off-by: Bruce Rogers <brogers@novell.com> > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > > Actually this code looks strange: > Note that add_buf inicates out of memory > condition with a positive return value, and ring full > (which is not an error!) with -ENOSPC. > > So it seems that this patch (and upstream code) will fill > the ring and then end up setting oom = true and rescheduling the work > forever. And I suspect I actually saw this at some point > on one of my systems: observed BW would drop > with high CPU usage until reboot. > Can't reproduce it now anymore .. I thought that at first too, but it's subtler than that. When the ring is exactly filled, err = 0. With mergeable bufs and small bufs that's the. With big buffers, it's probably not, and the code will indeed respond as if always out of memory, always trying to refill. Our current code has the same error. Probably a combination of noone using big buffers, and noone noticing one timer every 1/2 second. Want to fix that properly? And comment it? :) Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 7 Jun 2010 07:54:41 am Herbert Xu wrote: > On Sun, Jun 06, 2010 at 11:13:00PM +0300, Michael S. Tsirkin wrote: > > > > Actually this code looks strange: > > Note that add_buf inicates out of memory > > condition with a positive return value, and ring full > > (which is not an error!) with -ENOSPC. > > Indeed, this ultimately came from > > commit 9ab86bbcf8be755256f0a5e994e0b38af6b4d399 > Author: Shirley Ma <mashirle@us.ibm.com> > Date: Fri Jan 29 03:20:04 2010 +0000 > > virtio_net: Defer skb allocation in receive path Date: Wed, 13 Jan 2010 12:53:38 -0800 > > (Greg, please don't apply this even though I've just given you > the upstream commit ID that you were asking for :) > > where it confuses a memory allocation error with an add_buf failure. > > > Possibly the right thing to do is to > > 1. handle ENOMEM specially > > 2. fix add_buf to return ENOMEM on error > > I think we should make it so that only a memory allocation error > is returned as before. There is no need for returning the add_buf > error unless add_buf is now doing an allocation itself that needs > to be retried. With indirect bufs, this is indeed the case. The code works except for the bigpackets !mergable case, but should be clarified anyway. See my other mail... Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 07, 2010 at 08:24:41AM +1000, Herbert Xu wrote: > On Sun, Jun 06, 2010 at 11:13:00PM +0300, Michael S. Tsirkin wrote: > > > > Actually this code looks strange: > > Note that add_buf inicates out of memory > > condition with a positive return value, and ring full > > (which is not an error!) with -ENOSPC. > > Indeed, this ultimately came from > > commit 9ab86bbcf8be755256f0a5e994e0b38af6b4d399 > Author: Shirley Ma <mashirle@us.ibm.com> > Date: Fri Jan 29 03:20:04 2010 +0000 > > virtio_net: Defer skb allocation in receive path Date: Wed, 13 Jan 2010 12:53:38 -0800 > > (Greg, please don't apply this even though I've just given you > the upstream commit ID that you were asking for :) > > where it confuses a memory allocation error with an add_buf failure. > > > Possibly the right thing to do is to > > 1. handle ENOMEM specially > > 2. fix add_buf to return ENOMEM on error > > I think we should make it so that only a memory allocation error > is returned as before. There is no need for returning the add_buf > error unless add_buf is now doing an allocation itself that needs > to be retried. That's what my patch did, right? Ack it? > Thanks, > -- > Visit Openswan at http://www.openswan.org/ > Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> > Home Page: http://gondor.apana.org.au/~herbert/ > PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> On 6/6/2010 at 02:13 PM, "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Fri, Jun 04, 2010 at 10:28:56AM +0930, Rusty Russell wrote: >> This patch is a subset of an already upstream patch, but this portion >> is useful in earlier releases. >> >> Please consider for the 2.6.32 and 2.6.33 stable trees. >> >> If the add_buf operation fails, indicate failure to the caller. >> >> Signed-off-by: Bruce Rogers <brogers@novell.com> >> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > > Actually this code looks strange: > Note that add_buf inicates out of memory > condition with a positive return value, and ring full > (which is not an error!) with -ENOSPC. > > So it seems that this patch (and upstream code) will fill > the ring and then end up setting oom = true and rescheduling the work > forever. And I suspect I actually saw this at some point > on one of my systems: observed BW would drop > with high CPU usage until reboot. > Can't reproduce it now anymore .. > Thanks for looking into this. We've decided not to use this patch, since it is not a part of the solution we need. The upstream patch from whence it came at first glance seemed useful, but is problematic as you point out. We've retested without that patch and are still getting good results. Bruce -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 06c30df..85615a3 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -416,7 +416,7 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi, gfp_t gfp) static bool try_fill_recv(struct virtnet_info *vi, gfp_t gfp) { int err; - bool oom = false; + bool oom; do { if (vi->mergeable_rx_bufs) @@ -426,10 +426,9 @@ static bool try_fill_recv(struct virtnet_info *vi, gfp_t gfp) else err = add_recvbuf_small(vi, gfp); - if (err < 0) { - oom = true; + oom = err == -ENOMEM; + if (err < 0) break; - } ++vi->num; } while (err > 0); if (unlikely(vi->num > vi->max)) diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index f9e6fbb..3c7f10a 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -119,7 +119,7 @@ static int vring_add_indirect(struct vring_virtqueue *vq, desc = kmalloc((out + in) * sizeof(struct vring_desc), gfp); if (!desc) - return vq->vring.num; + return -ENOMEM; /* Transfer entries from the sg list into the indirect page */ for (i = 0; i < out; i++) {