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.
