Re: [PATCH v3] fib: network byte order IPv4 lookup

[David Marchand <david.marchand@redhat.com>]

On Thu, Oct 10, 2024 at 1:26 PM Vladimir Medvedkin
<vladimir.medvedkin@intel.com> wrote:
> diff --git a/lib/fib/meson.build b/lib/fib/meson.build
> index 6795f41a0a..8c03496cdc 100644
> --- a/lib/fib/meson.build
> +++ b/lib/fib/meson.build
> @@ -25,40 +25,28 @@ if dpdk_conf.has('RTE_ARCH_X86_64') and binutils_ok
>      # linked into main lib.
>
>      # check if all required flags already enabled (variant a).
> -    acl_avx512_flags = ['__AVX512F__','__AVX512DQ__']
> -    acl_avx512_on = true
> -    foreach f:acl_avx512_flags
> +    fib_avx512_flags = ['__AVX512F__','__AVX512DQ__', '__AVX512BW__']
> +    fib_avx512_on = true
> +    foreach f:fib_avx512_flags
>          if cc.get_define(f, args: machine_args) == ''
> -            acl_avx512_on = false
> +            fib_avx512_on = false
>          endif
>      endforeach

Repeating comment on v2 that was lost because of duplicate submission (?):

Please reuse the common checks recently merged, see for example:
https://git.dpdk.org/dpdk/diff/drivers/event/dlb2/meson.build?id=ef7a4025cd714189dc333bb19ea60c2abdeffb7d

>
> -    if acl_avx512_on == true
> -        cflags += ['-DCC_DIR24_8_AVX512_SUPPORT']
> -        sources += files('dir24_8_avx512.c')
> -        # TRIE AVX512 implementation uses avx512bw intrinsics along with
> -        # avx512f and avx512dq
> -        if cc.get_define('__AVX512BW__', args: machine_args) != ''
> -            cflags += ['-DCC_TRIE_AVX512_SUPPORT']
> -            sources += files('trie_avx512.c')
> -        endif
> -    elif cc.has_multi_arguments('-mavx512f', '-mavx512dq')
> +    if fib_avx512_on == true
> +        cflags += ['-DCC_DIR24_8_AVX512_SUPPORT', '-DCC_TRIE_AVX512_SUPPORT']

Nit: now that both dir24_8 and trie share the same requirement, can we
go with a simple CC_AVX512_SUPPORT?
This is really a nit, I am ok if you prefer to separate both.


> +        sources += files('dir24_8_avx512.c', 'trie_avx512.c')
> +    elif cc.has_multi_arguments('-mavx512f', '-mavx512dq', '-mavx512bw')
>          dir24_8_avx512_tmp = static_library('dir24_8_avx512_tmp',
>                  'dir24_8_avx512.c',
>                  dependencies: static_rte_eal,
> -                c_args: cflags + ['-mavx512f', '-mavx512dq'])
> +                c_args: cflags + ['-mavx512f', '-mavx512dq', '-mavx512bw'])
>          objs += dir24_8_avx512_tmp.extract_objects('dir24_8_avx512.c')
> -        cflags += ['-DCC_DIR24_8_AVX512_SUPPORT']
> -        # TRIE AVX512 implementation uses avx512bw intrinsics along with
> -        # avx512f and avx512dq
> -        if cc.has_argument('-mavx512bw')
> -            trie_avx512_tmp = static_library('trie_avx512_tmp',
> +        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
> +                c_args: cflags + ['-mavx512f', '-mavx512dq', '-mavx512bw'])
> +        objs += trie_avx512_tmp.extract_objects('trie_avx512.c')
> +        cflags += ['-DCC_DIR24_8_AVX512_SUPPORT', '-DCC_TRIE_AVX512_SUPPORT']
>      endif
>  endif
> diff --git a/lib/fib/rte_fib.c b/lib/fib/rte_fib.c
> index 4f9fba5a4f..991e48b5ea 100644
> --- a/lib/fib/rte_fib.c
> +++ b/lib/fib/rte_fib.c
> @@ -42,6 +42,7 @@ EAL_REGISTER_TAILQ(rte_fib_tailq)
>  struct rte_fib {
>         char                    name[RTE_FIB_NAMESIZE];
>         enum rte_fib_type       type;   /**< Type of FIB struct */
> +       int flags;                                      /**< Flags */
>         struct rte_rib          *rib;   /**< RIB helper datastructure */
>         void                    *dp;    /**< pointer to the dataplane struct*/
>         rte_fib_lookup_fn_t     lookup; /**< FIB lookup function */
> @@ -110,7 +111,7 @@ init_dataplane(struct rte_fib *fib, __rte_unused int socket_id,
>                 if (fib->dp == NULL)
>                         return -rte_errno;
>                 fib->lookup = dir24_8_get_lookup_fn(fib->dp,
> -                       RTE_FIB_LOOKUP_DEFAULT);
> +                       RTE_FIB_LOOKUP_DEFAULT, !!(fib->flags & RTE_FIB_FLAG_LOOKUP_BE));
>                 fib->modify = dir24_8_modify;
>                 return 0;
>         default:
> @@ -214,6 +215,7 @@ rte_fib_create(const char *name, int socket_id, struct rte_fib_conf *conf)
>         rte_strlcpy(fib->name, name, sizeof(fib->name));
>         fib->rib = rib;
>         fib->type = conf->type;
> +       fib->flags = conf->flags;

In addition to Robin comments, I also have a concern on the
extensibility aspect.

conf->flags must be validated against known flags.
Otherwise existing applications may pass wrong stuff and "work fine",
until the day we had one more flag.


>         fib->def_nh = conf->default_nh;
>         ret = init_dataplane(fib, socket_id, conf);
>         if (ret < 0) {


-- 
David Marchand