Re: [dpdk-dev] [EXT] RE: [PATCH v4 12/15] examples/ipsec-secgw: add app mode worker

[Akhil Goyal <akhil.goyal@nxp.com>]

Hi Lukasz,

> >
> > Is it not possible to use the existing functions for finding routes, checking
> packet types and checking security policies.
> > It will be very difficult to manage two separate functions for same work. I can
> see that the pkt->data_offs
> > Are not required to be updated in the inline case, but can we split the existing
> functions in two so that they can be
> > Called in the appropriate cases.
> >
> > As you have said in the cover note as well to add lookaside protocol support. I
> also tried adding it, and it will get very
> > Difficult to manage separate functions for separate code paths.
> >
> 
> [Lukasz] This was also Konstantin's comment during review of one of previous
> revisions.
> The prepare_one_packet() and prepare_tx_pkt() do much more than we need
> and for performance reasons
> we crafted new functions. For example, process_ipsec_get_pkt_type function
> returns nlp and whether
> packet type is plain or IPsec. That's all. Prepare_one_packet() process packets in
> chunks and does much more -
> it adjusts mbuf and packet length then it demultiplex packets into plain and IPsec
> flows and finally does
> inline checks. This is similar for update_mac_addrs() vs prepare_tx_pkt() and
> check_sp() vs inbound_sp_sa()
> that prepare_tx_pkt() and inbound_sp_sa() do more that we need in event mode.
> 
> I understand your concern from the perspective of code maintenance but on the
> other hand we are concerned with performance.
> The current code is not optimized to support multiple mode processing
> introduced with rte_security. We can work on a common
> routines once we have other modes also added, so that we can come up with a
> better solution than what we have today.
> 

Yes that is correct, but we should split the existing functions so that the part which is common
In both mode should stay common and we do not have duplicate code in the app.

I believe we should take care of this when we add lookaside cases. We shall remove all duplicate
Code. Ideally it should be part of this patchset. But we can postpone it to the lookaside case addition.


> 
> >> +	return 1;
> >
> > It will be better to use some MACROS while returning
> > Like
> > #define PKT_FORWARD   1
> > #define PKT_DROPPED     0
> > #define PKT_POSTED       2  /*may be for lookaside cases */
> >

I think you missed this comment.

> >> +
> >> +drop_pkt_and_exit:
> >> +	RTE_LOG(ERR, IPSEC, "Outbound packet dropped\n");
> >> +	rte_pktmbuf_free(pkt);
> >> +	ev->mbuf = NULL;
> >> +	return 0;
> >> +}
> >> +