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

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 19-Feb-21 3:09 PM, 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>
> ---

<snip>

> +			 || defined RTE_ARCH_PPC_64
> +#define FIB_SEND_MULTI
> +#endif
> +
> +static struct rte_fib *ipv4_l3fwd_fib_lookup_struct[NB_SOCKETS];
> +static struct rte_fib6 *ipv6_l3fwd_fib_lookup_struct[NB_SOCKETS];
> +
> +/* Parse packet type and ip address. */
> +static inline void
> +fib_parse_packet(struct rte_mbuf *mbuf,
> +		       uint32_t *ipv4, uint32_t *ipv4_cnt,
> +		       uint8_t ipv6[RTE_FIB6_IPV6_ADDR_SIZE],
> +		       uint32_t *ipv6_cnt, uint8_t *ip_type)

Nitpicking, but here and in a bunch of other places, the indentation is 
quite odd :)

<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_reassem = 0, ipv6_reassem = 0;

I don't quite follow the naming here - this looks like it's 
"reassembling" something but i don't see any IP reassembly going on? 
Artifacts of copy-paste?

> +	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]) {
> +			if (hopsv4[ipv4_reassem] != FIB_DEFAULT_HOP)
> +				hops[i] = (uint16_t)hopsv4[ipv4_reassem];
> +			else
> +				hops[i] = portid;
> +			ipv4_reassem++;

Nitpicking, but could be made slightly more concise and readable:

const uint16_t nh = (uint16_t)hopsv4[ipv4_reassem++];
hops[i] = nh != FIB_DEFAULT_HOP ? nh : portid;

Same for IPv6.

-- 
Thanks,
Anatoly