RE: rte_fib network order bug

["Morten Brørup" <mb@smartsharesystems.com>]

> -----Original Message-----
> From: Medvedkin, Vladimir [mailto:vladimir.medvedkin@intel.com]
> Sent: Wednesday, 13 November 2024 20.39
> To: Robin Jarry; dev@dpdk.org
> Subject: RE: rte_fib network order bug
> 
> 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?
> 

Joining the discussing with a couple of comments.

1. When I saw the byte order flag the first time, it was not clear to me either that it only affected lookups - I too thought it covered the entire API of the library. This needs to be emphasized in the description of the flag. And the flag's name should contain LOOKUP, e.g.:

/** If set, FIB lookup is expecting IPv4 address in network byte order. Note: Only lookup! */
#define RTE_FIB_F_LOOKUP_NETWORK_ORDER    1

2. Control plane API should use CPU byte order. I consider inet_pton() irrelevant in this context.
Adding network byte order lookup for fast path optimization makes good sense, and adding it to the RIB library would be nice too.

If it was an address table (not longest prefix table, but a hash or similar), the learn()/update() function could be fast path, and thus support network byte order too; but it's not, so add() is control plane.

> 
> -----Original Message-----
> From: Robin Jarry <rjarry@redhat.com>
> Sent: Wednesday, November 13, 2024 1:27 PM
> To: Medvedkin, Vladimir <vladimir.medvedkin@intel.com>; dev@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?