From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jerin Jacob Subject: Re: [RFC v2 5/9] ipsec: add SA data-path API Date: Wed, 24 Oct 2018 12:03:58 +0000 Message-ID: <20181024120346.GA15208@jerin> References: <1535129598-27301-1-git-send-email-konstantin.ananyev@intel.com> <1539109420-13412-6-git-send-email-konstantin.ananyev@intel.com> <20181018173745.GA14157@jerin> <2601191342CEEE43887BDE71AB9772580102FEA53E@IRSMSX106.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" , "Awal, Mohammad Abdul" , "Joseph, Anoob" , "Athreya, Narayana Prasad" To: "Ananyev, Konstantin" Return-path: Received: from NAM02-BL2-obe.outbound.protection.outlook.com (mail-bl2nam02on0080.outbound.protection.outlook.com [104.47.38.80]) by dpdk.org (Postfix) with ESMTP id 40CC01B1A0 for ; Wed, 24 Oct 2018 14:04:00 +0200 (CEST) In-Reply-To: <2601191342CEEE43887BDE71AB9772580102FEA53E@IRSMSX106.ger.corp.intel.com> Content-Language: en-US Content-ID: <55B2E0106887B54597708BDCF1AF13B6@namprd07.prod.outlook.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" -----Original Message----- > Date: Sun, 21 Oct 2018 22:01:48 +0000 > From: "Ananyev, Konstantin" > To: Jerin Jacob > CC: "dev@dpdk.org" , "Awal, Mohammad Abdul" > , "Joseph, Anoob" > , "Athreya, Narayana Prasad" > > Subject: RE: [dpdk-dev] [RFC v2 5/9] ipsec: add SA data-path API >=20 >=20 > Hi Jerin, Hi Konstantin, >=20 > > > > > + > > > +/** > > > + * IPsec session specific functions that will be used to: > > > + * - prepare - for input mbufs and given IPsec session prepare crypt= o ops > > > + * that can be enqueued into the cryptodev associated with given s= ession > > > + * (see *rte_ipsec_crypto_prepare* below for more details). > > > + * - process - finalize processing of packets after crypto-dev finis= hed > > > + * with them or process packets that are subjects to inline IPsec = offload > > > + * (see rte_ipsec_process for more details). > > > + */ > > > +struct rte_ipsec_sa_func { > > > + uint16_t (*prepare)(const struct rte_ipsec_session *ss, > > > + struct rte_mbuf *mb[], > > > + struct rte_crypto_op *cop[], > > > + uint16_t num); > > > + uint16_t (*process)(const struct rte_ipsec_session *ss, > > > + struct rte_mbuf *mb[], > > > + uint16_t num); > > > > IMO, It makes sense to have separate function pointers for inbound and > > outbound so that, implementation would be clean and we can avoid a > > "if" check. >=20 > SA object by itself is unidirectional (either inbound or outbound), so > I don't think we need 2 function pointers here. > Yes, right now, inside ipsec lib we select functions by action_type only, > but it doesn't have to stay that way. > It could be action_type, direction, mode (tunnel/transport), event/poll, = etc. > Let say inline_proto_process() could be split into: > inline_proto_outb_process() and inline_proto_inb_process() and > rte_ipsec_sa_func.process will point to appropriate one. > I probably will change things that way for next version. OK >=20 > > > > > +}; > > > + > > > +/** > > > + * rte_ipsec_session is an aggregate structure that defines particul= ar > > > + * IPsec Security Association IPsec (SA) on given security/crypto de= vice: > > > + * - 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 pack= ets. > > > + */ > > > +struct rte_ipsec_session { > > > + > > > + /** > > > + * SA that session belongs to. > > > + * Note that multiple sessions can belong to the same SA. > > > + */ > > > + struct rte_ipsec_sa *sa; > > > + /** session action type */ > > > + enum rte_security_session_action_type type; > > > + /** session and related data */ > > > + union { > > > + struct { > > > + struct rte_cryptodev_sym_session *ses; > > > + } crypto; > > > + struct { > > > + struct rte_security_session *ses; > > > + struct rte_security_ctx *ctx; > > > + uint32_t ol_flags; > > > + } security; > > > + }; > > > + /** functions to prepare/process IPsec packets */ > > > + struct rte_ipsec_sa_func func; > > > +}; > > > > IMO, it can be cache aligned as it is used in fast path. >=20 > Good point, will add. OK >=20 > > > > > + > > > +/** > > > + * Checks that inside given rte_ipsec_session crypto/security fields > > > + * are filled correctly and setups function pointers based on these = values. > > > + * @param ss > > > + * Pointer to the *rte_ipsec_session* object > > > + * @return > > > + * - Zero if operation completed successfully. > > > + * - -EINVAL if the parameters are invalid. > > > + */ > > > +int __rte_experimental > > > +rte_ipsec_session_prepare(struct rte_ipsec_session *ss); > > > + > > > +/** > > > + * For input mbufs and given IPsec session prepare crypto ops that c= an be > > > + * enqueued into the cryptodev associated with given session. > > > + * expects that for each input packet: > > > + * - l2_len, l3_len are setup correctly > > > + * Note that erroneous mbufs are not freed by the function, > > > + * but are placed beyond last valid mbuf in the *mb* array. > > > + * It is a user responsibility to handle them further. > > > + * @param ss > > > + * Pointer to the *rte_ipsec_session* object the packets belong to= . > > > + * @param mb > > > + * The address of an array of *num* pointers to *rte_mbuf* structu= res > > > + * which contain the input packets. > > > + * @param cop > > > + * The address of an array of *num* pointers to the output *rte_cr= ypto_op* > > > + * structures. > > > + * @param num > > > + * The maximum number of packets to process. > > > + * @return > > > + * Number of successfully processed packets, with error code set i= n rte_errno. > > > + */ > > > +static inline uint16_t __rte_experimental > > > +rte_ipsec_crypto_prepare(const struct rte_ipsec_session *ss, > > > + struct rte_mbuf *mb[], struct rte_crypto_op *cop[], uint16_t = num) > > > +{ > > > + return ss->func.prepare(ss, mb, cop, num); > > > +} > > > + > > static inline uint16_t __rte_experimental > > rte_ipsec_event_process(const struct rte_ipsec_session *ss, struct rte_= event *ev[], uint16_t num) > > { > > return ss->func.event_process(ss, ev, num); > > } >=20 > To fulfill that, we can either have 2 separate function pointers: > uint16_t (*pkt_process)( const struct rte_ipsec_session *ss, struct rte_m= buf *mb[],uint16_t num); > uint16_t (*event_process)( const struct rte_ipsec_session *ss, struct rte= _event *ev[],uint16_t num); >=20 > Or we can keep one function pointer, but change it to accept just array o= f pointers: > uint16_t (*process)( const struct rte_ipsec_session *ss, void *in[],uint1= 6_t num); > and then make session_prepare() to choose a proper function based on inpu= t. >=20 > I am ok with both schemes, but second one seems a bit nicer to me. How about best of both worlds, i.e save space and enable compile check by anonymous union of both functions RTE_STD_C11 union { uint16_t (*pkt_process)( const struct rte_ipsec_session *ss,struct rte_mbu= f *mb[],uint16_t num); uint16_t (*event_process)( const struct rte_ipsec_session *ss, struct rte_= event *ev[],uint16_t num); }; >=20 > > > > This is to, > > 1) Avoid Event mode application code duplication > > 2) Better I$ utilization rather moving event specific and mbuff > > specific at different code locations > > 3) Better performance as inside one function pointer we can do things > > in one shot rather splitting the work to application and library. > > 4) Event mode has different modes like ATQ, non ATQ etc, These things > > we can abstract through exiting function pointer scheme. > > 5) atomicity & ordering problems can be sorted out internally with the = events, > > having one function pointer for event would be enough. > > > > We will need some event related info (event dev, port, atomic queue to > > use etc) which need to be added in rte_ipsec_session *ss as UNION so it > > wont impact the normal mode. This way, all the required functionality o= f this library > > can be used with event-based model. >=20 > Yes, to support event model, I imagine ipsec_session might need to > contain some event specific data. > I am absolutely ok with that idea in general. > Related fields can be added to the ipsec_session structure as needed, > together with actual event mode implementation. OK >=20 > > > > See below some implementation thoughts on this. > > > > > + > > > +#ifdef __cplusplus > > > +} > > > +#endif > > > + > > > +#endif /* _RTE_IPSEC_H_ */ > > > diff --git a/lib/librte_ipsec/rte_ipsec_version.map b/lib/librte_ipse= c/rte_ipsec_version.map > > > +const struct rte_ipsec_sa_func * > > > +ipsec_sa_func_select(const struct rte_ipsec_session *ss) > > > +{ > > > + static const struct rte_ipsec_sa_func tfunc[] =3D { > > > + [RTE_SECURITY_ACTION_TYPE_NONE] =3D { > > > + .prepare =3D lksd_none_prepare, > > > + .process =3D lksd_none_process, > > > + }, > > > + [RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO] =3D { > > > + .prepare =3D NULL, > > > + .process =3D inline_crypto_process, > > > + }, > > > + [RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL] =3D { > > > + .prepare =3D NULL, > > > + .process =3D inline_proto_process, > > > + }, > > > + [RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL] =3D { > > > + .prepare =3D lksd_proto_prepare, > > > + .process =3D lksd_proto_process, > > > + }, > > > > [RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL][EVENT] =3D { > > .prepare =3D NULL, > > .process =3D NULL, > > .process_evt =3D lksd_event_process, > > }, > > [RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL][EVENT] =3D { > > .prepare =3D NULL, > > .process =3D NULL, > > .process_evt =3D inline_event_process, > > }, > > > > Probably add one more dimension in array to choose event/poll? >=20 > That's a static function/array, surely we can have here as many dimension= s as we need to. > As I said below, will probably need to select a function based on directi= on, mode, etc. anyway. > NP to have extra logic to select event/mbuf based functions. OK >=20 > > > > > > > + }; > > > + > > > + if (ss->type >=3D RTE_DIM(tfunc)) > > > + return NULL; > > > +int __rte_experimental > > > +rte_ipsec_session_prepare(struct rte_ipsec_session *ss) > > > +{ > > > > Probably add one more argument to choose event vs poll so that > > above function pointers can be selected. > > > > or have different API like rte_ipsec_use_mode(EVENT) or API > > other slow path scheme to select the mode. >=20 > Yes, we would need something here. > I think we can have some field inside ipsec_session that defines > which input types (mbuf/event) session expects. > I suppose we would need such field anyway - as you said above, > ipsec_session most likely will contain a union for event/non-event relate= d data. OK >=20 > Konstantin >=20