[dpdk-dev] [PATCH v3 0/3] add fallback session

Anoob Joseph anoobj at marvell.com
Sun Oct 13 14:47:52 CEST 2019


Hi Konstantin,

Thanks for clarifying your stand on review comments.

From the comments we(Marvell/Cavium) have received on multiple features, my impression is that the feature need to be correct/fully done/acceptable to the overall community. Features are usually agreed upon if there is solid plan on how it will work in the long run.

Discussions with Radu on what eventually became get_userdata()
http://mails.dpdk.org/archives/dev/2017-November/082039.html

From the mail: 

"> Having something like "get_pkt_metadata" should be fine for inline 
> protocol usage. But we will still need a "get cookie" call to get the 
> cookie, as the application would need something it can interpret.
Why can't you have a get_pkt_metadata that returns the "cookie" - which 
I would call it userdata or similar? What I'm trying to say is that we 
should try to keep it as generic as possible. For example, I wouldn't 
assume that the cookie is stored in pkt->udata64 in the application."

Marvell prefers using pkt->udata64 instead of get_userdata() as it avoids an extra cache line access while trying to get hold of the SA which processed the packet. We agreed to Radu's suggestion even though Intel/Radu didn't have access to an inline protocol enabled h/w and it wasn't affecting any data path that Intel/Radu would be interested in.

Discussions with Mattias on eventmode helper
http://mails.dpdk.org/archives/dev/2019-June/134900.html

"Or even worse, the 
application's developers are forced to do a big-bang switch over to 
using the event and ethernet device APIs directly, in case they can't 
patch DPDK to work around the 
eventmode-assumption-that-didn't-hold-for-them.

You could always add flexibility to the framework (as you encounter a 
need for it), but then it will grow in complexity as well."

Here Mattias resisted the idea of eventmode helper for three reasons,

1. It's purely software, ie, no backing PMD required (which is the same case for librte_ipsec as well).
2. It "could" break for a new feature that "could" get added in eventdev.
3. He "thought" it would grow in complexity.

Marvell/Cavium has respected his opinion and did rework the entire approach which exponentially increased the effort required from our end (one code base integrated into 3 different apps vs 3 different code bases for 3 apps). Please do note that this entire "l2fwd-event" had to be redone 3 times while trying to get a consensus on the approach. I believe you had okeyed the first approach, so you cannot say we had to redo the patches because the patches were done poorly or the feature was incorrect.

I see that there is a shift in the review process these days and the vendors are encouraged to commend only on the features they are interested in. In that case, Marvell will no longer bother with patches which doesn't come in modes which doesn't matter to us. I would expect the same courtesy returned back as well.

FYI, eventmode ipsec-secgw is submitted as an RFC. In line with the comment redressal here, I've documented all the limitations.

> 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) {

+			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.ananyev at intel.com>
> Sent: Thursday, October 10, 2019 4:25 PM
> To: Anoob Joseph <anoobj at marvell.com>; Smoczynski, MarcinX
> <marcinx.smoczynski at intel.com>; akhil.goyal at nxp.com
> Cc: dev at dpdk.org; Jerin Jacob Kollanukkaran <jerinj at marvell.com>; Narayana
> Prasad Raju Athreya <pathreya at marvell.com>; Archana Muniganti
> <marchana at 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-bounces at dpdk.org> On Behalf Of Ananyev, Konstantin
> > > Sent: Thursday, October 3, 2019 8:16 PM
> > > To: Anoob Joseph <anoobj at marvell.com>; Smoczynski, MarcinX
> > > <marcinx.smoczynski at intel.com>; akhil.goyal at nxp.com
> > > Cc: dev at dpdk.org; Jerin Jacob Kollanukkaran <jerinj at marvell.com>;
> > > Narayana Prasad Raju Athreya <pathreya at marvell.com>; Archana
> > > Muniganti <marchana at 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.
> > > > >
> > > > >



More information about the dev mailing list