From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id F114656A9 for ; Wed, 10 Feb 2016 16:07:04 +0100 (CET) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga102.fm.intel.com with ESMTP; 10 Feb 2016 07:07:03 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,426,1449561600"; d="scan'208";a="881280514" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.208.159]) by orsmga001.jf.intel.com with SMTP; 10 Feb 2016 07:07:01 -0800 Received: by (sSMTP sendmail emulation); Wed, 10 Feb 2016 15:07:01 +0025 Date: Wed, 10 Feb 2016 15:07:01 +0000 From: Bruce Richardson To: "Xie, Huawei" Message-ID: <20160210150700.GA14232@bricha3-MOBL3> References: <1447407013-6986-1-git-send-email-tkiely@brocade.com> <567299E9.60000@brocade.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Organization: Intel Shannon Ltd. User-Agent: Mutt/1.5.23 (2014-03-12) Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 10 Feb 2016 15:07:05 -0000 On Tue, Jan 05, 2016 at 07:13:04AM +0000, Xie, Huawei wrote: > On 12/17/2015 7:18 PM, Tom Kiely wrote: > > > > > > On 11/25/2015 05:32 PM, Xie, Huawei wrote: > >> On 11/13/2015 5:33 PM, Tom Kiely wrote: > >>> If all rx descriptors are processed while transient > >>> mbuf exhaustion is present, the rx ring ends up with > >>> no available descriptors. Thus no packets are received > >>> on that ring. Since descriptor refill is performed post > >>> rx descriptor processing, in this case no refill is > >>> ever subsequently performed resulting in permanent rx > >>> traffic drop. > >>> > >>> Signed-off-by: Tom Kiely > >>> --- > >>> drivers/net/virtio/virtio_rxtx.c | 6 ++++-- > >>> 1 file changed, 4 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/net/virtio/virtio_rxtx.c > >>> b/drivers/net/virtio/virtio_rxtx.c > >>> index 5770fa2..a95e234 100644 > >>> --- a/drivers/net/virtio/virtio_rxtx.c > >>> +++ b/drivers/net/virtio/virtio_rxtx.c > >>> @@ -586,7 +586,8 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf > >>> **rx_pkts, uint16_t nb_pkts) > >>> if (likely(num > DESC_PER_CACHELINE)) > >>> num = num - ((rxvq->vq_used_cons_idx + num) % > >>> DESC_PER_CACHELINE); > >>> - if (num == 0) > >>> + /* Refill free descriptors even if no pkts recvd */ > >>> + if (num == 0 && virtqueue_full(rxvq)) > >> Should the return condition be that no used buffers and we have avail > >> descs in avail ring, i.e, > >> num == 0 && rxvq->vq_free_cnt != rxvq->vq_nentries > >> > >> rather than > >> num == 0 && rxvq->vq_free_cnt == 0 > > Yes we could do that but I don't see a good reason to wait until the > > vq_free_cnt == vq_nentries > > before attempting the refill. The existing code will attempt refill > > even if only 1 packet was received > > and the free count is small. To me it seems safer to extend that to > > try refill even if no packet was received > > but the free count is non-zero. > The existing code attempt to refill only if 1 packet was received. > > If we want to refill even no packet was received, then the strict > condition should be > num == 0 && rxvq->vq_free_cnt != rxvq->vq_nentries > > The safer condition, what you want to use, should be > num == 0 && !virtqueue_full(...) > rather than > num == 0 && virtqueue_full(...) > > We could simplify things a bit, just remove this check, if the following > receiving code already takes care of the "num == 0" condition. > > I find virtqueue_full is confusing, maybe we could change it to some > other meaningful name. > > > > > Tom > > Ping. Tom and Huawei, what is the status of this patch? Will there be a V2? /Bruce