rte_fib network order bug

[Medvedkin, Vladimir]

I think control plane API should work with prefix addresses in CPU byte order. At least our RTE_IPV4 macro works this way. Also, prefix is an address + prefix length (not the mask), so it is more natural if address is in cpu byte order.
Also, I think byte swap should be done on the interface where byte order changes, and this boundary lies outside the FIB library. However, I've added this feature not only because it was asked, but also trying to improve performance in some cases, such as using AVX512 byte swap in vector path for users who don't want to bother about manually do byteswap on the fast path.

Why do you think this would discourage users?


-----Original Message-----
From: Robin Jarry <rjarry at redhat.com> 
Sent: Wednesday, November 13, 2024 1:27 PM
To: Medvedkin, Vladimir <vladimir.medvedkin at intel.com>; dev at dpdk.org
Subject: Re: rte_fib network order bug

Medvedkin, Vladimir, Nov 13, 2024 at 11:42:
> Hi Robin,
>
> It should not. Here is documentation says regarding this flag:
>
> /** If set, fib lookup is expecting IPv4 address in network byte order 
> */ #define RTE_FIB_F_NETWORK_ORDER    1
>
> As stated above lookups will be performed while expecting addresses to 
> be in BE byte order. Control plane API expects IPv4 prefix address to 
> be in CPU byte order.

I had not understood that it was *only* the lookups that were network order.

The original reason why a RTE_FIB_F_NETWORK_ORDER flag was suggested some time ago is that inet_pton() always returns network order addresses. It makes it much more natural to keep everything in network order instead of having to swap things around.

Now, only having the lookup functions requiring network order addresses but all the other functions in the API requiring host order addresses is even more confusing.

In my opinion, when RTE_FIB_F_NETWORK_ORDER is set, *all* functions of the fib API should take network order addresses. That obviously mean that rib should also have a similar flag.

Is that possible without a massive rework? If not, then I think we should revert the patch that adds it or at least discourage its use in its current state.

What do you think?