[dpdk-dev] [PATCH v5 00/10] examples/ipsec-secgw: make app to use ipsec library

[Ananyev, Konstantin]

> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal at nxp.com]
> Sent: Wednesday, January 2, 2019 2:29 PM
> To: Ananyev, Konstantin <konstantin.ananyev at intel.com>; dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 00/10] examples/ipsec-secgw: make app to use ipsec library
> 
> 
> 
> On 1/2/2019 6:31 PM, Ananyev, Konstantin wrote:
> > Hi Akhil,
> >
> >> Hi Konstantin,
> >>
> >> I just got results on running the ipsec-secgw on NXP hardware.
> > Thanks for doing that.
> > We don't have NXP HW, so would need more help from you.
> >
> >> with -l option, I got a seg fault while running traffic. gdb suggest
> >> that pkt_func is not filled up and is NULL.
> >> #1  0x00000000004689bc in rte_ipsec_pkt_crypto_prepare (ss=0x17ad82d80,
> >> mb=0xffffffffe498, cop=0xffffffffdfc0, num=1)
> >>       at
> >> /home/akhil/netperf/dpdk_up/dpdk-next-crypto/arm64-dpaa-linuxapp-gcc/include/rte_ipsec.h:115
> >> (gdb)  p /x *ss
> >> $1 = {sa = 0x17ad7ea40, type = 0x3, {crypto = {ses = 0x165a4e900},
> >> security = {ses = 0x165a4e900, ctx = 0x0, ol_flags = 0x0}}, pkt_func = {
> >>       prepare = 0x0, process = 0x0}}
> >>
> > I guess I understand the reason:
> > right now rte_ipsec_session_prepare() expects that
> > for all modes except RTE_SECURITY_ACTION_TYPE_NONE
> > security.ctx to be not NULL.
> > Which as I understand is not necessary for lookaside-proto.
> > Could you try the fix below?
> > If it would work as expected, I'll include these changes into v6?
> It did not crash this time with the below fix but the performance is
> very very poor(~95% drop) if I use -l option with lookaside mode.
> I have not analyzed the issue yet. Will be doing it on Friday.

Did you run it with your previous change applied?
With drain_crypto_queues() moved under 'if (unlikely(diff_tsc > drain_tsc))'
condition?
If so, that could be the reason for such slowdown, see my other mail
regarding it.
Can you try to revert it (yes it would mean 5% drop for legacy mode)
and  try again ((yes it would mean 5% drop for legacy mode,
but we can deal with it later)?

Konstantin

> >
> > ---
> >   examples/ipsec-secgw/ipsec_process.c | 24 ++++++++++++++++++++----
> >   lib/librte_ipsec/ses.c               | 11 +++++++++--
> >   2 files changed, 29 insertions(+), 6 deletions(-)
> >
> > diff --git a/examples/ipsec-secgw/ipsec_process.c b/examples/ipsec-secgw/ipsec_process.c
> > index 7ab378f6a..e403c461a 100644
> > --- a/examples/ipsec-secgw/ipsec_process.c
> > +++ b/examples/ipsec-secgw/ipsec_process.c
> > @@ -87,19 +87,36 @@ enqueue_cop_bulk(struct cdev_qp *cqp, struct rte_crypto_op *cop[], uint32_t num)
> >   }
> >
> >   static inline int
> > -fill_ipsec_session(struct rte_ipsec_session *ss, const struct ipsec_sa *sa)
> > +fill_ipsec_session(struct rte_ipsec_session *ss, struct ipsec_ctx *ctx,
> > +	struct ipsec_sa *sa)
> >   {
> > +	int32_t rc;
> > +
> >   	/* setup crypto section */
> >   	if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE) {
> > +		if (sa->crypto_session == NULL) {
> > +			rc = create_session(ctx, sa);
> > +			if (rc != 0)
> > +				return rc;
> > +		}
> >   		ss->crypto.ses = sa->crypto_session;
> >   	/* setup session action type */
> >   	} else {
> > +		if (sa->sec_session == NULL) {
> > +			rc = create_session(ctx, sa);
> > +			if (rc != 0)
> > +				return rc;
> > +		}
> >   		ss->security.ses = sa->sec_session;
> >   		ss->security.ctx = sa->security_ctx;
> >   		ss->security.ol_flags = sa->ol_flags;
> >   	}
> >
> > -	return rte_ipsec_session_prepare(ss);
> > +	rc = rte_ipsec_session_prepare(ss);
> > +	if (rc != 0)
> > +		memset(ss, 0, sizeof(*ss));
> > +
> > +	return rc;
> >   }
> >
> >   /*
> > @@ -209,8 +226,7 @@ ipsec_process(struct ipsec_ctx *ctx, struct ipsec_traffic *trf)
> >
> >   		/* no valid HW session for that SA, try to create one */
> >   		if (ips->crypto.ses == NULL &&
> > -				(create_session(ctx, sa) != 0 ||
> > -				fill_ipsec_session(ips, sa) != 0))
> > +				fill_ipsec_session(ips, ctx, sa) != 0)
> >   			k = 0;
> >
> >   		/* process packets inline */
> > diff --git a/lib/librte_ipsec/ses.c b/lib/librte_ipsec/ses.c
> > index 562c1423e..11580970e 100644
> > --- a/lib/librte_ipsec/ses.c
> > +++ b/lib/librte_ipsec/ses.c
> > @@ -14,8 +14,15 @@ session_check(struct rte_ipsec_session *ss)
> >   	if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE) {
> >   		if (ss->crypto.ses == NULL)
> >   			return -EINVAL;
> > -	} else if (ss->security.ses == NULL || ss->security.ctx == NULL)
> > -		return -EINVAL;
> > +	} else {
> > +		if (ss->security.ses == NULL)
> > +			return -EINVAL;
> > +		if ((ss->type == RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO ||
> > +				ss->type ==
> > +				RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL) &&
> > +				ss->security.ctx == NULL)
> > +			return -EINVAL;
> > +	}
> >
> >   	return 0;
> >   }