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