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 D1B3081B3 for ; Sat, 11 Oct 2014 10:14:40 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga101.fm.intel.com with ESMTP; 11 Oct 2014 01:22:09 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.97,862,1389772800"; d="scan'208";a="398642468" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by FMSMGA003.fm.intel.com with ESMTP; 11 Oct 2014 01:15:11 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.195.1; Sat, 11 Oct 2014 01:22:09 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.192]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.203]) with mapi id 14.03.0195.001; Sat, 11 Oct 2014 16:22:07 +0800 From: "Ouyang, Changchun" To: Olivier MATZ , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH] virtio: Fix vring entry number issue Thread-Index: AQHPyApm8Q/qzFS1ukufcJCf6OrUnpwaxSsAgBABGMA= Date: Sat, 11 Oct 2014 08:22:07 +0000 Message-ID: References: <1409812499-15656-1-git-send-email-changchun.ouyang@intel.com> <542BEA28.3030300@6wind.com> In-Reply-To: <542BEA28.3030300@6wind.com> Accept-Language: zh-CN, 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 vring entry number issue 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: Sat, 11 Oct 2014 08:14:41 -0000 Hi Olivier, I have v2 patch according to your comments. Please Ack it if you can. Thanks, Changchun > -----Original Message----- > From: Olivier MATZ [mailto:olivier.matz@6wind.com] > Sent: Wednesday, October 1, 2014 7:49 PM > To: Ouyang, Changchun; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] virtio: Fix vring entry number issue >=20 > Hello, >=20 > On 09/04/2014 08:34 AM, Ouyang Changchun wrote: > > Fix one issue in virtio TX: it needs one more vring entry to hold the > > virtio header when transmitting packets, it is used later to determine > > whether to free more entries from used vring. > > > > Signed-off-by: Changchun Ouyang > > Reviewed-by: Huawei Xie > > Tested-by: Jingguo Fu > > --- > > lib/librte_pmd_virtio/virtio_rxtx.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/lib/librte_pmd_virtio/virtio_rxtx.c > > b/lib/librte_pmd_virtio/virtio_rxtx.c > > index 0b10108..b1c189c 100644 > > --- a/lib/librte_pmd_virtio/virtio_rxtx.c > > +++ b/lib/librte_pmd_virtio/virtio_rxtx.c > > @@ -699,7 +699,8 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf > **tx_pkts, uint16_t nb_pkts) > > num =3D (uint16_t)(likely(nb_used < VIRTIO_MBUF_BURST_SZ) ? > nb_used : > > VIRTIO_MBUF_BURST_SZ); > > > > while (nb_tx < nb_pkts) { > > - int need =3D tx_pkts[nb_tx]->pkt.nb_segs - txvq- > >vq_free_cnt; > > + /* Need one more entry for virtio header. */ > > + int need =3D tx_pkts[nb_tx]->pkt.nb_segs - txvq->vq_free_cnt > + 1; > > int deq_cnt =3D RTE_MIN(need, (int)num); > > > > num -=3D (deq_cnt > 0) ? deq_cnt : 0; > > >=20 >=20 > In the commit log, you talk about vring entries, but to me it seems that = it is > about virtio descriptors. >=20 Agree, but in current virtio pmd, one entry has only one descriptor, so bot= h are ok. To be more accurate, in v2 patch I adopt your suggestion. :-) > I think it could be useful to explain what was the consequence of this is= sue. Is > it a performance issue? In my understanding, the problem is that we won't Not only performance issue, as I state in the v2 patch description,=20 In circumstance of only 1 descriptor in free list, the packet with only 1 s= egment can't be transmitted Because the transmitting need 2 descriptors(one for packet itself, the othe= r for virtio header), but only 1 freed descriptor available. And due to the false test condition, no more descriptors will be freed into= free list. > dequeue mbufs from the "used" vring, resulting in a possible "blocking" o= f > the queue. Is it correct? Also, I think it would help the review to expla= in in > which conditions the problem is triggered and how the fix was tested. >=20 > Last comment, I'm wondering if the next test should also be modified: >=20 > if (tx_pkts[nb_tx]->nb_segs <=3D txvq->vq_free_cnt) { >=20 > Into: >=20 > if (tx_pkts[nb_tx]->nb_segs <=3D txvq->vq_free_cnt + 1) { >=20 > (or maybe using the "need" variable) >=20 > Do you agree? Agree, change is made in v2 patch. >=20 > Regards, > Olivier