Re: [dpdk-dev] [PATCH v4 4/5] examples/l3fwd: implement FIB lookup method

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

Hi Conor,

see comment inlined

On 11/03/2021 12:01, Conor Walsh wrote:
> This patch implements the Forwarding Information Base (FIB) library
> in l3fwd using the function calls and infrastructure introduced in
> the previous patch.
> 
> Signed-off-by: Conor Walsh <conor.walsh@intel.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
>   examples/l3fwd/l3fwd_fib.c | 482 ++++++++++++++++++++++++++++++++++++-
>   1 file changed, 476 insertions(+), 6 deletions(-)
> 

<snip>

> +/* Bulk parse, fib lookup and send. */
> +static inline void
> +fib_send_packets(int nb_rx, struct rte_mbuf **pkts_burst,
> +		uint16_t portid, struct lcore_conf *qconf)
> +{
> +	uint32_t ipv4_arr[nb_rx];
> +	uint8_t ipv6_arr[nb_rx][RTE_FIB6_IPV6_ADDR_SIZE];
> +	uint16_t hops[nb_rx];
> +	uint64_t hopsv4[nb_rx], hopsv6[nb_rx];
> +	uint8_t type_arr[nb_rx];
> +	uint32_t ipv4_cnt = 0, ipv6_cnt = 0;
> +	uint32_t ipv4_arr_assem = 0, ipv6_arr_assem = 0;
> +	uint16_t nh;
> +	int32_t i;
> +
> +	/* Prefetch first packets. */
> +	for (i = 0; i < FIB_PREFETCH_OFFSET && i < nb_rx; i++)
> +		rte_prefetch0(rte_pktmbuf_mtod(pkts_burst[i], void *));
> +
> +	/* Parse packet info and prefetch. */
> +	for (i = 0; i < (nb_rx - FIB_PREFETCH_OFFSET); i++) {
> +		/* Prefetch packet. */
> +		rte_prefetch0(rte_pktmbuf_mtod(pkts_burst[
> +				i + FIB_PREFETCH_OFFSET], void *));
> +		fib_parse_packet(pkts_burst[i],
> +				&ipv4_arr[ipv4_cnt], &ipv4_cnt,
> +				ipv6_arr[ipv6_cnt], &ipv6_cnt,
> +				&type_arr[i]);
> +	}
> +
> +	/* Parse remaining packet info. */
> +	for (; i < nb_rx; i++)
> +		fib_parse_packet(pkts_burst[i],
> +				&ipv4_arr[ipv4_cnt], &ipv4_cnt,
> +				ipv6_arr[ipv6_cnt], &ipv6_cnt,
> +				&type_arr[i]);
> +
> +	/* Lookup IPv4 hops if IPv4 packets are present. */
> +	if (likely(ipv4_cnt > 0))
> +		rte_fib_lookup_bulk(qconf->ipv4_lookup_struct,
> +				ipv4_arr, hopsv4, ipv4_cnt);
> +
> +	/* Lookup IPv6 hops if IPv6 packets are present. */
> +	if (ipv6_cnt > 0)
> +		rte_fib6_lookup_bulk(qconf->ipv6_lookup_struct,
> +				ipv6_arr, hopsv6, ipv6_cnt);
> +
> +	/* Add IPv4 and IPv6 hops to one array depending on type. */
> +	for (i = 0; i < nb_rx; i++) {
> +		if (type_arr[i])
> +			nh = (uint16_t)hopsv4[ipv4_arr_assem++];
> +		else
> +			nh = (uint16_t)hopsv6[ipv6_arr_assem++];
> +		hops[i] = (nh != FIB_DEFAULT_HOP && nh <= RTE_MAX_ETHPORTS &&
> +			(enabled_port_mask & 1 << nh) != 0) ? nh : portid;

I think you can get rid of
"nh <= RTE_MAX_ETHPORTS && (enabled_port_mask & 1 << nh) != 0"
because it can be controlled during initialization when installing 
routes to the table. So you can check it just before rte_fib_add() and 
install FIB_DEFAULT_HOP if needed.

Apart from that LGTM.


> +	}
> +
> +#if defined FIB_SEND_MULTI
> +	send_packets_multi(qconf, pkts_burst, hops, nb_rx);
> +#else
> +	fib_send_single(nb_rx, qconf, pkts_burst, hops);
> +#endif
> +}
> +

<snip>

> 

-- 
Regards,
Vladimir