Re: [PATCH v1 04/12] node: add process callback for IP4 FIB

["Medvedkin, Vladimir" <vladimir.medvedkin@intel.com>]

Hi Ankur,

On 15/04/2025 13:10, Ankur Dwivedi wrote:
> Adds the process callback function for ip4_lookup_fib node.
>
> Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
> ---
>   lib/node/ip4_lookup_fib.c | 164 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 164 insertions(+)
>
> diff --git a/lib/node/ip4_lookup_fib.c b/lib/node/ip4_lookup_fib.c
> index e87864e672..c535b191f8 100644
> --- a/lib/node/ip4_lookup_fib.c
> +++ b/lib/node/ip4_lookup_fib.c
> @@ -40,6 +40,169 @@ static struct ip4_lookup_fib_node_main ip4_lookup_fib_nm;
>   #define IP4_LOOKUP_NODE_PRIV1_OFF(ctx) \
>   	(((struct ip4_lookup_fib_node_ctx *)ctx)->mbuf_priv1_off)
>   
> +static uint16_t
> +ip4_lookup_fib_node_process(struct rte_graph *graph, struct rte_node *node, void **objs,
> +			    uint16_t nb_objs)
> +{
> +	struct rte_mbuf *mbuf0, *mbuf1, *mbuf2, *mbuf3, **pkts;
> +	struct rte_fib *fib = IP4_LOOKUP_NODE_FIB(node->ctx);
> +	const int dyn = IP4_LOOKUP_NODE_PRIV1_OFF(node->ctx);
> +	struct rte_ipv4_hdr *ipv4_hdr;
> +	uint64_t next_hop[nb_objs];
> +	uint16_t lookup_err = 0;
> +	void **to_next, **from;
> +	uint16_t last_spec = 0;
> +	rte_edge_t next_index;
> +	uint16_t n_left_from;
> +	uint32_t ip[nb_objs];
> +	uint16_t held = 0;
> +	uint32_t drop_nh;
> +	uint16_t next;
> +	int i, rc;
> +
> +	/* Speculative next */
> +	next_index = RTE_NODE_IP4_LOOKUP_NEXT_REWRITE;
> +	/* Drop node */
> +	drop_nh = ((uint32_t)RTE_NODE_IP4_LOOKUP_NEXT_PKT_DROP) << 16;
> +
> +	pkts = (struct rte_mbuf **)objs;
> +	from = objs;
> +	n_left_from = nb_objs;
> +
> +	/* Get stream for the speculated next node */
> +	to_next = rte_node_next_stream_get(graph, node, next_index, nb_objs);
> +
> +	for (i = OBJS_PER_CLINE; i < RTE_GRAPH_BURST_SIZE; i += OBJS_PER_CLINE)
> +		rte_prefetch0(&objs[i]);

Does this prefetching loop make any sense? Unless objs are not passed 
across threads this array likely to be in the cache already.

And if objs are passed across threads, then why do you start prefetching 
from the next cache line instead of the first, and why don't you stop at 
nb_objs?

> +
> +#if RTE_GRAPH_BURST_SIZE > 64
> +	for (i = 0; i < 4 && i < n_left_from; i++) {
> +		rte_prefetch0(pkts[i]);
> +		rte_prefetch0(rte_pktmbuf_mtod_offset(pkts[i], void *,
> +					sizeof(struct rte_ether_hdr)));

This construction does not make sense to me. Same as similar 
constructions below

The second prefetch has memory dependency of the data, that will be 
prefetched by the first one. Does removing this prefetch affects 
performance?

> +	}
> +#endif
> +
> +	i = 0;
> +	while (n_left_from >= 4) {
> +#if RTE_GRAPH_BURST_SIZE > 64
> +		if (likely(n_left_from > 7)) {
> +			rte_prefetch0(pkts[4]);
> +			rte_prefetch0(rte_pktmbuf_mtod_offset(pkts[4], void *,
> +					sizeof(struct rte_ether_hdr)));
> +			rte_prefetch0(pkts[5]);
> +			rte_prefetch0(rte_pktmbuf_mtod_offset(pkts[5], void *,
> +					sizeof(struct rte_ether_hdr)));
> +			rte_prefetch0(pkts[6]);
> +			rte_prefetch0(rte_pktmbuf_mtod_offset(pkts[6], void *,
> +					sizeof(struct rte_ether_hdr)));
> +			rte_prefetch0(pkts[7]);
> +			rte_prefetch0(rte_pktmbuf_mtod_offset(pkts[7], void *,
> +					sizeof(struct rte_ether_hdr)));
> +		}
> +#endif
> +
> +		mbuf0 = pkts[0];
> +		mbuf1 = pkts[1];
> +		mbuf2 = pkts[2];
> +		mbuf3 = pkts[3];
> +		pkts += 4;
> +		n_left_from -= 4;
> +		/* Extract DIP of mbuf0 */
> +		ipv4_hdr = rte_pktmbuf_mtod_offset(mbuf0, struct rte_ipv4_hdr *,
> +				sizeof(struct rte_ether_hdr));
> +		/* Extract cksum, ttl as ipv4 hdr is in cache */
> +		node_mbuf_priv1(mbuf0, dyn)->cksum = ipv4_hdr->hdr_checksum;
> +		node_mbuf_priv1(mbuf0, dyn)->ttl = ipv4_hdr->time_to_live;
> +
> +		ip[i++] = rte_be_to_cpu_32(ipv4_hdr->dst_addr);
> +
> +		/* Extract DIP of mbuf1 */
> +		ipv4_hdr = rte_pktmbuf_mtod_offset(mbuf1, struct rte_ipv4_hdr *,
> +				sizeof(struct rte_ether_hdr));
> +		/* Extract cksum, ttl as ipv4 hdr is in cache */
> +		node_mbuf_priv1(mbuf1, dyn)->cksum = ipv4_hdr->hdr_checksum;
> +		node_mbuf_priv1(mbuf1, dyn)->ttl = ipv4_hdr->time_to_live;
> +
> +		ip[i++] = rte_be_to_cpu_32(ipv4_hdr->dst_addr);
> +
> +		/* Extract DIP of mbuf2 */
> +		ipv4_hdr = rte_pktmbuf_mtod_offset(mbuf2, struct rte_ipv4_hdr *,
> +				sizeof(struct rte_ether_hdr));
> +		/* Extract cksum, ttl as ipv4 hdr is in cache */
> +		node_mbuf_priv1(mbuf2, dyn)->cksum = ipv4_hdr->hdr_checksum;
> +		node_mbuf_priv1(mbuf2, dyn)->ttl = ipv4_hdr->time_to_live;
> +
> +		ip[i++] = rte_be_to_cpu_32(ipv4_hdr->dst_addr);
> +
> +		/* Extract DIP of mbuf3 */
> +		ipv4_hdr = rte_pktmbuf_mtod_offset(mbuf3, struct rte_ipv4_hdr *,
> +				sizeof(struct rte_ether_hdr));
> +
> +		/* Extract cksum, ttl as ipv4 hdr is in cache */
> +		node_mbuf_priv1(mbuf3, dyn)->cksum = ipv4_hdr->hdr_checksum;
> +		node_mbuf_priv1(mbuf3, dyn)->ttl = ipv4_hdr->time_to_live;
> +
> +		ip[i++] = rte_be_to_cpu_32(ipv4_hdr->dst_addr);
> +	}
> +	while (n_left_from > 0) {
> +		mbuf0 = pkts[0];
> +		pkts += 1;
> +		n_left_from -= 1;
> +
> +		/* Extract DIP of mbuf0 */
> +		ipv4_hdr = rte_pktmbuf_mtod_offset(mbuf0, struct rte_ipv4_hdr *,
> +				sizeof(struct rte_ether_hdr));
> +		/* Extract cksum, ttl as ipv4 hdr is in cache */
> +		node_mbuf_priv1(mbuf0, dyn)->cksum = ipv4_hdr->hdr_checksum;
> +		node_mbuf_priv1(mbuf0, dyn)->ttl = ipv4_hdr->time_to_live;
> +
> +		ip[i++] = rte_be_to_cpu_32(ipv4_hdr->dst_addr);
> +	}
> +
> +	rc = rte_fib_lookup_bulk(fib, ip, next_hop, nb_objs);
> +	if (unlikely(rc != 0))
> +		return 0;
> +
> +	for (i = 0; i < nb_objs; i++) {
> +		if (unlikely(next_hop[i] == FIB_DEFAULT_NH)) {
> +			next_hop[i] = drop_nh;
maybe it is worth just do define FIB_DEFAULT_NH as a drop_nh vaue to 
omit these next_hop reassignments?
> +			lookup_err += 1;
> +		}
> +
> +		mbuf0 = (struct rte_mbuf *)objs[i];
> +		node_mbuf_priv1(mbuf0, dyn)->nh = (uint16_t)next_hop[i];
> +		next = (uint16_t)(next_hop[i] >> 16);
> +
> +		if (unlikely(next_index ^ next)) {
> +			/* Copy things successfully speculated till now */
> +			rte_memcpy(to_next, from, last_spec * sizeof(from[0]));
> +			from += last_spec;
> +			to_next += last_spec;
> +			held += last_spec;
> +			last_spec = 0;
> +
> +			rte_node_enqueue_x1(graph, node, next, from[0]);
> +			from += 1;
> +		} else {
> +			last_spec += 1;
> +		}
> +	}
> +
> +	/* !!! Home run !!! */
> +	if (likely(last_spec == nb_objs)) {
> +		rte_node_next_stream_move(graph, node, next_index);
> +		return nb_objs;
> +	}
> +
> +	NODE_INCREMENT_XSTAT_ID(node, 0, lookup_err != 0, lookup_err);
> +	held += last_spec;
> +	rte_memcpy(to_next, from, last_spec * sizeof(from[0]));
> +	rte_node_next_stream_put(graph, node, next_index, held);
> +
> +	return nb_objs;
> +}
> +
>   RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_node_ip4_fib_route_add, 25.07)
>   int
>   rte_node_ip4_fib_route_add(uint32_t ip, uint8_t depth, uint16_t next_hop,
> @@ -147,6 +310,7 @@ static struct rte_node_xstats ip4_lookup_fib_xstats = {
>   };
>   
>   static struct rte_node_register ip4_lookup_fib_node = {
> +	.process = ip4_lookup_fib_node_process,
>   	.name = "ip4_lookup_fib",
>   
>   	.init = ip4_lookup_fib_node_init,

-- 
Regards,
Vladimir