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.gauth...@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: sta...@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