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

From: Akhil Goyal <akhil.goyal@nxp.com>
To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
	"Zhang, Roy Fan" <roy.fan.zhang@intel.com>,
	"'dev@dpdk.org'" <dev@dpdk.org>,
	"De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>,
	'Thomas Monjalon' <thomas@monjalon.net>,
	"Doherty, Declan" <declan.doherty@intel.com>
Cc: 'Anoob Joseph' <anoobj@marvell.com>,
	Jerin Jacob <jerinj@marvell.com>,
	Hemant Agrawal <hemant.agrawal@nxp.com>
Subject: Re: [dpdk-dev] [RFC PATCH 1/9] security: introduce CPU Crypto action type and API
Date: Tue, 15 Oct 2019 15:02:20 +0000	[thread overview]
Message-ID: <VE1PR04MB66396F3B5B31080ABFC2F126E6930@VE1PR04MB6639.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <2601191342CEEE43887BDE71AB97725801A8C67CF4@IRSMSX104.ger.corp.intel.com>

> 
> 
> > Hi Akhil,
> >
> > Thanks for the review and comments!
> > Knowing you are extremely busy. Here is my point in brief:
> > I think placing the CPU synchronous crypto in the rte_security make sense, as
> >
> > 1. rte_security contains inline crypto and lookaside crypto action type already,
> adding cpu_crypto action type is reasonable.
> > 2. rte_security contains the security features may not supported by all devices,
> such as crypto, ipsec, and PDCP. cpu_crypto follow this
> > category, again crypto.
> > 3. placing CPU synchronous crypto API in rte_security is natural - as inline
> mode works synchronously, too. However cryptodev doesn't.
> > 4. placing CPU synchronous crypto API in rte_security helps boosting SW
> crypto performance, I have already provided a simple perf test
> > inside the unit test in the patchset for the user to try out - just comparing its
> output against DPDK crypto perf app output.
> > 5. placing CPU synchronous crypto API in cryptodev will never serve HW
> lookaside crypto PMDs, as making them to work synchronously
> > have huge performance penalty. However Cryptodev framework's existing
> design is providing APIs that will work in all crypto PMDs
> > (rte_cryptodev_enqueue_burst / dequeue_burst for example), this does not fit
> in cryptodev's principle.
> > 6. placing CPU synchronous crypto API in cryptodev confuses the user, as:
> > 	- the session created for async mode may not work in sync mode
> > 	- both enqueue/dequeue and cpu_crypto_process does the same crypto
> processing, but one PMD may support only one API (set),
> > the other may support another, and the third PMD supports both. We have to
> provide another API to let the user query which one to
> > support which.
> > 	- two completely different code paths for async/sync mode.
> > 7. You said in the end of the email - placing CPU synchronous crypto API into
> rte_security is not acceptable as it does not do any
> > rte_security stuff - crypto isn't? You may call this a quibble, but in my idea, in
> the patchset both PMDs' implementations did offload the work
> > to the CPU's special circuit designed dedicated to accelerate the crypto
> processing.
> >
> > To me cryptodev is the one CPU synchronous crypto API should not go into,
> rte_security is.
> 
> I also don't understand why rte_security is not an option here.
> We do have inline-crypto right now, why we can't have cpu-crypto with new
> process() API here?
> Actually would like to hear more opinions from the community here -
> what other interested parties think is the best way for introducing cpu-crypto
> specific API?

I have raised this concern in the weekly status meeting as well. But it looks like nobody
is interested.