Message ID | 1409812499-15656-1-git-send-email-changchun.ouyang@intel.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers |
Return-Path: <couyang@shecgisg004.sh.intel.com> Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id C3E15685B for <dev@dpdk.org>; Thu, 4 Sep 2014 08:30:34 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP; 03 Sep 2014 23:35:13 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.04,464,1406617200"; d="scan'208";a="594370834" Received: from shvmail01.sh.intel.com ([10.239.29.42]) by fmsmga002.fm.intel.com with ESMTP; 03 Sep 2014 23:35:10 -0700 Received: from shecgisg004.sh.intel.com (shecgisg004.sh.intel.com [10.239.29.89]) by shvmail01.sh.intel.com with ESMTP id s846Z8TB013752; Thu, 4 Sep 2014 14:35:08 +0800 Received: from shecgisg004.sh.intel.com (localhost [127.0.0.1]) by shecgisg004.sh.intel.com (8.13.6/8.13.6/SuSE Linux 0.8) with ESMTP id s846Z6Fw015759; Thu, 4 Sep 2014 14:35:08 +0800 Received: (from couyang@localhost) by shecgisg004.sh.intel.com (8.13.6/8.13.6/Submit) id s846Z1vK015755; Thu, 4 Sep 2014 14:35:01 +0800 From: Ouyang Changchun <changchun.ouyang@intel.com> To: dev@dpdk.org Date: Thu, 4 Sep 2014 14:34:59 +0800 Message-Id: <1409812499-15656-1-git-send-email-changchun.ouyang@intel.com> X-Mailer: git-send-email 1.7.0.7 Subject: [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 <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> X-List-Received-Date: Thu, 04 Sep 2014 06:30:35 -0000 |
Commit Message
Ouyang Changchun
Sept. 4, 2014, 6:34 a.m. UTC
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 <changchun.ouyang@intel.com> Reviewed-by: Huawei Xie <huawei.xie@intel.com> Tested-by: Jingguo Fu <jingguox.fu@intel.com> --- lib/librte_pmd_virtio/virtio_rxtx.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Comments
Hello, 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 <changchun.ouyang@intel.com> > Reviewed-by: Huawei Xie <huawei.xie@intel.com> > Tested-by: Jingguo Fu <jingguox.fu@intel.com> > --- > 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 = (uint16_t)(likely(nb_used < VIRTIO_MBUF_BURST_SZ) ? nb_used : VIRTIO_MBUF_BURST_SZ); > > while (nb_tx < nb_pkts) { > - int need = tx_pkts[nb_tx]->pkt.nb_segs - txvq->vq_free_cnt; > + /* Need one more entry for virtio header. */ > + int need = tx_pkts[nb_tx]->pkt.nb_segs - txvq->vq_free_cnt + 1; > int deq_cnt = RTE_MIN(need, (int)num); > > num -= (deq_cnt > 0) ? deq_cnt : 0; > In the commit log, you talk about vring entries, but to me it seems that it is about virtio descriptors. I think it could be useful to explain what was the consequence of this issue. Is it a performance issue? In my understanding, the problem is that we won't dequeue mbufs from the "used" vring, resulting in a possible "blocking" of the queue. Is it correct? Also, I think it would help the review to explain in which conditions the problem is triggered and how the fix was tested. Last comment, I'm wondering if the next test should also be modified: if (tx_pkts[nb_tx]->nb_segs <= txvq->vq_free_cnt) { Into: if (tx_pkts[nb_tx]->nb_segs <= txvq->vq_free_cnt + 1) { (or maybe using the "need" variable) Do you agree? Regards, Olivier
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 > > Hello, > > 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 <changchun.ouyang@intel.com> > > Reviewed-by: Huawei Xie <huawei.xie@intel.com> > > Tested-by: Jingguo Fu <jingguox.fu@intel.com> > > --- > > 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 = (uint16_t)(likely(nb_used < VIRTIO_MBUF_BURST_SZ) ? > nb_used : > > VIRTIO_MBUF_BURST_SZ); > > > > while (nb_tx < nb_pkts) { > > - int need = tx_pkts[nb_tx]->pkt.nb_segs - txvq- > >vq_free_cnt; > > + /* Need one more entry for virtio header. */ > > + int need = tx_pkts[nb_tx]->pkt.nb_segs - txvq->vq_free_cnt > + 1; > > int deq_cnt = RTE_MIN(need, (int)num); > > > > num -= (deq_cnt > 0) ? deq_cnt : 0; > > > > > In the commit log, you talk about vring entries, but to me it seems that it is > about virtio descriptors. > Agree, but in current virtio pmd, one entry has only one descriptor, so both 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 issue. 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, In circumstance of only 1 descriptor in free list, the packet with only 1 segment can't be transmitted Because the transmitting need 2 descriptors(one for packet itself, the other 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" of > the queue. Is it correct? Also, I think it would help the review to explain in > which conditions the problem is triggered and how the fix was tested. > > Last comment, I'm wondering if the next test should also be modified: > > if (tx_pkts[nb_tx]->nb_segs <= txvq->vq_free_cnt) { > > Into: > > if (tx_pkts[nb_tx]->nb_segs <= txvq->vq_free_cnt + 1) { > > (or maybe using the "need" variable) > > Do you agree? Agree, change is made in v2 patch. > > Regards, > Olivier
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 = (uint16_t)(likely(nb_used < VIRTIO_MBUF_BURST_SZ) ? nb_used : VIRTIO_MBUF_BURST_SZ); while (nb_tx < nb_pkts) { - int need = tx_pkts[nb_tx]->pkt.nb_segs - txvq->vq_free_cnt; + /* Need one more entry for virtio header. */ + int need = tx_pkts[nb_tx]->pkt.nb_segs - txvq->vq_free_cnt + 1; int deq_cnt = RTE_MIN(need, (int)num); num -= (deq_cnt > 0) ? deq_cnt : 0;