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

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

Hi Olivier,

On Tue, Dec 19, 2017 at 11:50:31AM +0100, Olivier MATZ wrote:
> 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.

Looks good to me. Or I think it's also OK to mainly talk
about the typo in the commit log without talking about the
exact effect to the Tx/Rx handler selection.

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

I'm not sure how to fix it because we don't want to see big
performance drop after the fix. If it can be fixed properly
in this release, it would be great. Otherwise, we may need
to choose the 2nd solution (i.e. disabled by default) first.
Adding devargs looks good to me.

Best regards,
Tiwei Bie

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