Re: [dpdk-dev] [PATCH v4 19/29] node: add generic ipv4 lookup node

[Nithin Dabilpuram <ndabilpuram@marvell.com>]

On Fri, Apr 10, 2020 at 03:50:06PM +0530, Nithin Dabilpuram wrote:
> On Fri, Apr 10, 2020 at 01:07:34AM +0200, Andrzej Ostruszka wrote:
> > On 4/5/20 10:56 AM, jerinj@marvell.com wrote:
> > > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > 
> > > Add IPv4 lookup process function for ip4_lookup node.
> > > This node performs LPM lookup using simple RTE_LPM API on every packet
> > > received and forwards it to a next node that is identified by lookup
> > > result.
> > > 
> > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> > [...]
> > > +static uint16_t
> > > +ip4_lookup_node_process(struct rte_graph *graph, struct rte_node *node,
> > > +			void **objs, uint16_t nb_objs)
> > > +{
> > > +	struct rte_ipv4_hdr *ipv4_hdr;
> > > +	void **to_next, **from;
> > > +	uint16_t last_spec = 0;
> > > +	struct rte_mbuf *mbuf;
> > > +	rte_edge_t next_index;
> > > +	struct rte_lpm *lpm;
> > > +	uint16_t held = 0;
> > > +	uint32_t drop_nh;
> > > +	int i, rc;
> > > +
> > > +	/* Speculative next */
> > > +	next_index = RTE_NODE_IP4_LOOKUP_NEXT_REWRITE;
> > > +	/* Drop node */
> > > +	drop_nh = ((uint32_t)RTE_NODE_IP4_LOOKUP_NEXT_PKT_DROP) << 16;
> > > +
> > > +	/* Get socket specific LPM from ctx */
> > > +	lpm = *((struct rte_lpm **)node->ctx);
> > > +	from = objs;
> > > +
> > > +	/* Get stream for the speculated next node */
> > > +	to_next = rte_node_next_stream_get(graph, node, next_index, nb_objs);
> > > +	for (i = 0; i < nb_objs; i++) {
> > > +		uint32_t next_hop;
> > > +		uint16_t next;
> > > +
> > > +		mbuf = (struct rte_mbuf *)objs[i];
> > > +
> > > +		/* Extract DIP of mbuf0 */
> > > +		ipv4_hdr = rte_pktmbuf_mtod_offset(mbuf, struct rte_ipv4_hdr *,
> > > +				sizeof(struct rte_ether_hdr));
> > > +		/* Extract cksum, ttl as ipv4 hdr is in cache */
> > > +		rte_node_mbuf_priv1(mbuf)->cksum = ipv4_hdr->hdr_checksum;
> > > +		rte_node_mbuf_priv1(mbuf)->ttl = ipv4_hdr->time_to_live;
> > > +
> > > +		rc = rte_lpm_lookup(lpm, rte_be_to_cpu_32(ipv4_hdr->dst_addr),
> > > +				    &next_hop);
> > > +		next_hop = (rc == 0) ? next_hop : drop_nh;
> > 
> > Maybe simple if here?  I see the same in other patches.
> 
> Will fix it in V5.

I now see it is better to leave this statement 
instead of putting an "if(unlikely(rc))" that might force an
explicit branch instruction. On other hand, conditional might
be making use of an "csel" instruction in arm64 or "cmov" in x86.
Are you ok with leaving this line as it is ? or you have a strong opinion
to change it to if ?


> > 
> > > +
> > > +		rte_node_mbuf_priv1(mbuf)->nh = (uint16_t)next_hop;
> > > +		next_hop = next_hop >> 16;
> > > +		next = (uint16_t)next_hop;
> > > +
> > > +		if (unlikely(next_index != next)) {
> > > +			/* Copy things successfully speculated till now */
> > > +			rte_memcpy(to_next, from, last_spec * sizeof(from[0]));
> > > +			from += last_spec;
> > > +			to_next += last_spec;
> > > +			held += last_spec;
> > > +			last_spec = 0;
> > > +
> > > +			rte_node_enqueue_x1(graph, node, next, from[0]);
> > > +			from += 1;
> > > +		} else {
> > > +			last_spec += 1;
> > > +		}
> > > +	}
> > > +
> > > +	/* !!! Home run !!! */
> > > +	if (likely(last_spec == nb_objs)) {
> > > +		rte_node_next_stream_move(graph, node, next_index);
> > > +		return nb_objs;
> > > +	}
> > > +	held += last_spec;
> > > +	rte_memcpy(to_next, from, last_spec * sizeof(from[0]));
> > > +	rte_node_next_stream_put(graph, node, next_index, held);
> > 
> > OK.  Forget my comments in different mail about difference between
> > encode/put - I got it now.
> > 
> > > +
> > > +	return nb_objs;
> > > +}
> > > +
> > [...]
> > 
> > With regards
> > Andrzej Ostruszka