-----Original Message-----
> Date: Sun, 21 Oct 2018 22:01:48 +0000
> From: "Ananyev, Konstantin" <konstantin.anan...@intel.com>
> To: Jerin Jacob <jerin.ja...@caviumnetworks.com>
> CC: "dev@dpdk.org" <dev@dpdk.org>, "Awal, Mohammad Abdul"
> <mohammad.abdul.a...@intel.com>, "Joseph, Anoob"
> <anoob.jos...@cavium.com>, "Athreya, Narayana Prasad"
> <narayanaprasad.athr...@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
>
