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

[Nithin Dabilpuram]

On Fri, Apr 10, 2020 at 01:04:37AM +0200, Andrzej Ostruszka wrote:
> On 4/5/20 10:56 AM, jerinj at marvell.com wrote:
> > From: Nithin Dabilpuram <ndabilpuram at 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 at marvell.com>
> > Signed-off-by: Pavan Nikhilesh <pbhagavatula at marvell.com>
> > Signed-off-by: Kiran Kumar K <kirankumark at 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.

Agree. I'll mask out upper 8 bits here in next patch. 
Since next_node is enum and is in control of lookup node implementation, 
BIT(23:16) for "next_node" should be sufficient with space for 256 next nodes
for now.

> 
> > +	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
>