Re: [dpdk-stable] [dpdk-dev] [PATCH] net/virtio: fix Rx and Tx handler selection for arm32

[Tiwei Bie <tiwei.bie@intel.com>]

On Thu, Dec 14, 2017 at 03:32:13PM +0100, Olivier Matz wrote:
> From: Samuel Gauthier <samuel.gauthier@6wind.com>
> 
> On arm32, we were always selecting the simple handler, but it is only
> available if neon is present.
> 
> This is due to a typo in the name of the config option.
> CONFIG_RTE_ARCH_ARM is for Makefiles. One should use RTE_ARCH_ARM.
> 
> Fixes: 2d7c37194ee4 ("net/virtio: add NEON based Rx handler")
> Cc: stable@dpdk.org

Hi Olivier,

My comment isn't really related to this patch, but related
to the commit it fixes and some related commits from you.

The commit (2d7c37194ee4) specified by the fixline doesn't
really cause the problem described in the commit log:

"On arm32, we were always selecting the simple handler, ..."

Actually, it will cause the simple handler won't be chosen
on arm32. Below is the relevant part (use_simple_rxtx won't
get a chance to be updated to 1 on arm32):

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 0e369bd12..9ab441bc3 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -488,6 +488,9 @@ virtio_update_rxtx_handler(struct rte_eth_dev *dev,
 #if defined RTE_ARCH_X86
 	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
 		use_simple_rxtx = 1;
+#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
+	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
+		use_simple_rxtx = 1;
 #endif
 	/* Use simple rx/tx func if single segment and no offloads */
 	if (use_simple_rxtx &&

It's the below commits (together with the above commit)
caused the simple handler will always be chosen on arm32:

4819eae8d94b ("net/virtio: rationalize setting of Rx/Tx handlers")
0964936308cd ("net/virtio: keep Rx handler whatever the Tx queue config")

For the above two commits, I think they have some other
problems. From my understanding, vector Rx function of
virtio PMD doesn't really follow the virtio spec. It
assumes the desc idx in the used ring will be written by
the backend in an expected order (i.e. the same order in
avail ring). So it even doesn't read the id field from
the used_elem to get the desc idx (but actually it should,
unless we change the virtio spec). It seems that simple
Tx function also has similar problem.

So IMO, we shouldn't choose the simple functions on all
platforms unless users enable them explicitly as we can't
guarantee they will work with all backends. So maybe the
problems can be fixed further? Any thoughts?

Best regards,
Tiwei Bie

> 
> Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index e0328f61d..c0ba83b06 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1784,7 +1784,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>  	hw->use_simple_rx = 1;
>  	hw->use_simple_tx = 1;
>  
> -#if defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
> +#if defined RTE_ARCH_ARM64 || defined RTE_ARCH_ARM
>  	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON)) {
>  		hw->use_simple_rx = 0;
>  		hw->use_simple_tx = 0;
> -- 
> 2.11.0
>