Re: [dpdk-dev] [PATCH v2] virtio: enable indirect descriptors feature

[Maxime Coquelin <maxime.coquelin@redhat.com>]

Thanks Pierre for sending the fix.

Minor comments below:

On 09/05/2016 08:52 AM, Pierre Pfister (ppfister) wrote:
> Indirect descriptors support was disabled by commit 4a92b67151be11,
> presumably by accident as it was correctly supported.
>
> This patch simply adds VIRTIO_RING_F_INDIRECT_DESC back to
> the supported features bit mask, hence enabling the use of
> indirect descriptors when the feature is negociated with the
> device.
>

You should add the below line:
Fixes: 4a92b671 ("virtio: clarify feature bit handling")

Also, maybe we should consider add stable@dpdk.org in cc:,
because the regression was introduced before v16.07 final tag.
But the problem is that all the final validation has been done
without this feature enabled, and it impact quite a few lines of
code in Virtio PMD.

Other than that, you can add:
Reviewed-by: Maxime Coquelin <maxime.coquelin@¶edhat.com>

> Signed-off-by: Pierre Pfister <ppfister@cisco.com>
> ---
>  drivers/net/virtio/virtio_ethdev.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
> index 2ecec6e..31c91a5 100644
> --- a/drivers/net/virtio/virtio_ethdev.h
> +++ b/drivers/net/virtio/virtio_ethdev.h
> @@ -63,6 +63,7 @@
>          1u << VIRTIO_NET_F_CTRL_RX       |     \
>          1u << VIRTIO_NET_F_CTRL_VLAN     |     \
>          1u << VIRTIO_NET_F_MRG_RXBUF     |     \
> +        1u << VIRTIO_RING_F_INDIRECT_DESC |    \
>          1ULL << VIRTIO_F_VERSION_1)
>
>  /*
> --
> 2.7.4 (Apple Git-66)
>


One off-topic question, do you measure some improvement in perfs using
the feature? If yes, could you describe the use-case, and the figures?
I ask because I have implemented TX indirect descriptor in vhost lib
(see http://dpdk.org/dev/patchwork/patch/14797/), and failed to see
some use-case benefiting of it.

Regards,
Maxime