Re: [dpdk-dev] [PATCH v4 23/29] node: add ipv4 rewrite and lookup ctrl API

[Andrzej Ostruszka <amo@semihalf.com>]

On 4/5/20 10:56 AM, jerinj@marvell.com wrote:
> From: Nithin Dabilpuram <ndabilpuram@marvell.com>
> 
> Add ip4_rewrite and ip4_lookup ctrl API. ip4_lookup ctrl
> API is used to add route entries for LPM lookup with
> result data containing next hop id and next proto.
> ip4_rewrite ctrl API is used to add rewrite data for
> every next hop.
> 
> Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> Signed-off-by: Kiran Kumar K <kirankumark@marvell.com>
> ---
[...]
> @@ -93,6 +97,18 @@ rte_node_eth_config(struct rte_node_ethdev_config *conf, uint16_t nb_confs,
>  
>  		node_dbg("ethdev", "Tx node %s-%s: is at %u", tx_node->name,
>  			 name, id);
> +
> +		/* Prepare the actual name of the cloned node */
> +		snprintf(name, sizeof(name), "ethdev_tx-%u", port_id);
> +
> +		/* Add this tx port node as next to ip4_rewrite_node */
> +		rte_node_edge_update(ip4_rewrite_node->id, RTE_EDGE_ID_INVALID,
> +				     &next_nodes, 1);

Maybe I've missed something but it looks to me like all uses are of
"append" kind.  Either during initialization (with 0) or here, so maybe
a chance to simplify API?

> +		/* Assuming edge id is the last one alloc'ed */
> +		rc = ip4_rewrite_set_next(
> +			port_id, rte_node_edge_count(ip4_rewrite_node->id) - 1);
> +		if (rc < 0)
> +			return rc;
>  	}
>  
>  	ctrl.nb_graphs = nb_graphs;
> diff --git a/lib/librte_node/ip4_lookup.c b/lib/librte_node/ip4_lookup.c
> index 3a38f5ad8..d10d17879 100644
> --- a/lib/librte_node/ip4_lookup.c
> +++ b/lib/librte_node/ip4_lookup.c
> @@ -28,6 +28,8 @@ struct ip4_lookup_node_main {
>  	struct rte_lpm *lpm_tbl[RTE_MAX_NUMA_NODES];
>  };
>  
> +static struct ip4_lookup_node_main ip4_lookup_nm;
> +
>  #if defined(RTE_MACHINE_CPUFLAG_NEON)
>  #include "ip4_lookup_neon.h"
>  #elif defined(RTE_ARCH_X86)
> @@ -109,12 +111,90 @@ ip4_lookup_node_process(struct rte_graph *graph, struct rte_node *node,
>  
>  #endif
>  
> +int
> +rte_node_ip4_route_add(uint32_t ip, uint8_t depth, uint16_t next_hop,
> +		       enum rte_node_ip4_lookup_next next_node)
> +{
> +	char abuf[INET6_ADDRSTRLEN];
> +	struct in_addr in;
> +	uint8_t socket;
> +	uint32_t val;
> +	int ret;
> +
> +	in.s_addr = htonl(ip);
> +	inet_ntop(AF_INET, &in, abuf, sizeof(abuf));
> +	/* Embedded next node id in next hop */
> +	val = (next_node << 16) | next_hop;

I guess this assumes that the next hop is 32 bits.  I might be
misunderstanding the implementation but it looks to me like it is 24bits
(the docs still say "the user data is 1-byte long" though), at least
this is my impression from struct rte_lpm_tbl_entry.

> +	node_dbg("ip4_lookup", "LPM: Adding route %s / %d nh (0x%x)", abuf,
> +		 depth, val);
> +
> +	for (socket = 0; socket < RTE_MAX_NUMA_NODES; socket++) {
> +		if (!ip4_lookup_nm.lpm_tbl[socket])
> +			continue;
> +
> +		ret = rte_lpm_add(ip4_lookup_nm.lpm_tbl[socket], ip, depth,
> +				  val);
> +
> +		if (ret < 0) {
> +			node_err("ip4_lookup",
> +				 "Unable to add entry %s / %d nh (%x) to LPM table on sock %d, rc=%d\n",
> +				 abuf, depth, val, socket, ret);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}

With regards
Andrzej Ostruszka