Re: [dpdk-dev] [PATCH v3 7/8] fib6: introduce AVX512 lookup

[Medvedkin, Vladimir]

Hi Konstantin,

Thanks for review,

On 08/07/2020 13:23, Ananyev, Konstantin wrote:

Add new lookup implementation for FIB6 trie algorithm using
AVX512 instruction set

Signed-off-by: Vladimir Medvedkin <vladimir.medved...@intel.com>
---
  lib/librte_fib/Makefile      |  10 ++
  lib/librte_fib/meson.build   |   9 ++
  lib/librte_fib/rte_fib6.h    |   3 +-
  lib/librte_fib/trie.c        |  21 ++++
  lib/librte_fib/trie_avx512.c | 269 +++++++++++++++++++++++++++++++++++++++++++
  lib/librte_fib/trie_avx512.h |  20 ++++
  6 files changed, 331 insertions(+), 1 deletion(-)
  create mode 100644 lib/librte_fib/trie_avx512.c
  create mode 100644 lib/librte_fib/trie_avx512.h

diff --git a/lib/librte_fib/Makefile b/lib/librte_fib/Makefile
index 3958da1..761c7c8 100644
--- a/lib/librte_fib/Makefile
+++ b/lib/librte_fib/Makefile
@@ -25,12 +25,22 @@ grep -q __AVX512F__ && echo 1)
  CC_AVX512DQ_SUPPORT=$(shell $(CC) -mavx512dq -dM -E - </dev/null 2>&1 | \
  grep -q __AVX512DQ__ && echo 1)

+CC_AVX512BW_SUPPORT=$(shell $(CC) -mavx512bw -dM -E - </dev/null 2>&1 | \
+grep -q __AVX512BW__ && echo 1)
+
  ifeq ($(CC_AVX512F_SUPPORT), 1)
  ifeq ($(CC_AVX512DQ_SUPPORT), 1)
  SRCS-$(CONFIG_RTE_LIBRTE_FIB) += dir24_8_avx512.c
  CFLAGS_dir24_8_avx512.o += -mavx512f
  CFLAGS_dir24_8_avx512.o += -mavx512dq
  CFLAGS_dir24_8.o += -DCC_DIR24_8_AVX512_SUPPORT
+ifeq ($(CC_AVX512BW_SUPPORT), 1)
+SRCS-$(CONFIG_RTE_LIBRTE_FIB) += trie_avx512.c
+CFLAGS_trie_avx512.o += -mavx512f
+CFLAGS_trie_avx512.o += -mavx512dq
+CFLAGS_trie_avx512.o += -mavx512bw
+CFLAGS_trie.o += -DCC_TRIE_AVX512_SUPPORT
+endif
  endif
  endif
  include $(RTE_SDK)/mk/rte.lib.mk
diff --git a/lib/librte_fib/meson.build b/lib/librte_fib/meson.build
index 0963f3c..98adf11 100644
--- a/lib/librte_fib/meson.build
+++ b/lib/librte_fib/meson.build
@@ -14,5 +14,14 @@ if dpdk_conf.has('RTE_ARCH_X86') and 
cc.has_argument('-mavx512f')
  c_args: cflags + ['-mavx512f'] + ['-mavx512dq'])
  objs += dir24_8_avx512_tmp.extract_objects('dir24_8_avx512.c')
  cflags += '-DCC_DIR24_8_AVX512_SUPPORT'
+if cc.has_argument('-mavx512bw')
+trie_avx512_tmp = static_library('trie_avx512_tmp',
+'trie_avx512.c',
+dependencies: static_rte_eal,
+c_args: cflags + ['-mavx512f'] + \
+['-mavx512dq'] + ['-mavx512bw'])
+objs += trie_avx512_tmp.extract_objects('trie_avx512.c')
+cflags += '-DCC_TRIE_AVX512_SUPPORT'
+endif
  endif
  endif
diff --git a/lib/librte_fib/rte_fib6.h b/lib/librte_fib/rte_fib6.h
index b70369a..c55efdf 100644
--- a/lib/librte_fib/rte_fib6.h
+++ b/lib/librte_fib/rte_fib6.h
@@ -53,7 +53,8 @@ enum rte_fib_trie_nh_sz {
  };

  enum rte_fib_trie_lookup_type {
-RTE_FIB6_TRIE_SCALAR
+RTE_FIB6_TRIE_SCALAR,
+RTE_FIB6_TRIE_VECTOR

As a nit - does this enum needs to be public?
If it does, then probably worth to name it VECTOR_AVX512,
in case someone in future will want to add another vector implementation.
Probably same thought for v4.

This enum is used with rte_fib_set_lookup_fn() so it needs to be public.
I'll change name to be with _AVX512 suffix. The same for v4.

Apart from that - LGTM.
Acked-by: Konstantin Ananyev <konstantin.anan...@intel.com>

--
Regards,
Vladimir