RE: [EXTERNAL] Re: [PATCH v1 02/12] node: add IP4 lookup FIB node

[Ankur Dwivedi <adwivedi@marvell.com>]

Hi Vladimir,
>On 15/04/2025 13:10, Ankur Dwivedi wrote:
>> Adds a lookup FIB node for IP4.
>>
>> Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
>> ---
>>   lib/node/ip4_lookup_fib.c | 127
>++++++++++++++++++++++++++++++++++++++
>>   lib/node/meson.build      |   3 +-
>>   2 files changed, 129 insertions(+), 1 deletion(-)
>>   create mode 100644 lib/node/ip4_lookup_fib.c
>>
>> diff --git a/lib/node/ip4_lookup_fib.c b/lib/node/ip4_lookup_fib.c new
>> file mode 100644 index 0000000000..9c71610718
>> --- /dev/null
>> +++ b/lib/node/ip4_lookup_fib.c
>> @@ -0,0 +1,127 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause
>> + * Copyright(C) 2025 Marvell.
>> + */
>> +
>> +#include <rte_errno.h>
>> +#include <rte_ether.h>
>> +#include <rte_fib.h>
>> +#include <rte_graph.h>
>> +#include <rte_graph_worker.h>
>> +#include <rte_ip.h>
>> +
>> +#include "rte_node_ip4_api.h"
>> +
>> +#include "node_private.h"
>> +
>> +/* IP4 Lookup global data struct */
>> +struct ip4_lookup_fib_node_main {
>> +	struct rte_fib *fib[RTE_MAX_NUMA_NODES]; };
>> +
>> +struct ip4_lookup_fib_node_ctx {
>> +	/* Socket's FIB */
>> +	struct rte_fib *fib;
>> +	/* Dynamic offset to mbuf priv1 */
>> +	int mbuf_priv1_off;
>> +};
>> +
>> +static struct ip4_lookup_fib_node_main ip4_lookup_fib_nm;
>> +
>> +#define FIB_MAX_ROUTES (1 << 16)
>why only 64k routes? Modern BGP full view has about 1M prefixes
>> +#define FIB_NUM_TBL8   (1 << 15)
>> +#define FIB_DEFAULT_NH 999
>why this particular value? It is ok to use magic values in examples, but not for
>libs. Consider something like 0 or UINT{8,16,32,64}MAX or some meaningful
>value within graph infra
UINT{8,16,32,64}MAX should be fine.
Also, I am thinking about taking the values in fib conf as input from application.
>> +
>> +#define IP4_LOOKUP_NODE_FIB(ctx) \
>> +	(((struct ip4_lookup_fib_node_ctx *)ctx)->fib)
>> +
>> +#define IP4_LOOKUP_NODE_PRIV1_OFF(ctx) \
>> +	(((struct ip4_lookup_fib_node_ctx *)ctx)->mbuf_priv1_off)
>> +
>> +static int
>> +setup_fib(unsigned int socket)
>> +{
>> +	struct ip4_lookup_fib_node_main *nm = &ip4_lookup_fib_nm;
>> +	struct rte_fib_conf conf;
>> +	char s[RTE_FIB_NAMESIZE];
>> +
>> +	/* One fib per socket */
>> +	if (nm->fib[socket])
>> +		return 0;
>> +
>> +	conf.type = RTE_FIB_DIR24_8;
>> +	conf.default_nh = FIB_DEFAULT_NH;
>> +	conf.max_routes = FIB_MAX_ROUTES;
>> +	conf.rib_ext_sz = 0;
>> +	conf.dir24_8.nh_sz = RTE_FIB_DIR24_8_4B;
>> +	conf.dir24_8.num_tbl8 = FIB_NUM_TBL8;
>> +	conf.flags = 0;
>> +	snprintf(s, sizeof(s), "IPV4_LOOKUP_FIB_%d", socket);
>> +	nm->fib[socket] = rte_fib_create(s, socket, &conf);
>> +	if (nm->fib[socket] == NULL)
>> +		return -rte_errno;
>> +
>> +	return 0;
>> +}
>> +
>> +static int
>> +ip4_lookup_fib_node_init(const struct rte_graph *graph, struct
>> +rte_node *node) {
>> +	static uint8_t init_once;
>> +	unsigned int socket;
>> +	uint16_t lcore_id;
>> +	int rc;
>> +
>> +	RTE_BUILD_BUG_ON(sizeof(struct ip4_lookup_fib_node_ctx) >
>> +RTE_NODE_CTX_SZ);
>> +
>> +	if (!init_once) {
>> +		node_mbuf_priv1_dynfield_offset =
>rte_mbuf_dynfield_register(
>> +				&node_mbuf_priv1_dynfield_desc);
>> +		if (node_mbuf_priv1_dynfield_offset < 0)
>> +			return -rte_errno;
>> +
>> +		/* Setup FIB for all sockets */
>> +		RTE_LCORE_FOREACH(lcore_id)
>> +		{
>> +			socket = rte_lcore_to_socket_id(lcore_id);
>> +			rc = setup_fib(socket);
>> +			if (rc) {
>> +				node_err("ip4_lookup_fib",
>> +					 "Failed to setup fib for sock %u,
>rc=%d",
>> +					 socket, rc);
>> +				return rc;
>> +			}
>> +		}
>> +		init_once = 1;
>> +	}
>> +
>> +	/* Update socket's FIB and mbuf dyn priv1 offset in node ctx */
>> +	IP4_LOOKUP_NODE_FIB(node->ctx) = ip4_lookup_fib_nm.fib[graph-
>>socket];
>> +	IP4_LOOKUP_NODE_PRIV1_OFF(node->ctx) =
>> +node_mbuf_priv1_dynfield_offset;
>> +
>> +	node_dbg("ip4_lookup_fib", "Initialized ip4_lookup_fib node");
>> +
>> +	return 0;
>> +}
>> +
>> +static struct rte_node_xstats ip4_lookup_fib_xstats = {
>> +	.nb_xstats = 1,
>> +	.xstat_desc = {
>> +		[0] = "ip4_lookup_fib_error",
>> +	},
>> +};
>> +
>> +static struct rte_node_register ip4_lookup_fib_node = {
>> +	.name = "ip4_lookup_fib",
>> +
>> +	.init = ip4_lookup_fib_node_init,
>> +	.xstats = &ip4_lookup_fib_xstats,
>> +
>> +	.nb_edges = RTE_NODE_IP4_LOOKUP_NEXT_PKT_DROP + 1,
>> +	.next_nodes = {
>> +		[RTE_NODE_IP4_LOOKUP_NEXT_IP4_LOCAL] = "ip4_local",
>> +		[RTE_NODE_IP4_LOOKUP_NEXT_REWRITE] = "ip4_rewrite",
>> +		[RTE_NODE_IP4_LOOKUP_NEXT_PKT_DROP] = "pkt_drop",
>> +	},
>> +};
>> +
>> +RTE_NODE_REGISTER(ip4_lookup_fib_node);
>> diff --git a/lib/node/meson.build b/lib/node/meson.build index
>> 0bed97a96c..d2011c8f56 100644
>> --- a/lib/node/meson.build
>> +++ b/lib/node/meson.build
>> @@ -13,6 +13,7 @@ sources = files(
>>           'ethdev_tx.c',
>>           'ip4_local.c',
>>           'ip4_lookup.c',
>> +        'ip4_lookup_fib.c',
>>           'ip4_reassembly.c',
>>           'ip4_rewrite.c',
>>           'ip6_lookup.c',
>> @@ -34,4 +35,4 @@ headers = files(
>>
>>   # Strict-aliasing rules are violated by uint8_t[] to context size casts.
>>   cflags += '-fno-strict-aliasing'
>> -deps += ['graph', 'mbuf', 'lpm', 'ethdev', 'mempool', 'cryptodev',
>> 'ip_frag']
>> +deps += ['graph', 'mbuf', 'lpm', 'ethdev', 'mempool', 'cryptodev',
>> +'ip_frag', 'fib']
>
>--
>Regards,
>Vladimir