                        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++;

Greg KH

2010-06-04 16:06 UTC #1 lore raw

Quoted 2 lines · Rusty Russell

This patch is a subset of an already upstream patch, but this portion is useful in earlier releases.

What is the git commit id of that upstream patch?

thanks,

greg k-h

Michael S. Tsirkin

maintainer

2010-06-06 20:13 UTC #2 lore raw

In reply to Rusty Russell

Quoted 9 lines · Rusty Russell

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 <[email protected]>
Signed-off-by Rusty Russell <[email protected]>

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

Quoted 12 lines

drivers/net/virtio_net.c +1 -0

@@ -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++;

drivers/net/virtio_net.c static bool try_fill_recv(struct virtnet · line 375

Quoted 7 lines

drivers/net/virtio_net.c +1 -0

@@ -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:

drivers/net/virtio_net.c +3 -4

index 06c30df..85615a3 100644
@@ -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))

drivers/virtio/virtio_ring.c +1 -1

index f9e6fbb..3c7f10a 100644
@@ -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++) {

Herbert Xu

2010-06-06 22:24 UTC #3 lore raw

Quoted 5 lines · Michael S. Tsirkin

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 <[email protected]>
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.

Quoted 3 lines

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,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Herbert Xu

2010-06-06 22:24 UTC #4 lore raw

In reply to Michael S. Tsirkin

Quoted 5 lines · Michael S. Tsirkin

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 <[email protected]>
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.

Quoted 3 lines

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

Thanks,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Rusty Russell

author

2010-06-07 02:24 UTC #5 lore raw

Quoted 20 lines · Herbert Xu

Quoted 5 lines · Michael S. Tsirkin
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 <[email protected]>
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.

Quoted 8 lines

Quoted 3 lines
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.

Rusty Russell

author

2010-06-07 02:24 UTC #6 lore raw

In reply to Herbert Xu

Quoted 20 lines · Herbert Xu

Quoted 5 lines · Michael S. Tsirkin
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 <[email protected]>
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.

Quoted 8 lines

Quoted 3 lines
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.

Michael S. Tsirkin

maintainer

2010-06-07 09:15 UTC #7 lore raw

In reply to Herbert Xu

Quoted 20 lines · Herbert Xu

Quoted 5 lines · Michael S. Tsirkin
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 <[email protected]>
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.

Quoted 8 lines

Quoted 3 lines
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?

Quoted 6 lines

Thanks,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Rusty Russell

author

2010-06-07 02:17 UTC #8 lore raw

In reply to Michael S. Tsirkin

Quoted 16 lines · Michael S. Tsirkin

Quoted 9 lines · Rusty Russell
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 <[email protected]>
Signed-off-by Rusty Russell <[email protected]>
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.

Quoted 6 lines

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.

Rusty Russell

author

2010-06-07 02:17 UTC #9 lore raw

In reply to Michael S. Tsirkin

Quoted 16 lines · Michael S. Tsirkin

Quoted 9 lines · Rusty Russell
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 <[email protected]>
Signed-off-by Rusty Russell <[email protected]>
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.

Quoted 6 lines

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.

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.

Bruce Rogers

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

In reply to Michael S. Tsirkin

Quoted 17 lines

Quoted 1 line
Quoted 1 line · Michael S. Tsirkin
Quoted 9 lines · Rusty Russell
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 <[email protected]>
Signed-off-by Rusty Russell <[email protected]>
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.

Quoted 7 lines

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

Bruce Rogers

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

In reply to Michael S. Tsirkin

Quoted 17 lines

Quoted 1 line
Quoted 1 line · Michael S. Tsirkin
Quoted 9 lines · Rusty Russell
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 <[email protected]>
Signed-off-by Rusty Russell <[email protected]>
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.

Quoted 7 lines

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