Hi Akhil, Please see my answer below.
Thanks, Lukasz On 26.02.2020 07:04, Akhil Goyal wrote: > 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. > [Lukasz] Thank you for pointing out that I missed the comment. I will use macros when returning instead of magic numbers. >>>> + >>>> +drop_pkt_and_exit: >>>> + RTE_LOG(ERR, IPSEC, "Outbound packet dropped\n"); >>>> + rte_pktmbuf_free(pkt); >>>> + ev->mbuf = NULL; >>>> + return 0; >>>> +} >>>> +
