Re: [dpdk-dev] [PATCH v14 1/8] fib: make lookup function type configurable

["Medvedkin, Vladimir" <vladimir.medvedkin@intel.com>]

Hello David,

On 26/10/2020 13:58, David Marchand wrote:
> Hello Vladimir,
> 
> On Sun, Oct 25, 2020 at 7:08 PM Vladimir Medvedkin
> <vladimir.medvedkin@intel.com> wrote:
>> diff --git a/lib/librte_fib/rte_fib.h b/lib/librte_fib/rte_fib.h
>> index 84ee774..2097ee5 100644
>> --- a/lib/librte_fib/rte_fib.h
>> +++ b/lib/librte_fib/rte_fib.h
>> @@ -58,6 +58,21 @@ enum rte_fib_dir24_8_nh_sz {
>>          RTE_FIB_DIR24_8_8B
>>   };
>>
>> +/** Type of lookup function implementation */
>> +enum rte_fib_dir24_8_lookup_type {
>> +       RTE_FIB_DIR24_8_SCALAR_MACRO,
>> +       /**< Macro based lookup function */
>> +       RTE_FIB_DIR24_8_SCALAR_INLINE,
>> +       /**<
>> +        * Lookup implementation using inlined functions
>> +        * for different next hop sizes
>> +        */
>> +       RTE_FIB_DIR24_8_SCALAR_UNI
>> +       /**<
>> +        * Unified lookup function for all next hop sizes
>> +        */
>> +};
>> +
> 
> We can't have a generic function, with a specific type/
> Let's have a generic name, in hope it will be extended later for other
> fib implementations.
> For the default behavior and selecting the "best" possible
> implementation, we can introduce a RTE_FIB_LOOKUP_DEFAULT magic value
> that would work with any fib type.
> 
> How about:
> 
> enum rte_fib_lookup_type {
>    RTE_FIB_LOOKUP_DEFAULT,
>    RTE_FIB_LOOKUP_DIR24_8_SCALAR_MACRO,
>    RTE_FIB_LOOKUP_DIR24_8_SCALAR_INLINE,
>    RTE_FIB_LOOKUP_DIR24_8_SCALAR_UNI,
>    RTE_FIB_LOOKUP_DIR24_8_VECTOR_AVX512,
> };
> 
> 

I introduced special magic value to select the "best" possible lookup 
implementation in "fib: introduce AVX512 lookup patch":
+       RTE_FIB_DIR24_8_ANY = UINT32_MAX
+       /**< Selects the best implementation based on the max simd 
bitwidth */
and I wanted to get rid of dir24_8 in names after I remove all 
unnecessary lookup implementations in the separate patches.

But I'm OK with your suggestion, I will reflect it in v15.


>>   /** FIB configuration structure */
>>   struct rte_fib_conf {
>>          enum rte_fib_type type; /**< Type of FIB struct */
>> @@ -196,6 +211,23 @@ __rte_experimental
>>   struct rte_rib *
>>   rte_fib_get_rib(struct rte_fib *fib);
>>
>> +/**
>> + * Set lookup function based on type
>> + *
>> + * @param fib
>> + *   FIB object handle
>> + * @param type
>> + *   type of lookup function
>> + *
>> + * @return
>> + *    -EINVAL on failure
>> + *    0 on success
>> + */
>> +__rte_experimental
>> +int
>> +rte_fib_set_lookup_fn(struct rte_fib *fib,
>> +       enum rte_fib_dir24_8_lookup_type type);
>> +
> 
> _fn does not give much info, how about rte_fib_select_lookup ?
> 

rte_fib_select_lookup is OK, I will rename it in v15 as well.

> 
>>   #ifdef __cplusplus
>>   }
>>   #endif
> 
> 

-- 
Regards,
Vladimir