Hi Robin,
On 10/16/2026 4:56 PM, Robin Jarry wrote:
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?
I think we can't just give up host order, we need to maintain some level
of backward compatibility with the good old LPM library. People have
been using it for a long time and have built their code around its API,
which uses host order in both the control and data planes.
FIB keeps the same semantics, which makes migrating from LPM
straightforward. The problem with a hard break is not only the ABI,
function prototypes remains the same (use uint32_t), so existing apps
would still compile and links without warnings, they just will install
byte-swapped routes.
The existing RTE_FIB_F_LOOKUP_NETWORK_ORDER flag has been there since
24.11 and is part of two LTS releases, so I would keep it as is, at
least for a while.
However you have a fair point about the RIB, it is exposed through
rte_fib_get_rib(), it is not 100% internal, and user may expect to have
the same byte order as was used for FIB add/del. I'm fine with
propagating the flag to the internal RIB, although we can just mark host
order in documentation.
So I'd suggest:
- keep host order (for both CP and DP ops) as default (to maintain
alignment with legacy LPM)
- keep RTE_FIB_F_LOOKUP_NETWORK_ORDER unchanged (lookup only)
- add a new flag that makes all IPv4 operations (CP and DP) use network
order (with this flag order propagated to the RIB if you think it would
be better)
-/** 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.
--
Regards,
Vladimir
