> > Once again - you keep making statements without any evidence.
> > Provide something more concrete here - description of packet and code flow
> > (function names, exact lines of code) that you think will cause a problem.
>
> I saw atleast the ones below. It would be frivolous to assume that the
> feature is added without any data path checks.
>
> + if (MBUF_NO_SEC_OFFLOAD(pkt) && sa->fallback_sessions > 0) {
Ok here we have extra 2 comparsions for data (pkt->ol_flags and
sa->fallback_sessions),
that already would be in the cache and .
For common path both will be evaluated to 0, we didn't observe any perf
difference because of that.
I would expect the same for other archs.
If you would see something on your boxes - let us know.
We could think what we can do here...
Probably at least swap the comparisons or so...
But honestly, I don't expect any observable slowdown.
>
> + if (pg->id.val & IPSEC_SA_OFFLOAD_FALLBACK_FLAG) {
>
> @maintainers, Marvell is not blocking this feature anymore assuming that the
> same yardstick will be used while reviewing patches
> submitted by us.
>
> Thanks,
> Anoob
>
> > -----Original Message-----
> > From: Ananyev, Konstantin <konstantin.anan...@intel.com>
> > Sent: Thursday, October 10, 2019 4:25 PM
> > To: Anoob Joseph <ano...@marvell.com>; Smoczynski, MarcinX
> > <marcinx.smoczyn...@intel.com>; akhil.go...@nxp.com
> > Cc: dev@dpdk.org; Jerin Jacob Kollanukkaran <jer...@marvell.com>; Narayana
> > Prasad Raju Athreya <pathr...@marvell.com>; Archana Muniganti
> > <march...@marvell.com>
> > Subject: RE: [dpdk-dev] [PATCH v3 0/3] add fallback session
> >
> >
> > Hi Anoob,
> >
> > >
> > > Following are the main issues I had identified,
> > >
> > > 1. Doesn't work for inline protocol offload (the plan itself won't
> > > scale for that case) 2. Ignores the data from the h/w lookup 3. The
> > > fallback support is actually supported in ipsec-secgw (application)
> > > instead of
> > librte_ipsec.
> > > 4. Reordering issue.
> > > 5. Introduces checks in existing data path even when the feature is not
> > properly implemented/is not suitable for other PMDs.
> > >
> > > For Issue #1, I get that Intel doesn't have plans to support the feature
> > > and
> > won't be submitting anything in that direction.
> > > But adding code that further exacerbates the situation is probably not
> > > desired.
> >
> > We already discussed why that feature can't be fully supported right now by
> > inline-proto/lookaside-proto devices:
> > There is no API defined to sync session state between HW and SW
> > https://urldefense.proofpoint.com/v2/url?u=http-
> > 3A__mails.dpdk.org_archives_dev_2019-
> > 2DSeptember_143881.html&d=DwIGaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=jPfB8
> > rwwviRSxyLWs2n6B-
> > WYLn1v9SyTMrT5EQqh2TU&m=UGz2FemapWdsohgNRq2UX5ArFEL_pLGYEVrwe
> > IzaUX4&s=ObkJK9pyzOZuwoeulr6mrr6iKQF1NgXu3p2v7XwkYEs&e=
> >
> > I already outlined that gap nearly a year ago:
> > https://urldefense.proofpoint.com/v2/url?u=http-
> > 3A__mails.dpdk.org_archives_dev_2018-
> > 2DSeptember_113600.html&d=DwIGaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=jPfB8
> > rwwviRSxyLWs2n6B-
> > WYLn1v9SyTMrT5EQqh2TU&m=UGz2FemapWdsohgNRq2UX5ArFEL_pLGYEVrwe
> > IzaUX4&s=0x8pmfsS7nT7Xn-a_RvhI-DxtHMdk0zr4jiapi22tVE&e=
> > Though so far I didn't see any attempt to resolve this problem by Marvell
> > (or
> > NXP) engineers.
> > Which makes me think that either you guys don't consider it as a problem at
> > all,
> > or have no desire to work on it.
> > I never insisted, as I don't think it is my call by obvious reasons:
> > 1) right now Intel doesn't have HW that provides inline-proto/lksd-proto
> > capabilities.
> > 2) Intel DPDK team doesn't have access to such HW.
> > 3) There is no PMD inside dpdk.org that implements inline-proto anyway.
> >
> > Now, one year later we are in a situation where:
> > 1) there still no PMD at dpdk.org that supports inline-proto.
> > 2) API to sync session state between HW and SW still not
> > defined/implemented.
> > 3) you trying to accuse me and other Intel DPDK team for lack of interest to
> > work on that problem for you
> > and threat our patches because of that.
> >
> > That just doesn't look fair to me.
> > You shouldn't expect other people patches to provide 'complete' solution for
> > inline-proto, if (in a year time) you failed to define/implement an API for
> > that.
> >
> > > For Issue #2, since ipsec-secgw is not utilizing the h/w lookup data,
> > > the solution here follows suit and is preventing other usage models of the
> > hardware which are allowed by the library.
> >
> > Same as above, right now there is no PMD that supports inline-proto and no
> > API
> > to extract that extra H/W lookup data.
> > Though I replied to you and explained *twice*how I think such functionality
> > could be added if in hypothetical future there would be a PMD that will
> > support
> > these features.
> > But you simply ignored my replies and kept threating our patches.
> > Obviously, I don't think it is a proper attitude for open-source community.
> > At least I would expect to provide some clear explanation, why do you think
> > approach I outlined is not good enough.
> > Even better would be to:
> > 1) submit an RFC/patch with API you envision and PMD implementation for it.
> > 2) submit an RFC/patch with changes in ipsec-secgw to enable these new
> > features.
> > Without that in place there is not much to discuss.
> >
> > > For Issue #3, librte_ipsec is still experimental and is just 2-3
> > > releases old. It has scope for improvement and all these features would've
> > added value to its utility. No strong feelings here.
> > >
> > > For Issue #4, glad that you documented that issue.
> > >
> > > For Issue #5, in your reply you had mentioned that there is no
> > > additional check done. I see atleast few checks in the datapath. Since the
> > feature itself is experimental, it would be less contentious if it was
> > introduced as
> > a new data path.
> >
> > Once again - you keep making statements without any evidence.
> > Provide something more concrete here - description of packet and code flow
> > (function names, exact lines of code) that you think will cause a problem.
> >
> > >
> > > Also, to quote from the reply,
> > >
> > > > > [Anoob] Shouldn't we get that accepted first then?
> > > >
> > > > I don't think so.
> > > > Current implementation does provide expected functionality (with
> > > > known limitations).
> > > > In future, we can try to improve it and/or remove existing limitations.
> > > > That's a common iteration development approach that is used though
> > > > whole
> > > > DPDK:
> > > > - provide initial solution with basic functionality first
> > > > - improve/extend with future releases/patches
> > >
> > > If your suggestion is to stick to above guidelines,
> >
> > It is not my suggestion, that's how things work within DPDK right now.
> > Nearly every dpdk library, driver and sample app was developed based on that
> > principle.
> > Even ipsec-secgw itself, just look at the git log for it.
> >
> > > I don't think there is any point in pushing any further. I leave this
> > > to maintainers to decide. I would assume the same rules would apply to
> > > every
> > feature that is being attempted.
> >
> > I believe that for #1, #2, #5 your comments are biased, and doesn't provide
> > real
> > picture.
> > So my suggestion to maintainers would be to ignore them as not valuable.
> >
> > Konstantin
> >
> >
> > >
> > > Thanks,
> > > Anoob
> > >
> > > > -----Original Message-----
> > > > From: dev <dev-boun...@dpdk.org> On Behalf Of Ananyev, Konstantin
> > > > Sent: Thursday, October 3, 2019 8:16 PM
> > > > To: Anoob Joseph <ano...@marvell.com>; Smoczynski, MarcinX
> > > > <marcinx.smoczyn...@intel.com>; akhil.go...@nxp.com
> > > > Cc: dev@dpdk.org; Jerin Jacob Kollanukkaran <jer...@marvell.com>;
> > > > Narayana Prasad Raju Athreya <pathr...@marvell.com>; Archana
> > > > Muniganti <march...@marvell.com>
> > > > Subject: Re: [dpdk-dev] [PATCH v3 0/3] add fallback session
> > > >
> > > > Hi Anoob,
> > > >
> > > > > > > > > I've few more observations regarding the proposed feature.
> > > > > > > > >
> > > > > > > > > 1. From what I understood, if an ESP packet ends up on an
> > > > > > > > > unprotected interface and doesn't have 'PKT_RX_SEC_OFFLOAD'
> > > > > > > > > bit set, then the packet
> > > > > > > > would be looked up to see the associated SA and then
> > > > > > > > fallback session is figured out and then further processing is
> > > > > > > > done.
> > > > > > > > >
> > > > > > > > > Can you confirm if I understood the sequence correctly? If
> > > > > > > > > yes, then aren't we doing an extra lookup in the s/w? The
> > > > > > > > > packet may be looked by the h/w using rte_flow and that
> > > > > > > > > information could be used to determine the
> > > > > > > > SA. Also, if the ESP packet is expected to be forwarded,
> > > > > > > > then the above logic will add an unnecessary lookup even
> > > > > > > > after your h/w has detected that the packet need not be security
> > processed.
> > > > > > > >
> > > > > > > > Not sure I understood your whole statement above.
> > > > > > > > AFAIK, right now (with dpdk master) for unprotected iface it
> > > > > > > > works like
> > > > that:
> > > > > > > >
> > > > > > > > 1. slit incoming traffic into 3 groups: ESP packets, IPv4
> > > > > > > > packets, IPv6
> > > > > > packets.
> > > > > > > > For ESP packets:
> > > > > > > > 2. perform SAD lookup
> > > > > > > > a. if it fails (non SA found for that SPI), then drop the
> > > > > > > > packet.
> > > > > > > > b. otherwise (SA found) process the packet using found
> > > > > > > > SA
> > > > > > > >
> > > > > > > > What fall-back patch adds:
> > > > > > > > Before step 2.b check:
> > > > > > > > does that SA has its primary session with type
> > > > > > > > INLINE-CRYPTO and does HW fail to do IPsec realted processing
> > > > > > > > for it
> > (by some reason)?
> > > > > > > > If yes, then mark this packet to be processed by fall-back
> > > > > > > > session.
> > > > > > > > if (MBUF_NO_SEC_OFFLOAD(pkt) && sa->fallback_sessions > 0) {
> > > > > > > > uintptr_t intsa = (uintptr_t)sa;
> > > > > > > > intsa |= IPSEC_SA_OFFLOAD_FALLBACK_FLAG;
> > > > > > > > result_sa = (void *)intsa; }
> > > > > > > >
> > > > > > > > So from my perspective, no additional lookup where introduced.
> > > > > > >
> > > > > > > [Anoob] For inline processing, h/w does a lookup and figures
> > > > > > > out the security
> > > > > > processing to be done on the packet.
> > > > > > > "rte_security_get_userdata()" allows s/w to further segregate
> > > > > > > that info. In this approach, I believe we don't consider how
> > > > > > > such info from h/w can
> > > > > > be used.
> > > > > >
> > > > > > Right now it is not the case with ipsec-secgw:
> > > > >
> > > > > [Anoob] Are you saying there is no h/w lookup involved when doing
> > > > > inline crypto processing? Then I'm confused. My understanding is
> > > > > that rte_flow
> > > > with ACTION type as SECURITY is being used to setup h/w lookups.
> > > >
> > > > I am saying that current ipsec-secgw code for each inbound ESP
> > > > packet
> > > > *always* does SW lookup.
> > > > No matter did HW lookup take place already and does information
> > > > about that lookup is available via mbuf in some way or not.
> > > > Just look at the ipsec-secgw code yourself:
> > > >
> > > > nb_rx = rte_eth_rx_burst(portid, queueid, pkts, MAX_PKT_BURST);
> > > >
> > > > if (nb_rx > 0)
> > > > process_pkts(qconf, pkts, nb_rx, portid);
> > > >
> > > > Inside process_pkts()
> > > > https://urldefense.proofpoint.com/v2/url?u=http-
> > > > 3A__lxr.dpdk.org_dpdk_latest_ident_prepare-5Fone-
> > > >
> > 5Fpacket&d=DwIGaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=BPcGOOudUMrTDQ9Yb
> > > >
> > gKcOkO5ChYiUPPlPNIEvTOhjNE&m=KVBgvXYlUGyMmu260erISKr73Qt6PBLux0SI
> > > > oSO1i-M&s=opbGGHUqfxy8p_4NvwCG6od6VdV0qN8XafaPiubfO1A&e=
> > > > it first calls prepare_traffic() which does sort of de-muxing:
> > > > put packet in one of 3 arrays:
> > > > 1) ESP packets
> > > > 2) non ESP IPv4 packets
> > > > 3) non ESP IPv6 packets
> > > > Also it checks is PKT_RX_SEC_OFFLOAD set for that packet.
> > > > If yes, it retrieves associated security user-data and stores it
> > > > inside private mbuf area.
> > > >
> > > > Then it goes into process_pkts_inbound()
> > > > https://urldefense.proofpoint.com/v2/url?u=http-
> > > > 3A__lxr.dpdk.org_dpdk_latest_ident_process-5Fpkts-
> > > >
> > 5Finbound&d=DwIGaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=BPcGOOudUMrTDQ9Y
> > > >
> > bgKcOkO5ChYiUPPlPNIEvTOhjNE&m=KVBgvXYlUGyMmu260erISKr73Qt6PBLux0S
> > > > IoSO1i-M&s=2OxMXug7jtUQPcYiDN4nW9WbrMtRfTJx5k-N7ikOeME&e=
> > > > and here for all ESP packets:
> > > > legacy code path:
> > > > ipsec_inbound()->inbound_sa_lookup()->single_inbound_lookup()
> > > > for librte_ipsec code path:
> > > > inbound_sa_lookup()->single_inbound_lookup()
> > > >
> > > > single_inbound_lookup() is the one that does actual SW lookup for
> > > > each input packet.
> > > > https://urldefense.proofpoint.com/v2/url?u=http-
> > > > 3A__lxr.dpdk.org_dpdk_latest_ident_single-5Finbound-
> > > >
> > 5Flookup&d=DwIGaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=BPcGOOudUMrTDQ9Yb
> > > >
> > gKcOkO5ChYiUPPlPNIEvTOhjNE&m=KVBgvXYlUGyMmu260erISKr73Qt6PBLux0SI
> > > > oSO1i-M&s=D2ZPZPLbcQSbaluG8KH0F3uUDHfQWNYxw4MOc0fSkI8&e=
> > > >
> > > > >
> > > > > > for each inbound ESP packet it *always* does a lookup in SW
> > > > > > based SADB, and if lookup fails - drops the packet (see 2.a above).
> > > > > > So, I still not see any additional lookups introduced with the
> > > > > > patch.
> > > > >
> > > > > [Anoob] In case of inline crypto, you could do that. No disagreement.
> > > > > But that doesn't mean that is the only way. If PMDs can retrieve
> > > > > info about
> > > > h/w lookups and pass it on to the upper layers, isn't that the better
> > approach?
> > > >
> > > > Please let's keep our conversation in a constructive way.
> > > > We are not discussing what could be done in theory, but a particular
> > > > patch for
> > > > ipsec-secgw:
> > > > Right now ipsec-secgw does a SW lookup for each inbound ESP packet
> > > > (no matter what HW offload does) and this patch doesn't introduce
> > > > any extra SW lookups.
> > > >
> > > > If you are not happy with current ipsec-secgw approach, that's fine
> > > > - feel free to submit RFC/patch to fix that, or start a discussion in a
> > > > new
> > thread.
> > > > I just don't see why we have to discuss it in context of this patch.
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > > Also AFAIK, right now there is no possibility to configure
> > > > > > > > ipsec-secgw to BYPASS some ESP traffic.
> > > > > > > > Should we do it (to conform to ipsec RFC) - that's probably
> > > > > > > > subject of another discussion.
> > > > > > >
> > > > > > > [Anoob] The approach (choice of flags) forces a software-based
> > > > > > > SA lookup for packets that need to be bypassed instead of
> > > > > > > leveraging possible
> > > > > > hardware SA lookup. I don't think what ipsec-secgw does matters
> > > > > > here.
> > > > > >
> > > > > > I do not agree with that statement.
> > > > > > First of all - current patch doesn't introduce any additional SW
> > > > > > lookups, see above.
> > > > > > Second, if someone would like to add BYPASS for ESP packets into
> > > > > > ipsec-secgw, I think he would have to insert new code that do
> > > > > > de-mux
> > > > > > *before* any ESP related processing starts.
> > > > > > Most obvious variant: at prepare_one_packet() when we split our
> > > > > > incoming traffic to IPsec and non-IPsec ones.
> > > > > > Second variant - at early stage of single_inbound_lookup(),
> > > > > > straight after actual SAD lookup failure.
> > > > > > In both cases, I don't see how session selection (primary or
> > > > > > faillback) would affect us here, actual decision do we want to
> > > > > > PROCESS or BYPASS particular ESP packet needs to take place before
> > that.
> > > > > > So I still believe we are ok here.
> > > > >
> > > > > [Anoob] I don't think you understood what I'm trying to highlight
> > > > > here. You could have a situation when h/w can detect that the
> > > > > packet has to be "PROTECT"ed using an "inline crypto" session. But
> > > > > then it might detect that it cannot proceed with inline processing
> > > > > because of some
> > > > issue (fragmentation, h/w queue overflow etc). Now the h/w has
> > > > already figured out the action to be done to the packet. If PMDs
> > > > allow this to be communicated to the application, the application won't
> > > > be
> > required to do the lookup.
> > > > >
> > > > > In inline crypto, application can ignore the h/w lookup data and
> > > > > do a s/w lookup with the protocol headers as they are not stripped
> > > > > off. It was done this way, because API and the associated means to
> > > > > get this info from PMD was not introduced when inline crypto and
> > > > > corresponding
> > > > support in Intel's PMD was added. But inline protocol cannot do the
> > > > lookup in the application and so the concept of userdata etc was
> > > > introduced. The same can be applied to inline crypto also.
> > > > Advantage? It could remove the extra lookup done in s/w.
> > > > >
> > > > > To summarize, we cannot assume that every inline crypto packet
> > > > > need to looked up in the application to figure out the processing
> > > > > done on
> > the packet.
> > > > There can be applications which relies on h/w lookup data to figure
> > > > out the processing done.
> > > > >
> > > > > Example: Using rte_flow, I can create a rule for ESP packets to be
> > > > > MARKed. There is no security session created for the flow and the
> > > > > application intends to forward this flow packets to a different
> > > > > port. With your
> > > > approach, even these packets would be looked up. The packet will
> > > > have info from the h/w lookup which doesn't get used, because the
> > > > approach fails to introduce the required concepts.
> > > >
> > > > I think I understood your point.
> > > > Basically you saying that in future your PMD for unprocessed ESP
> > > > packets might provide some additional information (via
> > > > rte_security_get_userdata) that might be used by SW - let say to choose
> > BYPASS for such packets.
> > > > Current ipsec-secgw doesn't support such ability.
> > > > Your concern is that this patch will somehow prevent (or will make
> > > > it harder) for you to make your future changes.
> > > > Correct?
> > > > If, so then I said before, I don't think that concern is valid.
> > > > Most obvious variant where to add such code is inisde inside
> > > > prepare_one_packet() when we split our incoming traffic to IPsec and
> > > > non-IPsec ones.
> > > > To be more specific something like that
> > > >
> > > > if (eth->ether_type == rte_cpu_to_be_16(RTE_ETHER_TYPE_IPV4)) {
> > > >
> > > > iph4 = (const struct rte_ipv4_hdr *)rte_pktmbuf_adj(pkt,
> > > > RTE_ETHER_HDR_LEN);
> > > > adjust_ipv4_pktlen(pkt, iph4, 0);
> > > >
> > > > - if (iph4->next_proto_id == IPPROTO_ESP)
> > > > + if (iph4->next_proto_id == IPPROTO_ESP) {
> > > > + if (pkt->ol_flags & PKT_RX_SEC_OFFLOAD) {
> > > > + /* extract extra info, make decision based on
> > > > that */
> > > > + } else
> > > > t->ipsec.pkts[(t->ipsec.num)++] = pkt;
> > > > + }
> > > >
> > > > Of course some code reordering might be needed to avoid performance
> > > > drop, but I suppose the main idea is clear enough.
> > > > Second variant - at early stage of single_inbound_lookup(), either
> > > > before or straight after actual SAD SW lookup failure.
> > > > In both cases, this new code will be executed *before* session selection
> > code.
> > > >
> > > > >
> > > > > Does the above sound like BYPASS? Yes.
> > > > > Does ipsec-secgw do it this way? No.
> > > >
> > > > Correct, it doesn't.
> > > > You have to submit patches if you'd like to support such ability.
> > > >
> > > > > Does this approach prevent an application from introducing such a
> > BYPASS?
> > > > Yes.
> > > >
> > > > That's not correct, I believe, see above.
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > For example, ESN was not supported by ipsec-secgw (before
> > > > > > > library mode was introduced), but every single library change
> > > > > > > and application change was added keeping in mind that ESN is a
> > > > > > > valid feature for ipsec. So it is
> > > > > > one thing to say ipsec-secgw doesn't support ESP bypass and
> > > > > > saying the solution doesn't consider a possibility of ESP bypass.
> > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > 2. The solution proposed here seems like adding the
> > > > > > > > > handling in ipsec-secgw instead of ipsec library. In other
> > > > > > > > > words, this feature is not getting
> > > > > > > > added in ipsec library, which was supposed to simplify the
> > > > > > > > whole ipsec usage in DPDK, but fails to handle the case of
> > fragmentation.
> > > > > > > >
> > > > > > > > What we have right now with ipsec library is SA (low) level API.
> > > > > > > > It can handle multi-segment packets properly, but expects
> > > > > > > > someone else to do other steps (fragmentation/reassembly).
> > > > > > > > ipsec-secgw demonstrates how librte_ip_frag and librte_ipsec
> > > > > > > > can be used together to deal with fragmented IPsec traffic
> > > > > > > > in a proper
> > > > manner.
> > > > > > > > Probably in future we'll consider adding some high-level API
> > > > > > > > that will be able to do whole processing under hood (SPD/SAD
> > > > > > > > lookup, fragment/reassembly, actual IPsec packet processing,
> > > > > > > > matching inbound selectors, etc.), but right now it is not the
> > > > > > > > case.
> > > > > > > >
> > > > > > > > > Also, since the fallback feature is entirely done in the
> > > > > > > > > application, it begs the
> > > > > > > > question why the same feature is omitted for legacy use case.
> > > > > > > >
> > > > > > > > Because legacy mode doesn't support multi-seg packets at first
> > > > > > > > place.
> > > > > > > > Also it is an extra overhead to support 2 code-paths (legacy
> > > > > > > > and
> > > > > > > > library) for the same app, so we'd like in future to
> > > > > > > > deprecate and later remove legacy code- path.
> > > > > > > > As a first step we propose to make library code-path a default
> > > > > > > > one:
> > > > > > > > https://urldefense.proofpoint.com/v2/url?u=http-3A__patches.
> > > > > > > > dpdk
> > > > > > > > .org
> > > > > > > >
> > > > > >
> > > >
> > _cover_58247_&d=DwIFAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyL
> > > > > > Ws2
> > > > > > > > n6B-WYLn1v9SyTMrT5EQqh2TU&m=b3E429fuo8P-K5XfH8wG-
> > > > > > 7hwr1d8oM4uJGErAkbf
> > > > > > > > DvA&s=vz7ioUzJOuzoJdmV7QO0QLPKYn1ytFsj_0eYatbSCrg&e=
> > > > > > >
> > > > > > > [Anoob] Even if we need the application to do the reassembly,
> > > > > > > it would look simple if application uses the same
> > > > > > > "rte_ipsec_session"
> > > > > > > to process the
> > > > > > fallback packet.
> > > > > >
> > > > > > I think there is some sort of misunderstanding here.
> > > > > > With current librte_ipsec design:
> > > > > > rte_ipsec_sa - opaque SW representation of the SA (HW neutral)
> > > > > > rte_ipsec_session associates SA (rte_ipsec_sa)with particular HW
> > > > > > device (session).
> > > > > > Same SA can be referred by multiple sessions.
> > > > > >
> > > > > > From rte_ipsec.h:
> > > > > > /**
> > > > > > * rte_ipsec_session is an aggregate structure that defines
> > > > > > particular
> > > > > > * IPsec Security Association IPsec (SA) on given security/crypto
> > > > > > device:
> > > > > > * - pointer to the SA object
> > > > > > * - security session action type
> > > > > > * - pointer to security/crypto session, plus other related data
> > > > > > * - session/device specific functions to prepare/process IPsec
> > > > > > packets.
> > > > > > */
> > > > > >
> > > > > > > In that case, the processing required to handle the other
> > > > > > > packet will be hidden from the application. Here application
> > > > > > > has to make sure it chooses
> > > > > > the correct session, which I believe should be moved to ipsec
> > > > > > library.
> > > > > >
> > > > > > From my side it is a good thing to let application define its own
> > > > > > usage
> > model.
> > > > > > I.E. librte_ipsec supports multiple-sessions per SA, upper-layer
> > > > > > (app) decides how it will use that feature.
> > > > > > Though if you strongly feel that some higher-level API is needed
> > > > > > here, and have some good ideas about it - please go ahead and
> > > > > > submit RFC
> > > > for it.
> > > > >
> > > > > [Anoob] If you are okay with application defining how it
> > > > > implements ipsec,
> > > > then probably there is no use case for lib ipsec?
> > > >
> > > > :)
> > > > Once again - library provides an implementation.
> > > > Application defines the way to use it, i.e. how it will apply
> > > > functionality library provides for different use-case scenarios.
> > > > As an example: snpritnf() provides user with ability to format
> > > > strings, application decides what exactly format to use and for which
> > > > string.
> > > > Same for librte_ipsec, library provides functionality to perform
> > > > processing for ESP packets.
> > > > Application defines which packets to process and what session to use.
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > 3. It seems like ordering won't be maintained once this
> > > > > > > > > processing is done. Again, this is the sequence I understood.
> > > > > > > > > Please correct me if I missed
> > > > > > > > something,
> > > > > > > > > a. Application receives a bunch of packets (let's
> > > > > > > > > say 6 packets), in which few are fragmented (P3 & P4) and
> > > > > > > > > the rest can be inline
> > > > > > > > processed.
> > > > > > > > > b. Application receives P1->P2->P3->P4->P5->P6 (in
> > > > > > > > > this, P1, P2, P5, P6 are
> > > > > > > > inline processed successfully) and P4 & P5 are the fragments
> > > > > > > > > c. Application groups packets. P1->P2->P5->P6
> > > > > > > > >becomes one group and P3-
> > > > > > > > >P4 becomes another and goes for fallback processing.
> > > > > > > > > Now how is ordering maintained? I couldn't figure out how
> > > > > > > > >that is done in this
> > > > > > > > case.
> > > > > > > >
> > > > > > > > You right, fallback session can introduce packet reordering.
> > > > > > > > At least till we'll have ability to process packets in sync
> > > > > > > > mode too.
> > > > > > > > See our presentation at dpdk userspace (slides 17, 18):
> > > > > > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__static.
> > > > > > > > sche
> > > > > > > > d.co
> > > > > > > > m_hosted-5Ffiles_dpdkbordeaux2019_8f_DPDK-
> > > > > > 2DIPSECu9.pdf&d=DwIFAg&c=n
> > > > > > > > KjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyLWs2n6B-
> > > > > > WYLn1v9SyTMrT5EQqh2TU&
> > > > > > > > m=b3E429fuo8P-K5XfH8wG-
> > > > > > 7hwr1d8oM4uJGErAkbfDvA&s=MLRnnYcykjnsqXrHGUuR
> > > > > > > > YHc5uDxaAod0Yl7f06EQr1M&e= Right now the only way to deal
> > > > > > > > with it - have replay window big enough to sustain
> > > > > > > > reordering and async processing latency.
> > > > > > > > That's actually another reason why we add this feature into
> > > > > > > > ipsec-secgw sample
> > > > > > > > app:
> > > > > > > > so people can evaluate it on their platforms, determine what
> > > > > > > > replay window size would be needed, what issues/slowdowns it
> > > > > > > > might
> > > > cause, etc.
> > > > > > >
> > > > > > > [Anoob] This is something I had noticed while going through the
> > > > > > > code
> > flow.
> > > > > > The ordering info is lost because of the approach.
> > > > > >
> > > > > > Once again, it is a known limitation and we are not trying to
> > > > > > hide it from you :)
> > > > >
> > > > > [Anoob] Never said it was hidden. Initially I had okayed the
> > > > > approach even though it wasn't a solution suitable for inline
> > > > > protocol. But as more cases
> > > > were considered, few shortcomings were observed, and I was skeptical
> > > > about how any of that would be solved in the long run.
> > > > >
> > > > > My suggestion, if these limitations and the plans to address it
> > > > > were mentioned
> > > > in the cover letter, we could've saved few cycles here.
> > > >
> > > > Point taken.
> > > >
> > > > > But my reply would still be the same 😊
> > > > >
> > > > > > It
> > > > > > was outlined here:
> > > > > > https://urldefense.proofpoint.com/v2/url?u=https-
> > > > > > 3A__static.sched.com_hosted-5Ffiles_dpdkbordeaux2019_8f_DPDK-
> > > > > >
> > > >
> > 2DIPSECu9.pdf&d=DwIFAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyL
> > > > > > Ws2n6B-WYLn1v9SyTMrT5EQqh2TU&m=b3E429fuo8P-K5XfH8wG-
> > > > > >
> > > >
> > 7hwr1d8oM4uJGErAkbfDvA&s=MLRnnYcykjnsqXrHGUuRYHc5uDxaAod0Yl7f06E
> > > > > > Qr1M&e=
> > > > > > And in the latest patch version Marcin clearly stated it inside the
> > > > > > AG
> > session:
> > > > > > https://urldefense.proofpoint.com/v2/url?u=http-
> > > > > >
> > > >
> > 3A__patches.dpdk.org_patch_60039_&d=DwIFAg&c=nKjWec2b6R0mOyPaz7xtf
> > > > > > Q&r=jPfB8rwwviRSxyLWs2n6B-
> > > > WYLn1v9SyTMrT5EQqh2TU&m=b3E429fuo8P-
> > > > > > K5XfH8wG-
> > > > > >
> > > >
> > 7hwr1d8oM4uJGErAkbfDvA&s=zrn91Vjf_ZElAlDf7IeZGmGevcA6RMwpPTLUCGlgZ
> > > > > > cY&e=
> > > > > > If you think even further clarification within the doc is
> > > > > > needed, please let us know.
> > > > > >
> > > > > > About the actual packet re-oridering:
> > > > > > AFAIK, for some use-cases it might be acceptable, for others not.
> > > > > > Though it is an optional feature, that wouldn't be enabled by
> > > > > > default, so user can always choose is it does he needs/wants
> > > > > > this feature or
> > > > not.
> > > > > > If/when we'll have CPU_CRYPTO processing mode:
> > > > >
> > > > > [Anoob] Shouldn't we get that accepted first then?
> > > >
> > > > I don't think so.
> > > > Current implementation does provide expected functionality (with
> > > > known limitations).
> > > > In future, we can try to improve it and/or remove existing limitations.
> > > > That's a common iteration development approach that is used though
> > > > whole
> > > > DPDK:
> > > > - provide initial solution with basic functionality first
> > > > - improve/extend with future releases/patches
> > > >
> > > > >
> > > > > Also, the ordering is lost because application is not considering
> > > > > that. Why can't
> > > > we use re-ordering library? Or an event-dev?
> > > >
> > > > No-one saying it can't be improved in one way or another.
> > > > We don't plan to introduce such code right now due to its complexity.
> > > > Might be something in future.
> > > > Though you are welcome to go ahead and submit your own patches to
> > > > improve that solution.
> > > >
> > > > >
> > > > > > https://urldefense.proofpoint.com/v2/url?u=http-
> > > > > >
> > > >
> > 3A__patches.dpdk.org_cover_58862_&d=DwIFAg&c=nKjWec2b6R0mOyPaz7xtf
> > > > > > Q&r=jPfB8rwwviRSxyLWs2n6B-
> > > > WYLn1v9SyTMrT5EQqh2TU&m=b3E429fuo8P-
> > > > > > K5XfH8wG-
> > > > > >
> > > >
> > 7hwr1d8oM4uJGErAkbfDvA&s=k1zef_1uqELXblKN3_qjX04WXNFGAUEFIIJrvKPr6
> > > > > > eA&e=
> > > > > > we'll add ability for the user to avoid this reordering problem
> > > > > >
> > > > > > > As we dig deeper, the feature looks hardly complete. The
> > > > > > > concerning part is the approach doesn't seem conducive to
> > > > > > > fixing any of these issues in the
> > > > > > future.
> > > > > >
> > > > > > Again, don't agree with that statement.
> > > > > > From my perspective: while current implementation has its
> > > > > > limitations (packet reordering, inline-proto/lksd-proto), it is
> > > > > > totally functional and provides users with a reference how to
> > > > > > deal with such
> > > > kind of problems.
> > > > >
> > > > > [Anoob] Well, that is exactly my problem. It gives a skewed
> > > > > reference on how to deal with the problem, without considering other
> > possibilities.
> > > >
> > > > Once again, please feel free and come-up with your own patches, that
> > > > will address this problem in a smarter way.
> > > >
> > > > > If every application starts adopting this method and starts doing
> > > > > lookup for every ESP packet, h/w based lookups (and the associated
> > > > > h/w) would be rendered useless. I really think this series, which
> > > > > happens to be
> > > > the first step in solving a complex problem, will be very useful if
> > > > all these issues are addressed properly.
> > > > >
> > > > > If you can introduce the inline+lookaside via librte_ipsec, its
> > > > > perhaps ok for the patch to have some limitations(like lack of
> > > > > ordering,
> > > > assumptions on ESP etc). However, if the patch is introduced
> > > > directly into ipsec- secgw, it needs to be more comprehensive and
> > > > robust.
> > > >
> > > > I think common DPDK policy is exactly opposite:
> > > > library/driver has to be as robust and comprehensive as possible.
> > > > sample app code allowed to have some limitations (as long as they
> > > > are documented, etc.).
> > > >
> > > > > I also expect performance impact due to these changes
> > > >
> > > > Could you be more specific here?
> > > > What performance impact do you expect and why?
> > > > Can you point to the exact piece of patch code that you believe
> > > > would cause a degradation.
> > > > I don't expect any and performance tests on our platforms didn't
> > > > show any regression so far.
> > > >
> > > > > and so I prefer if the inline+lookaside can be made separate
> > > > > datapath rather than overloading the existing one.
> > > >
> > > > I don't see any good reason for that.
> > > > This feature is optional.
> > > > User have to enable it manually for each session (via config file).
> > > > for all other sessions there is no impact.
> > > > What we probably can do - add extra check inside sa.c to allow
> > > > fallback sessions only for inline-crypto-offload type of sessions.
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > Also, if the new ipsec related features are introduced via
> > > > > > > ipsec-secgw than via rte_ipsec, then it raises doubts over the
> > > > > > > utility of
> > > > rte_ipsec library.
> > > > > >
> > > > > >
