From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 81729919F for ; Tue, 5 Jan 2016 08:13:29 +0100 (CET) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga101.jf.intel.com with ESMTP; 04 Jan 2016 23:13:28 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,524,1444719600"; d="scan'208";a="853711366" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by orsmga001.jf.intel.com with ESMTP; 04 Jan 2016 23:13:28 -0800 Received: from fmsmsx113.amr.corp.intel.com (10.18.116.7) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.248.2; Mon, 4 Jan 2016 23:13:27 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX113.amr.corp.intel.com (10.18.116.7) with Microsoft SMTP Server (TLS) id 14.3.248.2; Mon, 4 Jan 2016 23:13:27 -0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.111]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.220]) with mapi id 14.03.0248.002; Tue, 5 Jan 2016 15:13:05 +0800 From: "Xie, Huawei" To: Tom Kiely , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation Thread-Index: AdEnp1J61clOw+YwQ8y+r2GXRAF+4Q== Date: Tue, 5 Jan 2016 07:13:04 +0000 Message-ID: References: <1447407013-6986-1-git-send-email-tkiely@brocade.com> <567299E9.60000@brocade.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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: Tue, 05 Jan 2016 07:13:30 -0000 On 12/17/2015 7:18 PM, Tom Kiely wrote:=0A= >=0A= >=0A= > On 11/25/2015 05:32 PM, Xie, Huawei wrote:=0A= >> On 11/13/2015 5:33 PM, Tom Kiely wrote:=0A= >>> If all rx descriptors are processed while transient=0A= >>> mbuf exhaustion is present, the rx ring ends up with=0A= >>> no available descriptors. Thus no packets are received=0A= >>> on that ring. Since descriptor refill is performed post=0A= >>> rx descriptor processing, in this case no refill is=0A= >>> ever subsequently performed resulting in permanent rx=0A= >>> traffic drop.=0A= >>>=0A= >>> Signed-off-by: Tom Kiely =0A= >>> ---=0A= >>> drivers/net/virtio/virtio_rxtx.c | 6 ++++--=0A= >>> 1 file changed, 4 insertions(+), 2 deletions(-)=0A= >>>=0A= >>> diff --git a/drivers/net/virtio/virtio_rxtx.c=0A= >>> b/drivers/net/virtio/virtio_rxtx.c=0A= >>> index 5770fa2..a95e234 100644=0A= >>> --- a/drivers/net/virtio/virtio_rxtx.c=0A= >>> +++ b/drivers/net/virtio/virtio_rxtx.c=0A= >>> @@ -586,7 +586,8 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf=0A= >>> **rx_pkts, uint16_t nb_pkts)=0A= >>> if (likely(num > DESC_PER_CACHELINE))=0A= >>> num =3D num - ((rxvq->vq_used_cons_idx + num) %=0A= >>> DESC_PER_CACHELINE);=0A= >>> - if (num =3D=3D 0)=0A= >>> + /* Refill free descriptors even if no pkts recvd */=0A= >>> + if (num =3D=3D 0 && virtqueue_full(rxvq))=0A= >> Should the return condition be that no used buffers and we have avail=0A= >> descs in avail ring, i.e,=0A= >> num =3D=3D 0 && rxvq->vq_free_cnt !=3D rxvq->vq_nentries=0A= >>=0A= >> rather than=0A= >> num =3D=3D 0 && rxvq->vq_free_cnt =3D=3D 0=0A= > Yes we could do that but I don't see a good reason to wait until the=0A= > vq_free_cnt =3D=3D vq_nentries=0A= > before attempting the refill. The existing code will attempt refill=0A= > even if only 1 packet was received=0A= > and the free count is small. To me it seems safer to extend that to=0A= > try refill even if no packet was received=0A= > but the free count is non-zero.=0A= The existing code attempt to refill only if 1 packet was received.=0A= =0A= If we want to refill even no packet was received, then the strict=0A= condition should be=0A= num =3D=3D 0 && rxvq->vq_free_cnt !=3D rxvq->vq_nentries=0A= =0A= The safer condition, what you want to use, should be=0A= num =3D=3D 0 && !virtqueue_full(...)=0A= rather than=0A= num =3D=3D 0 && virtqueue_full(...)=0A= =0A= We could simplify things a bit, just remove this check, if the following=0A= receiving code already takes care of the "num =3D=3D 0" condition.=0A= =0A= I find virtqueue_full is confusing, maybe we could change it to some=0A= other meaningful name.=0A= =0A= >=0A= > Tom=0A= >=0A= >>> return 0;=0A= >>> num =3D virtqueue_dequeue_burst_rx(rxvq, rcv_pkts, len, num);= =0A= >>> @@ -683,7 +684,8 @@ virtio_recv_mergeable_pkts(void *rx_queue,=0A= >>> virtio_rmb();=0A= >>> - if (nb_used =3D=3D 0)=0A= >>> + /* Refill free descriptors even if no pkts recvd */=0A= >>> + if (nb_used =3D=3D 0 && virtqueue_full(rxvq))=0A= >>> return 0;=0A= >>> PMD_RX_LOG(DEBUG, "used:%d\n", nb_used);=0A= >=0A= >=0A= =0A=