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 at 6wind.com] > Sent: Wednesday, October 1, 2014 7:49 PM > To: Ouyang, Changchun; dev at 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 at intel.com> > > Reviewed-by: Huawei Xie <huawei.xie at intel.com> > > Tested-by: Jingguo Fu <jingguox.fu at 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
