Re: [dpdk-dev] [RFC v2 5/9] ipsec: add SA data-path API

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

-----Original Message-----
> Date: Sun, 21 Oct 2018 22:01:48 +0000
> From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: "dev@dpdk.org" <dev@dpdk.org>, "Awal, Mohammad Abdul"
>  <mohammad.abdul.awal@intel.com>, "Joseph, Anoob"
>  <Anoob.Joseph@cavium.com>, "Athreya, Narayana Prasad"
>  <NarayanaPrasad.Athreya@cavium.com>
> Subject: RE: [dpdk-dev] [RFC v2 5/9] ipsec: add SA data-path API
> 
> 
> Hi Jerin,

Hi Konstantin,

> 
> >
> > > +
> > > +/**
> > > + * IPsec session specific functions that will be used to:
> > > + * - prepare - for input mbufs and given IPsec session prepare crypto ops
> > > + *   that can be enqueued into the cryptodev associated with given session
> > > + *   (see *rte_ipsec_crypto_prepare* below for more details).
> > > + * - process - finalize processing of packets after crypto-dev finished
> > > + *   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.
> 
> 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

> 
> >
> > > +};
> > > +
> > > +/**
> > > + * 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.
> > > + */
> > > +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.
> 
> Good point, will add.

OK

> 
> >
> > > +
> > > +/**
> > > + * 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 can 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* structures
> > > + *   which contain the input packets.
> > > + * @param cop
> > > + *   The address of an array of *num* pointers to the output *rte_crypto_op*
> > > + *   structures.
> > > + * @param num
> > > + *   The maximum number of packets to process.
> > > + * @return
> > > + *   Number of successfully processed packets, with error code set in 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);
> > }
> 
> To fulfill that, we can either have 2 separate function pointers:
> uint16_t (*pkt_process)( const struct rte_ipsec_session *ss, struct rte_mbuf *mb[],uint16_t num);
> uint16_t (*event_process)( const struct rte_ipsec_session *ss, struct rte_event *ev[],uint16_t num);
> 
> Or we can keep one function pointer, but change it to accept just array of pointers:
> uint16_t (*process)( const struct rte_ipsec_session *ss, void *in[],uint16_t num);
> and then make session_prepare() to choose a proper function based on input.
> 
> 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_mbuf *mb[],uint16_t num);
	uint16_t (*event_process)( const struct rte_ipsec_session *ss, struct rte_event *ev[],uint16_t num);
};

> 
> >
> > 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 of this library
> > can be used with event-based model.
> 
> 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

> 
> >
> > 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_ipsec/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[] = {
> > > +               [RTE_SECURITY_ACTION_TYPE_NONE] = {
> > > +                       .prepare = lksd_none_prepare,
> > > +                       .process = lksd_none_process,
> > > +               },
> > > +               [RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO] = {
> > > +                       .prepare = NULL,
> > > +                       .process = inline_crypto_process,
> > > +               },
> > > +               [RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL] = {
> > > +                       .prepare = NULL,
> > > +                       .process = inline_proto_process,
> > > +               },
> > > +               [RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL] = {
> > > +                       .prepare = lksd_proto_prepare,
> > > +                       .process = lksd_proto_process,
> > > +               },
> >
> >              [RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL][EVENT] = {
> >                     .prepare = NULL,
> >                     .process = NULL,
> >                     .process_evt = lksd_event_process,
> >              },
> >              [RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL][EVENT] = {
> >                     .prepare = NULL,
> >                     .process = NULL,
> >                     .process_evt = inline_event_process,
> >              },
> >
> > Probably add one more dimension in array to choose event/poll?
> 
> That's a static function/array, surely we can have here as many dimensions as we need to.
> As I said below, will probably need to select a function based on direction, mode, etc. anyway.
> NP to have extra logic to select event/mbuf based functions.

OK

> 
> >
> >
> > > +       };
> > > +
> > > +       if (ss->type >= 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.
> 
> 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 related data.

OK

> 
> Konstantin
>