Re: [PATCH dpdk] fib,rib: implement full network order support for IPv4

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

Hi Vladimir,

Sorry for the late reply, your email was in spam...

Medvedkin, Vladimir, Sep 30, 2026 at 19:12:
> Hi Robin,
>
> Thanks for the patch.
>
> check-git-log.sh reports
> Wrong headline format:
>          fib,rib: implement full network order support for IPv4
> could you please split for 2 patches?

I will try. The changes are intertwined somehow.

> A few comments inline.
> Also reflect changes in release notes please.

Ack.

>> +	ret = rte_fib_add(fib, ip_be, 24, nh_set);
>> +	RTE_TEST_ASSERT(ret == 0, "Failed to add route\n");
> it is probably worth to add an extra more specific route to better 
> coverage. Or keep RIB in host order and byteswap inside the 
> rte_fib_add() as I suggested below

>> +int32_t
>> +test_network_order(void)
>> +{
>> +	struct rte_rib *rib = NULL;
>> +	struct rte_rib_node *node;
>> +	struct rte_rib_conf config;
>> +
>> +	uint32_t ip_he = RTE_IPV4(192, 0, 2, 0);
> just a nit: ip_he does it stands for "host endian"? ip_ho instead for 
> "host order"?

Yes, to match with be (Big Endian).

>> +/*
>> + * modify_fib operates in host byte order. When the RIB carries the
>> + * network-order flag, rte_rib_get_nxt/rte_rib_get_ip return addresses
>
> nit: rte_rib_get_nxt() does not return address directly. It accepts it as an argument

I will rephrase that comment.

>> +	rib_conf.flags = (conf->flags & RTE_FIB_F_NETWORK_ORDER) ?
>> +		RTE_RIB_F_NETWORK_ORDER : 0;
> do we really need to align the RIB and FIB byte order? Since the RIB 
> structure is internal, why not keep it in host order and byteswap just 
> once at the API boundary, i.e. in _fib_add/del? In that case, we 
> wouldn’t need to touch the internal algorithm-specific code (dir24_8). 
> This would make things an order of magnitude simpler and cleaner.

Yes, because the control plane will iterate over the RIB and will expect
addresses to be in a consistent endianness.

Honestly, I would really prefer if that host-order thing was only an
internal implementation detail of the FIB algorithm and that *all* user
facing APIs only dealt with *network order* addresses.

But that would be a hard-break of the ABI. Would that be so terrible?
Who reasonably needs IPv4 host-order addresses for real world
applications?

>> -/** If set, fib lookup is expecting IPv4 address in network byte order */
>> -#define RTE_FIB_F_LOOKUP_NETWORK_ORDER 1
>> -#define RTE_FIB_ALLOWED_FLAGS (RTE_FIB_F_LOOKUP_NETWORK_ORDER)
>> +/** If set, all user-facing IPv4 addresses are in network byte order */
>> +#define RTE_FIB_F_NETWORK_ORDER 1
>> +#define RTE_FIB_F_LOOKUP_NETWORK_ORDER \
>> +	(RTE_DEPRECATED(RTE_FIB_F_LOOKUP_NETWORK_ORDER) RTE_FIB_F_NETWORK_ORDER)
> I'd suggest to keep 2 flags at the moment - the old one just for lookup 
> and a new one for lookup and control plane operations

OK, depending on your decision on my previous question, I'll change
accordingly.

>> +#define RIB_NODE_NET_ORDER	2
> RTE_RIB_NET_ORDER_NODE, just to avoid confusion with current naming? 
> Also since this is a bit flag it would be better to define it via bit 
> shift instead of a power of 2 digit

OK, same thing as above. If we can decide to change all user-facing APIs
to big-endian, we don't need this flag ;)

>> +	if (node->flag & RIB_NODE_NET_ORDER)
>> +		*ip = rte_cpu_to_be_32(*ip);
> plz reflect this change of behavior in doxygen

Ditto.

>> --- a/lib/rib/rte_rib.h
>> +++ b/lib/rib/rte_rib.h
>> @@ -17,6 +17,7 @@
>>   #include <stdlib.h>
>>   #include <stdint.h>
>>   
>> +#include <rte_byteorder.h>
> why include it here in .h?

I need to revisit this patch. I don't remember.

>> +/** If set, all user-facing IPv4 addresses are in network byte order */
>> +#define RTE_RIB_F_NETWORK_ORDER 1
>> +
>>   /** RIB configuration structure */
>>   struct rte_rib_conf {
>>   	/**
>> @@ -46,6 +50,7 @@ struct rte_rib_conf {
>>   	size_t	ext_sz;
>>   	/* size of rte_rib_node's pool */
>>   	int	max_nodes;
>> +	unsigned int flags; /**< Optional feature flags from RTE_RIB_F_* */
> old tests (and all possible places we're using rte_rib_conf struct) 
> should be updated to account for this new flag

I thought I had updated all sites, but I will make a sweep.

Thanks for the review.

-- 
Robin

# Contains a substantial amount of non-tobacco ingredients.