From patchwork Tue Jun 2 14:04:28 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rusty Russell X-Patchwork-Id: 28007 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@bilbo.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from ozlabs.org (ozlabs.org [203.10.76.45]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "mx.ozlabs.org", Issuer "CA Cert Signing Authority" (verified OK)) by bilbo.ozlabs.org (Postfix) with ESMTPS id 4309AB707D for ; Wed, 3 Jun 2009 00:11:38 +1000 (EST) Received: by ozlabs.org (Postfix) id DCE43DDEE0; Wed, 3 Jun 2009 00:06:56 +1000 (EST) Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by ozlabs.org (Postfix) with ESMTP id 7E146DDDF6 for ; Wed, 3 Jun 2009 00:06:56 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754138AbZFBOGs (ORCPT ); Tue, 2 Jun 2009 10:06:48 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752950AbZFBOGr (ORCPT ); Tue, 2 Jun 2009 10:06:47 -0400 Received: from ozlabs.org ([203.10.76.45]:38614 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752795AbZFBOGq (ORCPT ); Tue, 2 Jun 2009 10:06:46 -0400 Received: from vivaldi.localnet (unknown [150.101.102.135]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPSA id 67C97DE874; Wed, 3 Jun 2009 00:04:35 +1000 (EST) From: Rusty Russell To: Mark McLoughlin Subject: Re: [PATCH 2/4] virtio_net: return NETDEV_TX_BUSY instead of queueing an extra skb. Date: Tue, 2 Jun 2009 23:34:28 +0930 User-Agent: KMail/1.11.2 (Linux/2.6.28-11-generic; KDE/4.2.2; i686; ; ) Cc: netdev@vger.kernel.org, virtualization@lists.linux-foundation.org, Herbert Xu References: <200905292346.04815.rusty@rustcorp.com.au> <1243930050.9146.53.camel@blaa> In-Reply-To: <1243930050.9146.53.camel@blaa> MIME-Version: 1.0 Content-Disposition: inline Message-Id: <200906022334.29428.rusty@rustcorp.com.au> Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Tue, 2 Jun 2009 05:37:30 pm Mark McLoughlin wrote: > On Fri, 2009-05-29 at 23:46 +0930, Rusty Russell wrote: > > This effectively reverts 99ffc696d10b28580fe93441d627cf290ac4484c > > "virtio: wean net driver off NETDEV_TX_BUSY". > > > > The complexity of queuing an skb (setting a tasklet to re-xmit) is > > questionable, > > It certainly adds some subtle complexities to start_xmit() > > > especially once we get rid of the other reason for the > > tasklet in the next patch. > > > > If the skb won't fit in the tx queue, just return NETDEV_TX_BUSY. It > > might be frowned upon, but it's common and not going away any time > > soon. > > > > Signed-off-by: Rusty Russell > > Cc: Herbert Xu > > --- > > drivers/net/virtio_net.c | 49 > > ++++++++++------------------------------------- 1 file changed, 11 > > insertions(+), 38 deletions(-) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > > > @@ -526,27 +517,14 @@ again: > > /* Free up any pending old buffers before queueing new ones. */ > > free_old_xmit_skbs(vi); > > > > - /* If we has a buffer left over from last time, send it now. */ > > - if (unlikely(vi->last_xmit_skb) && > > - xmit_skb(vi, vi->last_xmit_skb) != 0) > > - goto stop_queue; > > + /* Put new one in send queue and do transmit */ > > + __skb_queue_head(&vi->send, skb); > > + if (likely(xmit_skb(vi, skb) == 0)) { > > + vi->svq->vq_ops->kick(vi->svq); > > + return NETDEV_TX_OK; > > + } > > Hmm, is it okay to leave the skb on the send queue if we return > NETDEV_TX_BUSY? Certainly not. That's a bug. Incremental fix is: 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 diff -u b/drivers/net/virtio_net.c b/drivers/net/virtio_net.c --- b/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -524,8 +524,9 @@ return NETDEV_TX_OK; } - /* Ring too full for this packet. */ + /* Ring too full for this packet, remove it from queue again. */ pr_debug("%s: virtio not prepared to send\n", dev->name); + __skb_unlink(skb, &vi->send); netif_stop_queue(dev); /* Activate callback for using skbs: if this returns false it