[PATCH] virtio: Fix vring entry number issue

[PATCH] virtio: Fix vring entry number issue

[Ouyang Changchun]

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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Reviewed-by: Huawei Xie <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Tested-by: Jingguo Fu <jingguox.fu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 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;
-- 
1.8.4.2

Re: [PATCH] virtio: Fix vring entry number issue

[Olivier MATZ]

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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Huawei Xie <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Tested-by: Jingguo Fu <jingguox.fu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>   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

Re: [PATCH] virtio: Fix vring entry number issue

[Ouyang, Changchun]

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-pdR9zngts4EAvxtiuMwx3w@public.gmane.org]
> Sent: Wednesday, October 1, 2014 7:49 PM
> To: Ouyang, Changchun; dev-VfR2kkLFssw@public.gmane.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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Reviewed-by: Huawei Xie <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Tested-by: Jingguo Fu <jingguox.fu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > ---
> >   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