From patchwork Sun Jun 6 20:13:00 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Michael S. Tsirkin" X-Patchwork-Id: 54821 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id C115EB7D85 for ; Mon, 7 Jun 2010 06:18:41 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754106Ab0FFUSe (ORCPT ); Sun, 6 Jun 2010 16:18:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:10304 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751599Ab0FFUSd (ORCPT ); Sun, 6 Jun 2010 16:18:33 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o56KHAhq004606 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Sun, 6 Jun 2010 16:17:11 -0400 Received: from redhat.com (vpn-6-112.tlv.redhat.com [10.35.6.112]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with SMTP id o56KH7Qd018147; Sun, 6 Jun 2010 16:17:08 -0400 Date: Sun, 6 Jun 2010 23:13:00 +0300 From: "Michael S. Tsirkin" To: Rusty Russell Cc: stable@kernel.org, Bruce Rogers , Herbert Xu , netdev@vger.kernel.org, virtualization@lists.linux-foundation.org Subject: Re: [PATCH] virtio_net: indicate oom when addbuf returns failure Message-ID: <20100606201258.GA21196@redhat.com> References: <201006041028.56798.rusty@rustcorp.com.au> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <201006041028.56798.rusty@rustcorp.com.au> User-Agent: Mutt/1.5.19 (2009-01-05) X-Scanned-By: MIMEDefang 2.67 on 10.5.11.11 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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 > Signed-off-by: Rusty Russell 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: --- 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++) {