From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 4A9EC5AB0 for ; Tue, 23 Feb 2016 09:26:33 +0100 (CET) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga101.fm.intel.com with ESMTP; 23 Feb 2016 00:26:32 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,488,1449561600"; d="scan'208";a="909219958" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga001.fm.intel.com with ESMTP; 23 Feb 2016 00:26:32 -0800 Received: from fmsmsx158.amr.corp.intel.com (10.18.116.75) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 23 Feb 2016 00:26:31 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx158.amr.corp.intel.com (10.18.116.75) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 23 Feb 2016 00:26:31 -0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.249]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.84]) with mapi id 14.03.0248.002; Tue, 23 Feb 2016 16:26:29 +0800 From: "Xie, Huawei" To: Tom Kiely , Kyle Larose Thread-Topic: [dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation Thread-Index: AdEnp1J61clOw+YwQ8y+r2GXRAF+4Q== Date: Tue, 23 Feb 2016 08:26:28 +0000 Message-ID: References: <1447407013-6986-1-git-send-email-tkiely@brocade.com> <567299E9.60000@brocade.com> <56CB35E9.50309@brocade.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.4.80] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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: Tue, 23 Feb 2016 08:26:33 -0000 On 2/23/2016 12:23 AM, Tom Kiely wrote:=0A= > Hi,=0A= > Sorry I missed the last few messages until now. I'm happy with=0A= > just removing the "if". Kyle, when you say you fixed it, do you mean=0A= > that you will push the patch or have already done so ?=0A= > Thanks,=0A= > Tom=0A= >=0A= > On 02/18/2016 02:03 PM, Kyle Larose wrote:=0A= >> On Tue, Jan 5, 2016 at 2:13 AM, Xie, Huawei =0A= >> wrote:=0A= >>> On 12/17/2015 7:18 PM, Tom Kiely wrote:=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=0A= >>> following=0A= >>> receiving code already takes care of the "num =3D=3D 0" condition.=0A= >>>=0A= >> FWIW, I fixed this issue myself by just removing the if(num =3D=3D 0)=0A= >> checks entirely. I didn't see any benefit in short-circuiting a loop=0A= >> which pretty much does nothing anyway when num =3D=3D 0. Further, we onl= y=0A= >> hit this case when there's no packets to receive, which means there's=0A= >> probably a few cycles to spare. This is even simpler.=0A= =0A= Yes, as i said, that is the simplest fix.=0A= =0A= >>=0A= >>> I find virtqueue_full is confusing, maybe we could change it to some=0A= >>> other meaningful name.=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= =0A=