From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from proxy.6wind.com (host.76.145.23.62.rev.coltfrance.com [62.23.145.76]) by dpdk.org (Postfix) with ESMTP id 3D2C81D7; Tue, 19 Dec 2017 11:50:43 +0100 (CET) Received: from core.dev.6wind.com (unknown [10.0.0.1]) by proxy.6wind.com (Postfix) with ESMTPS id 112EA11699B; Tue, 19 Dec 2017 11:42:04 +0100 (CET) Received: from [10.16.0.195] (helo=6wind.com) by core.dev.6wind.com with smtp (Exim 4.84_2) (envelope-from ) id 1eRFTj-0004jO-Gn; Tue, 19 Dec 2017 11:50:32 +0100 Received: by 6wind.com (sSMTP sendmail emulation); Tue, 19 Dec 2017 11:50:31 +0100 Date: Tue, 19 Dec 2017 11:50:31 +0100 From: Olivier MATZ To: Tiwei Bie Cc: dev@dpdk.org, Yuanhan Liu , Maxime Coquelin , Samuel Gauthier , stable@dpdk.org Message-ID: <20171219105030.kazbbek5anz2js25@glumotte.dev.6wind.com> References: <20171214143213.28577-1-olivier.matz@6wind.com> <20171218025047.a6ofepaey2ztozd3@debian-xvivbkq> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171218025047.a6ofepaey2ztozd3@debian-xvivbkq> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH] net/virtio: fix Rx and Tx handler selection for arm32 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 19 Dec 2017 10:50:43 -0000 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 > > > > 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 = ; hw->use_simple_tx = ; Olivier