Re: [dpdk-dev] [RFC PATCH 1/9] security: introduce CPU Crypto action type and API

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

Hi lads,
> >
> > You are right, the new API will process the crypto workload, no heavy enqueue
> > Dequeue operations required.
> >
> > Cryptodev tends to support multiple crypto devices, including HW and SW.
> > The 3-cache line access, iova address computation and assignment, simulation
> > of async enqueue/dequeue operations, allocate and free crypto ops, even the
> > mbuf linked-list for scatter-gather buffers are too heavy for SW crypto PMDs.
> 
> Why cant we have a cryptodev synchronous API which work on plain bufs as your suggested
> API and use the same crypto sym_session creation logic as it was before? It will perform
> same as it is doing in this series.

I tried to summarize our reasons in another mail in that thread.

> 
> >
> > To create this new synchronous API in cryptodev cannot avoid the problem
> > listed above:  first the API shall not serve only to part of the crypto (SW) PMDs -
> > as you know, it is Cryptodev. The users can expect some PMD only support part
> > of the overall algorithms, but not the workload processing API.
> 
> Why cant we have an optional data path in cryptodev for synchronous behavior if the
> underlying PMD support it. It depends on the PMD to decide whether it can have it supported or not.
> Only a feature flag will be needed to decide that.
> One more option could be a PMD API which the application can directly call if the
> mode is only supported in very few PMDs. This could be a backup if there is a
> requirement of deprecation notice etc.
> 
> >
> > Another reason is, there is assumption made, first when creating a crypto op
> > we have to allocate the memory to hold crypto op + sym op + iv, - we cannot
> > simply declare an array of crypto ops in the run-time and discard it when
> > processing
> > is done. Also we need to fill aad and digest HW address, which is not required for
> > SW at all.
> 
> We are defining a new API which may have its own parameters and requirements which
> Need to be fulfilled. In case it was a rte_security API, then also you are defining a new way
> Of packet execution and API params. So it would be same.
> You can reduce the cache line accesses as you need in the new API.
> The session logic need not be changed from crypto session to security session.
> Only the data patch need to be altered as per the new API.
> 
> >
> > Bottom line: using crypto op will still have 3 cache-line access performance
> > problem.
> >
> > So if we to create the new API in Cryptodev instead of rte_security, we need to
> > create new crypto op structure only for the SW PMDs, carefully document them
> > to not confuse with existing cryptodev APIs, make new device feature flags to
> > indicate the API is not supported by some PMDs, and again carefully document
> > them of these device feature flags.
> 
> The explanation of the new API will also happen in case it is a security API. Instead you need
> to add more explanation for session also which is already there in cryptodev.
> 
> >
> > So, to push these changes to rte_security instead the above problem can be
> > resolved,
> > and the performance improvement because of this change is big for smaller
> > packets
> > - I attached a performance test app in the patchset.
> 
> I believe there wont be any perf gap in case the optimized new cryptodev API is used.
> 
> >
> > For rte_security, we already have inline-crypto type that works quite close to the
> > this
> > new API, the only difference is that it is processed by the CPU cycles. As you may
> > have already seen the ipsec-library has wrapped these changes, and ipsec-secgw
> > has only minimum updates to adopt this change too. So to the end user, if they
> > use IPSec this patchset can seamlessly enabled with just commandline update
> > when
> > creating an SA.
> 
> In the IPSec application I do not see the changes wrt the new execution API.
> So the data path is not getting handled there. It looks incomplete. The user experience
> to use the new API will definitely be changed.

I believe we do support it for libtre_ipsec mode.
librte_ipsec hides all processing complexity inside and
does call rte_security_process_cpu_crypto_bulk() internally.
That's why for librte_ipsec it is literally 2 lines change:
--- a/examples/ipsec-secgw/ipsec_process.c
+++ b/examples/ipsec-secgw/ipsec_process.c
@@ -101,7 +101,8 @@  fill_ipsec_session(struct rte_ipsec_session *ss, struct ipsec_ctx *ctx,
 		}
 		ss->crypto.ses = sa->crypto_session;
 	/* setup session action type */
-	} else if (sa->type == RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL) {
+	} else if (sa->type == RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL ||
+			sa->type == RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO) {
 		if (sa->sec_session == NULL) {
 			rc = create_lookaside_session(ctx, sa);
 			if (rc != 0)
@@ -227,8 +228,8 @@  ipsec_process(struct ipsec_ctx *ctx, struct ipsec_traffic *trf)
 
 		/* process packets inline */
 		else if (sa->type == RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO ||
-				sa->type ==
-				RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL) {
+			sa->type == RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL ||
+			sa->type == RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO) {
 
 			satp = rte_ipsec_sa_type(ips->sa);