Re: [dpdk-dev] [PATCH] virtio: Fix vring entry number issue

["Ouyang, Changchun" <changchun.ouyang@intel.com>]

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