From: "Michael S. Tsirkin" <mst@redhat•com>
To: Rusty Russell <rusty@rustcorp•com.au>
Cc: Herbert Xu <herbert@gondor•hengli.com.au>,
netdev@vger•kernel.org,
virtualization@lists•linux-foundation.org, stable@kernel•org,
Bruce Rogers <brogers@novell•com>
Subject: Re: [PATCH] virtio_net: indicate oom when addbuf returns failure
Date: Sun, 6 Jun 2010 23:13:00 +0300 [thread overview]
Message-ID: <20100606201258.GA21196@redhat.com> (raw)
In-Reply-To: <201006041028.56798.rusty@rustcorp.com.au>
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 ..
> --- 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++) {
next prev parent reply other threads:[~2010-06-06 20:13 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-06-04 0:58 [PATCH] virtio_net: indicate oom when addbuf returns failure Rusty Russell
2010-06-04 16:05 ` [stable] " Greg KH
2010-06-06 20:13 ` Michael S. Tsirkin [this message]
2010-06-06 22:24 ` Herbert Xu
2010-06-07 2:24 ` Rusty Russell
2010-06-07 9:15 ` Michael S. Tsirkin
2010-06-07 2:17 ` Rusty Russell
2010-06-10 15:46 ` Bruce Rogers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20100606201258.GA21196@redhat.com \
--to=mst@redhat$(echo .)com \
--cc=brogers@novell$(echo .)com \
--cc=herbert@gondor$(echo .)hengli.com.au \
--cc=netdev@vger$(echo .)kernel.org \
--cc=rusty@rustcorp$(echo .)com.au \
--cc=stable@kernel$(echo .)org \
--cc=virtualization@lists$(echo .)linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox