From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from wes1-so1.wedos.net (wes1-so1-c.wedos.net [46.28.106.44]) by dpdk.org (Postfix) with ESMTP id 8CB531E34 for ; Fri, 28 Apr 2017 11:55:54 +0200 (CEST) Received: from jvn (188.215.broadband18.iol.cz [109.81.215.188]) by wes1-so1.wedos.net (Postfix) with ESMTPSA id 3wDq2K5VTdz8qL; Fri, 28 Apr 2017 11:55:53 +0200 (CEST) Date: Fri, 28 Apr 2017 11:55:44 +0200 From: Jan Viktorin To: Ashwin Sekhar T K Cc: thomas@monjalon.net, jasvinder.singh@intel.com, jerin.jacob@caviumnetworks.com, jianbo.liu@linaro.org, dev@dpdk.org Message-ID: <20170428115544.52d3388d@jvn> In-Reply-To: <20170427141021.18767-1-ashwin.sekhar@caviumnetworks.com> References: <20170427141021.18767-1-ashwin.sekhar@caviumnetworks.com> Organization: RehiveTech X-Mailer: Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-unknown-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 1/2] net: add arm64 neon version of CRC compute APIs 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: Fri, 28 Apr 2017 09:55:54 -0000 Hello Ashwin Sekhar, some comments below... On Thu, 27 Apr 2017 07:10:20 -0700 Ashwin Sekhar T K wrote: > * Added CRC compute APIs for arm64 utilizing the pmull capability > * Added new file net_crc_neon.h to hold the arm64 pmull CRC > implementation > * Added crypto capability in compilation of generic armv8 and > thunderx targets > * pmull CRC version is used only after checking the pmull capability > at runtime > * Verified the changes with crc_autotest unit test case > > Signed-off-by: Ashwin Sekhar T K > --- > MAINTAINERS | 1 + > lib/librte_eal/common/include/arch/arm/rte_vect.h | 45 +++ > lib/librte_net/net_crc_neon.h | 357 ++++++++++++++++++++++ > lib/librte_net/rte_net_crc.c | 32 +- > lib/librte_net/rte_net_crc.h | 2 + > mk/machine/armv8a/rte.vars.mk | 2 +- > mk/machine/thunderx/rte.vars.mk | 2 +- > mk/rte.cpuflags.mk | 3 + > mk/toolchain/gcc/rte.toolchain-compat.mk | 1 + > 9 files changed, 438 insertions(+), 7 deletions(-) > create mode 100644 lib/librte_net/net_crc_neon.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 576d60a..283743e 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -149,6 +149,7 @@ F: lib/librte_lpm/rte_lpm_neon.h > F: lib/librte_hash/rte*_arm64.h > F: lib/librte_efd/rte*_arm64.h > F: lib/librte_table/rte*_arm64.h > +F: lib/librte_net/net_crc_neon.h > F: drivers/net/ixgbe/ixgbe_rxtx_vec_neon.c > F: drivers/net/i40e/i40e_rxtx_vec_neon.c > F: drivers/net/virtio/virtio_rxtx_simple_neon.c > diff --git a/lib/librte_eal/common/include/arch/arm/rte_vect.h b/lib/librte_eal/common/include/arch/arm/rte_vect.h > index 4107c99..9a3dfdf 100644 > --- a/lib/librte_eal/common/include/arch/arm/rte_vect.h > +++ b/lib/librte_eal/common/include/arch/arm/rte_vect.h > @@ -34,9 +34,18 @@ > #define _RTE_VECT_ARM_H_ > > #include > +#include > + > #include "generic/rte_vect.h" > #include "arm_neon.h" > > +#ifdef GCC_VERSION > +#undef GCC_VERSION > +#endif Why are you doing this? What is wrong with GCC_VERSION? > + > +#define GCC_VERSION (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 \ > + + __GNUC_PATCHLEVEL__) > + If you have any specific requirements for testing GCC version then it should be done in a more elegant way. However, I do not understand your intention. > #ifdef __cplusplus > extern "C" { > #endif > @@ -78,6 +87,42 @@ vqtbl1q_u8(uint8x16_t a, uint8x16_t b) > } > #endif > > +#if (GCC_VERSION < 70000) Is this code is gcc-specific? In such case there should be check for GCC compiler. We can also build e.g. by clang. > +/* > + * NEON intrinsic vreinterpretq_u64_p128() is not supported > + * in GCC versions < 7 > + */ I'd be positive about those comments, like: NEON intrinsic vreinterpretq_u64_p128() is supported since GCC 7. > +static inline uint64x2_t > +vreinterpretq_u64_p128(poly128_t x) > +{ > + return (uint64x2_t)x; > +} > + > +/* > + * NEON intrinsic vreinterpretq_p64_u64() is not supported > + * in GCC versions < 7 > + */ > +static inline poly64x2_t > +vreinterpretq_p64_u64(uint64x2_t x) > +{ > + return (poly64x2_t)x; > +} > + > +/* > + * NEON intrinsic vgetq_lane_p64() is not supported > + * in GCC versions < 7 > + */ > +static inline poly64_t > +vgetq_lane_p64(poly64x2_t x, const int lane) > +{ > + assert(lane >= 0 && lane <= 1); > + > + poly64_t *p = (poly64_t *)&x; > + > + return p[lane]; > +} > +#endif > + > #ifdef __cplusplus > } > #endif > diff --git a/lib/librte_net/net_crc_neon.h b/lib/librte_net/net_crc_neon.h [...] > # CPU_LDFLAGS = > # CPU_ASFLAGS = > > -MACHINE_CFLAGS += -march=armv8-a+crc > +MACHINE_CFLAGS += -march=armv8-a+crc+crypto > diff --git a/mk/machine/thunderx/rte.vars.mk b/mk/machine/thunderx/rte.vars.mk > index ad5a379..6784105 100644 > --- a/mk/machine/thunderx/rte.vars.mk > +++ b/mk/machine/thunderx/rte.vars.mk > @@ -55,4 +55,4 @@ > # CPU_LDFLAGS = > # CPU_ASFLAGS = > > -MACHINE_CFLAGS += -march=armv8-a+crc -mcpu=thunderx > +MACHINE_CFLAGS += -march=armv8-a+crc+crypto -mcpu=thunderx > diff --git a/mk/rte.cpuflags.mk b/mk/rte.cpuflags.mk > index e634abc..6bbd742 100644 > --- a/mk/rte.cpuflags.mk > +++ b/mk/rte.cpuflags.mk > @@ -119,6 +119,9 @@ ifneq ($(filter $(AUTO_CPUFLAGS),__ARM_FEATURE_CRC32),) > CPUFLAGS += CRC32 > endif > > +ifneq ($(filter $(AUTO_CPUFLAGS),__ARM_FEATURE_CRYPTO),) > +CPUFLAGS += PMULL > +endif > > MACHINE_CFLAGS += $(addprefix -DRTE_MACHINE_CPUFLAG_,$(CPUFLAGS)) > > diff --git a/mk/toolchain/gcc/rte.toolchain-compat.mk b/mk/toolchain/gcc/rte.toolchain-compat.mk > index 280dde2..01ac7e2 100644 > --- a/mk/toolchain/gcc/rte.toolchain-compat.mk > +++ b/mk/toolchain/gcc/rte.toolchain-compat.mk > @@ -60,6 +60,7 @@ else > # > ifeq ($(shell test $(GCC_VERSION) -le 49 && echo 1), 1) > MACHINE_CFLAGS := $(patsubst -march=armv8-a+crc,-march=armv8-a+crc -D__ARM_FEATURE_CRC32=1,$(MACHINE_CFLAGS)) The line above is to be dropped, isn't it? > + MACHINE_CFLAGS := $(patsubst -march=armv8-a+crc+crypto,-march=armv8-a+crc+crypto -D__ARM_FEATURE_CRC32=1,$(MACHINE_CFLAGS)) Please, split the "feature-detection" changes into a separate commit and explain it. In the code, you test for GCC 7. Here you are ok with GCC 4.9. It's likely to be correct but it is not clear. Also, please explain why is the "crypto" feature required. Regards Jan > endif > ifeq ($(shell test $(GCC_VERSION) -le 47 && echo 1), 1) > MACHINE_CFLAGS := $(patsubst -march=core-avx-i,-march=corei7-avx,$(MACHINE_CFLAGS))