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

[Olivier MATZ <olivier.matz@6wind.com>]

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