Re: [dpdk-dev] [PATCH 1/2] net: add arm64 neon version of CRC compute APIs

[Jan Viktorin <viktorin@rehivetech.com>]

Hello Ashwin Sekhar,

some comments below...

On Thu, 27 Apr 2017 07:10:20 -0700
Ashwin Sekhar T K <ashwin.sekhar@caviumnetworks.com> 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 <ashwin.sekhar@caviumnetworks.com>
> ---
>  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 <stdint.h>
> +#include <assert.h>
> +
>  #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))