Virtualization - Jun 2010 - [PATCH] virtio_net: indicate oom when addbuf returns failure

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 at novell.com>
> Signed-off-by: Rusty Russell <rusty at 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 ..

> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> 
> @@ -318,6 +318,7 @@ static bool try_fill_recv_maxbufs(struct
>                         skb_unlink(skb, &vi->recv);
>                         trim_pages(vi, skb);
>                         kfree_skb(skb);
> +                       oom = true;
>                         break;
>                 }
>                 vi->num++;
> @@ -368,6 +369,7 @@ static bool try_fill_recv(struct virtnet
>                 if (err < 0) {
>                         skb_unlink(skb, &vi->recv);
>                         kfree_skb(skb);
> +                       oom = true;
>                         break;
>                 }
>                 vi->num++;

Possibly the right thing to do is to
1. handle ENOMEM specially
2. fix add_buf to return ENOMEM on error

Something like the below for upstream (warning: compile
tested only) and a similar one later for stable:


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++) {

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 at 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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert at gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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 at novell.com>
> > Signed-off-by: Rusty Russell <rusty at 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.

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 at 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.

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 at 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 at gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

>>> On 6/6/2010 at 02:13 PM, "Michael S. Tsirkin" <mst at
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 at novell.com>
>> Signed-off-by: Rusty Russell <rusty at 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