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

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

Hi Tiwei,

On Mon, Dec 18, 2017 at 10:50:47AM +0800, Tiwei Bie wrote:
> 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 &&

You are right. The commit 2d7c37194ee4 introduces the typo in the config
name, but does not trigger the same issue since the handler selection
logic was reversed later.

I think we can keep this "Fixes: 2d7c37194ee4" but also add the other
that you quote below.

> 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.

Ok, I was not aware of this.

> 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?

Yes, if the vector implementation does not follow the specifications, I
think it should be fixed (prefered solution) or disabled by default. I'm
wondering what would be the proper way to enable it, maybe it could be a
device argument (rte_devargs)?

In that case, we could replace:

   hw->use_simple_rx = 1;
   hw->use_simple_tx = 1;

by:

   hw->use_simple_rx = <value from devargs>;
   hw->use_simple_tx = <value from devargs>;


Olivier