From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 39B232BF7; Mon, 18 Dec 2017 03:51:18 +0100 (CET) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Dec 2017 18:51:17 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,419,1508828400"; d="scan'208";a="13453957" Received: from deepin-15.sh.intel.com (HELO debian-xvivbkq) ([10.67.104.165]) by fmsmga004.fm.intel.com with ESMTP; 17 Dec 2017 18:51:16 -0800 Date: Mon, 18 Dec 2017 10:50:47 +0800 From: Tiwei Bie To: Olivier Matz Cc: dev@dpdk.org, Yuanhan Liu , Maxime Coquelin , Samuel Gauthier , stable@dpdk.org Message-ID: <20171218025047.a6ofepaey2ztozd3@debian-xvivbkq> References: <20171214143213.28577-1-olivier.matz@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20171214143213.28577-1-olivier.matz@6wind.com> User-Agent: NeoMutt/20170609 (1.8.3) 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: Mon, 18 Dec 2017 02:51:19 -0000 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 && 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 > --- > 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 >