From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from new2-smtp.messagingengine.com (new2-smtp.messagingengine.com [66.111.4.224]) by dpdk.org (Postfix) with ESMTP id ACB9E9B6B; Thu, 19 Oct 2017 15:53:24 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailnew.nyi.internal (Postfix) with ESMTP id C220AC13; Thu, 19 Oct 2017 09:53:23 -0400 (EDT) Received: from frontend2 ([10.202.2.161]) by compute1.internal (MEProxy); Thu, 19 Oct 2017 09:53:23 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fridaylinux.org; h=cc:content-type:date:from:in-reply-to:message-id:mime-version :references:subject:to:x-me-sender:x-me-sender:x-sasl-enc; s= fm1; bh=T3liDjEcN0KehhzarYOXR8yFRb5uLoKlZSKxHiL7L+4=; b=DU6uDfMv b5M9wi4GORVrOQRGnrT8ZPa7gFl8gtvOCBh2VCTTPJvF9lIogHamkN3ZNgOGlCu6 IlI9sas/yVU5qRludrotubKxHhQHWTirYBRDl2qm1Q1SMgzDd+GKXeGbmPKFSvhf k6Pfe+0FkC/9gcyr4UTYyMTx9HcE1m1hVx8hKsZGCRXw+XV6yMCkLzuxb5pr6qnI C1ghVSuunYlmEV2H5YfLWIqHYLGl/R0dl9IfuqKBwPPxYXyJNCKXRc+Vqd7V/ciT jTGGCMtUFuDo7hn4xtpLv03ZlkXr/RyC7gqOtwf4/6ZpQ1i88Ow+h+VoAjCaX9oM /tWc1mZmpBjLTg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-sender :x-me-sender:x-sasl-enc; s=fm1; bh=T3liDjEcN0KehhzarYOXR8yFRb5uL oKlZSKxHiL7L+4=; b=pzeT646/9QCqjR/OgrZgKdCbgjfVIxJNjDMfBdeV93D2q uH9iBQXGckWQn4GCMELHMbeLgNihyDqdYNKc5GlUtgPikgTz8usIy6pLHMklHId9 gV3sc9UlpxNsCfyHYkKo2p3kzM9jcBv7HWpke4ftTrN/wAmcOpG8x3xp9c6ttLb2 2rPCFMoJF8MSFqhzzRiMt52SgnNYb90UQyCRKI/TZy0TdmhxhzBooUashb5GLxU8 evTgRg9T7/RKNFc9FpJya90k3VSBs/RnsDBncdKktdj+Qpr3uV+5mlpsTd09r/j+ bMLsC5zH/m1TQXj2cTVhriCMVRQ+s3Jl7MmosDtCQ== X-ME-Sender: Received: from yliu-home (unknown [124.79.168.36]) by mail.messagingengine.com (Postfix) with ESMTPA id 79AF624A6F; Thu, 19 Oct 2017 09:53:21 -0400 (EDT) Date: Thu, 19 Oct 2017 21:53:14 +0800 From: Yuanhan Liu To: Tiwei Bie Cc: Jens Freimann , dev@dpdk.org, maxime.coquelin@redhat.com, stable@dpdk.org Message-ID: <20171019135314.GA1545@yliu-home> References: <20170829082601.30349-1-tiwei.bie@intel.com> <20170830091306.a5whdd7xrgb4jbtn@dhcp-192-218.str.redhat.com> <20170830102423.GA15019@debian-ZGViaWFuCg> <20170901062646.iek4azi6nez7vyrr@dhcp-192-218.str.redhat.com> <20170901071426.GA25578@debian-ZGViaWFuCg> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170901071426.GA25578@debian-ZGViaWFuCg> User-Agent: Mutt/1.5.24 (2015-08-30) Subject: Re: [dpdk-dev] [PATCH] net/virtio: fix an incorrect behavior of device stop/start X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 Oct 2017 13:53:25 -0000 On Fri, Sep 01, 2017 at 03:14:26PM +0800, Tiwei Bie wrote: > > > > On Tue, Aug 29, 2017 at 04:26:01PM +0800, Tiwei Bie wrote: > > > > > After starting a device, the driver shouldn't deliver the > > > > > packets that already existed in the device before it is > > > > > started to the applications. Otherwise? I'm assuming you fixed a real issue? If so, it'd be better if you can add a bit info about the issue. > This patch fixes this issue > > > > > by flushing the Rx queues when starting the device. > > > > > > > > > > Fixes: a85786dc816f ("virtio: fix states handling during initialization") ... > > > > > @@ -1737,6 +1737,12 @@ virtio_dev_start(struct rte_eth_dev *dev) > > > > > } > > > > > } > > > > > > > > > > + /* Flush the packets in Rx queues. */ > > > > > + for (i = 0; i < dev->data->nb_rx_queues; i++) { > > > > > + rxvq = dev->data->rx_queues[i]; > > > > > + virtqueue_flush(rxvq->vq); > > > > > + } > > > > > + > > > > > > > > A little bit further down is a for loop going over rx queues calling > > > > notify. Could we flush directly before the notify and save the > > > > additional loop? > > > > > > > > > > I saw there is also another `for' loop to dump the Rx queues. > > > And I think it makes the code more readable to flush the Rx > > > queues in a separate `for' loop too. Besides, this function > > > isn't performance critical. So I didn't combine them into one > > > `for' loop. > > > > To me code is better readable when it is concise, so I'd still vote for > > combining the loops if its logically equivalent. > > > > On the other hand I think this should be fixed soon, so > > > > Reviewed-by: Jens Freimann > > > > Thank you! :-) > > It's not a big deal. I'd like to leave it up to the maintainers. > They can make the decision when applying the patch. I agree with Jens here. We already have too many for loops in this function. Let's not add yet another one. Besides that, the VIRTQUEU_DUMP loop probably should also be removed and more it to the notify loop. --yliu