[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.medvedkin at 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.ananyev at intel.com>
> 

-- 
Regards,
Vladimir