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

["Robin Jarry" <rjarry@redhat.com>]

Hi Vladimir,

Vladimir Medvedkin, Oct 10, 2024 at 13:26:
> Previously when running rte_fib_lookup IPv4 addresses must have been in
> host byte order.
>
> This patch adds a new flag RTE_FIB_FLAG_LOOKUP_BE that can be passed on
> fib create, which will allow to have IPv4 in network byte order on
> lookup.
>
> Signed-off-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>

[snip]

> diff --git a/lib/fib/dir24_8.h b/lib/fib/dir24_8.h
> index 7125049f15..2c776e118f 100644
> --- a/lib/fib/dir24_8.h
> +++ b/lib/fib/dir24_8.h
> @@ -7,7 +7,9 @@
>  #define _DIR24_8_H_
>  
>  #include <stdalign.h>
> +#include <stdbool.h>
>  
> +#include <rte_byteorder.h>
>  #include <rte_prefetch.h>
>  #include <rte_branch_prediction.h>
>  
> @@ -237,6 +239,46 @@ dir24_8_lookup_bulk_uni(void *p, const uint32_t *ips,
>  	}
>  }
>  
> +#define BSWAP_MAX_LENGTH	64
> +
> +typedef void (*dir24_8_lookup_bulk_be_cb)(void *p, const uint32_t *ips,
> +	uint64_t *next_hops, const unsigned int n);
> +
> +static inline void
> +dir24_8_lookup_bulk_be(void *p, const uint32_t *ips,
> +	uint64_t *next_hops, const unsigned int n,
> +	dir24_8_lookup_bulk_be_cb cb)
> +{
> +	uint32_t le_ips[BSWAP_MAX_LENGTH];
> +	unsigned int i;
> +
> +	for (i = 0; i < n; i += BSWAP_MAX_LENGTH) {
> +		int j;
> +		for (j = 0; j < BSWAP_MAX_LENGTH && i + j < n; j++)
> +			le_ips[j] = rte_be_to_cpu_32(ips[i + j]);
> +
> +		cb(p, le_ips, next_hops + i, j);
> +	}

This should be a noop for big endian platforms. I'm not sure the 
complier will be smart enough to collapse the nested loops.

> +}
> +
> +#define DECLARE_BE_LOOKUP_FN(name)					\
> +static inline void							\
> +name##_be(void *p, const uint32_t *ips,					\
> +	uint64_t *next_hops, const unsigned int n)			\
> +{									\
> +	dir24_8_lookup_bulk_be(p, ips, next_hops, n, name);		\
> +}
> +
> +DECLARE_BE_LOOKUP_FN(dir24_8_lookup_bulk_1b)
> +DECLARE_BE_LOOKUP_FN(dir24_8_lookup_bulk_2b)
> +DECLARE_BE_LOOKUP_FN(dir24_8_lookup_bulk_4b)
> +DECLARE_BE_LOOKUP_FN(dir24_8_lookup_bulk_8b)
> +DECLARE_BE_LOOKUP_FN(dir24_8_lookup_bulk_0)
> +DECLARE_BE_LOOKUP_FN(dir24_8_lookup_bulk_1)
> +DECLARE_BE_LOOKUP_FN(dir24_8_lookup_bulk_2)
> +DECLARE_BE_LOOKUP_FN(dir24_8_lookup_bulk_3)
> +DECLARE_BE_LOOKUP_FN(dir24_8_lookup_bulk_uni)
> +
>  void *
>  dir24_8_create(const char *name, int socket_id, struct rte_fib_conf *conf);
>  
> @@ -244,7 +286,7 @@ void
>  dir24_8_free(void *p);
>  
>  rte_fib_lookup_fn_t
> -dir24_8_get_lookup_fn(void *p, enum rte_fib_lookup_type type);
> +dir24_8_get_lookup_fn(void *p, enum rte_fib_lookup_type type, bool be_addr);
>  
>  int
>  dir24_8_modify(struct rte_fib *fib, uint32_t ip, uint8_t depth,

[snip]

> diff --git a/lib/fib/rte_fib.h b/lib/fib/rte_fib.h
> index d7a5aafe53..1617235e85 100644
> --- a/lib/fib/rte_fib.h
> +++ b/lib/fib/rte_fib.h
> @@ -28,6 +28,9 @@ struct rte_rib;
>  /** Maximum depth value possible for IPv4 FIB. */
>  #define RTE_FIB_MAXDEPTH	32
>  
> +/** If set fib lookup is expecting ipv4 in network byte order */
> +#define RTE_FIB_FLAG_LOOKUP_BE	1

I think RTE_FIB_F_NETWORK_ORDER would be more appropriate.

> +
>  /** Type of FIB struct */
>  enum rte_fib_type {
>  	RTE_FIB_DUMMY,		/**< RIB tree based FIB */
> @@ -76,6 +79,7 @@ enum rte_fib_lookup_type {
>  /** FIB configuration structure */
>  struct rte_fib_conf {
>  	enum rte_fib_type type; /**< Type of FIB struct */
> +	unsigned int flags;

Maybe use an explicit int size for flags like uint32_t? I doubt we'll 
ever need more than 32 flags.

Also, maybe it would be better to add this field at the end to avoid 
breaking the API?

You forgot to add a doc string for that field:

    uint32_t flags; /**< Optional feature flags from RTE_FIB_F_* **/

Thanks!