DPDK patches and discussions
 help / color / Atom feed
From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
	Akhil Goyal <akhil.goyal@nxp.com>,
	"'dev@dpdk.org'" <dev@dpdk.org>,
	"De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>,
	'Thomas Monjalon' <thomas@monjalon.net>,
	"Zhang, Roy Fan" <roy.fan.zhang@intel.com>,
	"Doherty, Declan" <declan.doherty@intel.com>
Cc: 'Anoob Joseph' <anoobj@marvell.com>
Subject: Re: [dpdk-dev] [RFC PATCH 1/9] security: introduce CPU Crypto action type and API
Date: Thu, 17 Oct 2019 12:49:20 +0000
Message-ID: <2601191342CEEE43887BDE71AB97725801A8C6A3D0@IRSMSX104.ger.corp.intel.com> (raw)
In-Reply-To: <2601191342CEEE43887BDE71AB97725801A8C69C5E@IRSMSX104.ger.corp.intel.com>


> 
> > > > User can use the same session, that is what I am also insisting, but it may have
> > > separate
> > > > Session private data. Cryptodev session create API provide that functionality
> > > and we can
> > > > Leverage that.
> > >
> > > rte_cryptodev_sym_session. sess_data[] is indexed by driver_id, which means
> > > we can't use
> > > the same rte_cryptodev_sym_session to hold sessions for both sync and async
> > > mode
> > > for the same device. Off course we can add a hard requirement that any driver
> > > that wants to
> > > support process() has to create sessions that can handle both  process and
> > > enqueue/dequeue,
> > > but then again  what for to create such overhead?
> > >
> > > BTW, to be honest, I don't consider current rte_cryptodev_sym_session
> > > construct for multiple device_ids:
> > > __extension__ struct {
> > >                 void *data;
> > >                 uint16_t refcnt;
> > >         } sess_data[0];
> > >         /**< Driver specific session material, variable size */
> > >
> > Yes I also feel the same. I was also not in favor of this when it was introduced.
> > Please go ahead and remove this. I have no issues with that.
> 
> If you are not happy with that structure, and admit there are issues with it,
> why do you push for reusing it for cpu-crypto API?
> Why  not to take step back, take into account current drawbacks
> and define something that (hopefully) would suite us better?
> Again new API will be experimental for some time, so we'll
> have some opportunity to see does it works and if not fix it.
> 
> About removing data[] from existing rte_cryptodev_sym_session -
> Personally would like to do that, but the change seems to be too massive.
> Definitely not ready for such effort right now.
> 
> >
> > > as an advantage.
> > > It looks too error prone for me:
> > > 1. Simultaneous session initialization/de-initialization for devices with the same
> > > driver_id is not possible.
> > > 2. It assumes that all device driver will be loaded before we start to create
> > > session pools.
> > >
> > > Right now it seems ok, as no-one requires such functionality, but I don't know
> > > how it will be in future.
> > > For me rte_security session model, where for each security context user have to
> > > create new session
> > > looks much more robust.
> > Agreed
> >
> > >
> > > >
> > > > BTW, I can see a v2 to this RFC which is still based on security library.
> > >
> > > Yes, v2 was concentrated on fixing found issues, some code restructuring,
> > > i.e. - changes that would be needed anyway whatever API aproach we'll choose.
> > >
> > > > When do you plan
> > > > To submit the patches for crypto based APIs. We have RC1 merge deadline for
> > > this
> > > > patchset on 21st Oct.
> > >
> > > We'd like to start working on it ASAP, but it seems we still have a major
> > > disagreement
> > > about how this crypto-dev API should look like.
> > > Which makes me think - should we return to our original proposal via
> > > rte_security?
> > > It still looks to me like clean and straightforward way to enable this new API,
> > > and probably wouldn't cause that much controversy.
> > > What do you think?
> >
> > I cannot spend more time discussing on this until RC1 date. I have some other stuff pending.
> > You can send the patches early next week with the approach that I mentioned or else we
> > can discuss this post RC1(which would mean deferring to 20.02).
> >
> > But moving back to security is not acceptable to me. The code should be put where it is
> > intended and not where it is easy to put. You are not doing any rte_security stuff.
> >
> 
> Ok, then my suggestion:
> Let's at least write down all points about crypto-dev approach where we
> disagree and then probably try to resolve them one by one....
> If we fail to make an agreement/progress in next week or so,
> (and no more reviews from the community)
> will have bring that subject to TB meeting to decide.
> Sounds fair to you?
> 
> List is below.
> Please add/correct me, if I missed something.
> 
> Konstantin
> 
> 1. extra input parameters to create/init rte_(cpu)_sym_session.
> 
> Will leverage existing 6B gap inside rte_crypto_*_xform between 'algo' and 'key' fields.
> New fields will be optional and would be used by PMD only when cpu-crypto session is requested.
> For lksd-crypto session PMD is free to ignore these fields.
> No ABI breakage is required.
> 
> Hopefully no controversy here with #1.
> 
> 2. cpu-crypto create/init.
>     a) Our suggestion - introduce new API for that:
>         - rte_crypto_cpu_sym_init() that would init completely opaque  rte_crypto_cpu_sym_session.
>         - struct rte_crypto_cpu_sym_session_ops {(*process)(...); (*clear); /*whatever else we'll need *'};
>         - rte_crypto_cpu_sym_get_ops(const struct rte_crypto_sym_xform *xforms)
>           that would return const struct rte_crypto_cpu_sym_session_ops *based on input xforms.
> 	Advantages:
> 	1)  totally opaque data structure (no ABI breakages in future), PMD writer is totally free
> 	     with it format and contents.
> 	2) each session entity is self-contained, user doesn't need to bring along dev_id etc.
> 	    dev_id is needed  only at init stage, after that user will use session ops to perform
> 	    all operations on that session (process(), clear(), etc.).
> 	3) User can decide does he wants to store ops[] pointer on a per session basis,
> 	    or on a per group of same sessions, or...
> 	4) No mandatory mempools for private sessions. User can allocate memory for cpu-crypto
> 	    session whenever he likes.
> 	Disadvantages:
> 	5) Extra changes in control path
> 	6) User has to store session_ops pointer explicitly.

After another thought if 2.a.6 is really that big deal we can have small shim layer on top:

rte_crypto_cpu_sym_session { void *ses; struct rte_crypto_cpu_sym_session_ops * const ops; }
OR even
rte_crypto_cpu_sym_session { void *ses; struct rte_crypto_cpu_sym_session_ops ops; }

And merge rte_crypto_cpu_sym_init() and rte_crypto_cpu_sym_get_ops() into one (init).

Then process() can become a wrapper:
rte_crypto_cpu_sym_process(ses, ...) {return ses->ops->process(ses->ses, ...);}
OR
rte_crypto_cpu_sym_process(ses, ...) {return ses->ops.process(ses->ses, ...);}

if that would help to reach consensus - works for me. 

>      b) Your suggestion - reuse existing rte_cryptodev_sym_session_init() and existing rte_cryptodev_sym_session
>       structure.
> 	Advantages:
> 	1) allows to reuse same struct and init/create/clear() functions.
> 	    Probably less changes in control path.
> 	Disadvantages:
> 	2) rte_cryptodev_sym_session. sess_data[] is indexed by driver_id, which means that
> 	    we can't use the same rte_cryptodev_sym_session to hold private sessions pointers
> 	    for both sync and async mode  for the same device.
>                    So wthe only option we have - make PMD devops->sym_session_configure()
> 	    always create a session that can work in both cpu and lksd modes.
> 	    For some implementations that would probably mean that under the hood  PMD would create
> 	    2 different session structs (sync/async) and then use one or another depending on from what API been called.
> 	    Seems doable, but ...:
>                    - will contradict with statement from 1:
> 	      " New fields will be optional and would be used by PMD only when cpu-crypto session is requested."
>                       Now it becomes mandatory for all apps to specify cpu-crypto related parameters too,
> 	       even if they don't plan to use that mode - i.e. behavior change, existing app change.
>                      - might cause extra space overhead.
> 	3) not possible to store device (not driver) specific data within the session, but I think it is not really needed right now.
> 	    So probably minor compared to 2.b.2.
> 
> Actually #3 follows from #2, but decided to have them separated.
> 
> 3. process() parameters/behavior
>     a) Our suggestion: user stores ptr to session ops (or to (*process) itself) and just does:
>         session_ops->process(sess, ...);
> 	Advantages:
> 	1) fastest possible execution path
> 	2) no need to carry on dev_id for data-path
> 	Disadvantages:
> 	3) user has to carry on session_ops pointer explicitly
>     b) Your suggestion: add  (*cpu_process) inside rte_cryptodev_ops and then:
>         rte_crypto_cpu_sym_process(uint8_t dev_id, rte_cryptodev_sym_session  *sess, /*data parameters*/) {...
>                      rte_cryptodevs[dev_id].dev_ops->cpu_process(ses, ...);
>                       /*and then inside PMD specifc process: */
>                      pmd_private_session = sess->sess_data[this_pmd_driver_id].data;
>                      /* and then most likely either */
>                      pmd_private_session->process(pmd_private_session, ...);
>                      /* or jump based on session/input data */
> 	Advantages:
> 	1) don't see any...
> 	Disadvantages:
> 	2) User has to carry on dev_id inside data-path
> 	3) Extra level of indirection (plus data dependency) - both for data and instructions.
> 	    Possible slowdown compared to a) (not measured).
> 

  reply index

Thread overview: 87+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-03 15:40 [dpdk-dev] [RFC PATCH 0/9] security: add software synchronous crypto process Fan Zhang
2019-09-03 15:40 ` [dpdk-dev] [RFC PATCH 1/9] security: introduce CPU Crypto action type and API Fan Zhang
2019-09-04 10:32   ` Akhil Goyal
2019-09-04 13:06     ` Zhang, Roy Fan
2019-09-06  9:01       ` Akhil Goyal
2019-09-06 13:12         ` Zhang, Roy Fan
2019-09-10 11:25           ` Akhil Goyal
2019-09-11 13:01             ` Ananyev, Konstantin
2019-09-06 13:27         ` Ananyev, Konstantin
2019-09-10 10:44           ` Akhil Goyal
2019-09-11 12:29             ` Ananyev, Konstantin
2019-09-12 14:12               ` Akhil Goyal
2019-09-16 14:53                 ` Ananyev, Konstantin
2019-09-16 15:08                   ` Ananyev, Konstantin
2019-09-17  6:02                   ` Akhil Goyal
2019-09-18  7:44                     ` Ananyev, Konstantin
2019-09-25 18:24                       ` Ananyev, Konstantin
2019-09-27  9:26                         ` Akhil Goyal
2019-09-30 12:22                           ` Ananyev, Konstantin
2019-09-30 13:43                             ` Akhil Goyal
2019-10-01 14:49                               ` Ananyev, Konstantin
2019-10-03 13:24                                 ` Akhil Goyal
2019-10-07 12:53                                   ` Ananyev, Konstantin
2019-10-09  7:20                                     ` Akhil Goyal
2019-10-09 13:43                                       ` Ananyev, Konstantin
2019-10-11 13:23                                         ` Akhil Goyal
2019-10-13 23:07                                           ` Zhang, Roy Fan
2019-10-14 11:10                                             ` Ananyev, Konstantin
2019-10-15 15:02                                               ` Akhil Goyal
2019-10-16 13:04                                                 ` Ananyev, Konstantin
2019-10-15 15:00                                             ` Akhil Goyal
2019-10-16 22:07                                           ` Ananyev, Konstantin
2019-10-17 12:49                                             ` Ananyev, Konstantin [this message]
2019-10-18 13:17                                             ` Akhil Goyal
2019-10-21 13:47                                               ` Ananyev, Konstantin
2019-10-22 13:31                                                 ` Akhil Goyal
2019-10-22 17:44                                                   ` Ananyev, Konstantin
2019-10-22 22:21                                                     ` Ananyev, Konstantin
2019-10-23 10:05                                                     ` Akhil Goyal
2019-10-30 14:23                                                       ` Ananyev, Konstantin
2019-11-01 13:53                                                         ` Akhil Goyal
2019-09-03 15:40 ` [dpdk-dev] [RFC PATCH 2/9] crypto/aesni_gcm: add rte_security handler Fan Zhang
2019-09-03 15:40 ` [dpdk-dev] [RFC PATCH 3/9] app/test: add security cpu crypto autotest Fan Zhang
2019-09-03 15:40 ` [dpdk-dev] [RFC PATCH 4/9] app/test: add security cpu crypto perftest Fan Zhang
2019-09-03 15:40 ` [dpdk-dev] [RFC PATCH 5/9] crypto/aesni_mb: add rte_security handler Fan Zhang
2019-09-03 15:40 ` [dpdk-dev] [RFC PATCH 6/9] app/test: add aesni_mb security cpu crypto autotest Fan Zhang
2019-09-03 15:40 ` [dpdk-dev] [RFC PATCH 7/9] app/test: add aesni_mb security cpu crypto perftest Fan Zhang
2019-09-03 15:40 ` [dpdk-dev] [RFC PATCH 8/9] ipsec: add rte_security cpu_crypto action support Fan Zhang
2019-09-03 15:40 ` [dpdk-dev] [RFC PATCH 9/9] examples/ipsec-secgw: add security " Fan Zhang
2019-09-06 13:13 ` [dpdk-dev] [PATCH 00/10] security: add software synchronous crypto process Fan Zhang
2019-09-06 13:13   ` [dpdk-dev] [PATCH 01/10] security: introduce CPU Crypto action type and API Fan Zhang
2019-09-18 12:45     ` Ananyev, Konstantin
2019-09-29  6:00     ` Hemant Agrawal
2019-09-29 16:59       ` Ananyev, Konstantin
2019-09-30  9:43         ` Hemant Agrawal
2019-10-01 15:27           ` Ananyev, Konstantin
2019-10-02  2:47             ` Hemant Agrawal
2019-09-06 13:13   ` [dpdk-dev] [PATCH 02/10] crypto/aesni_gcm: add rte_security handler Fan Zhang
2019-09-18 10:24     ` Ananyev, Konstantin
2019-09-06 13:13   ` [dpdk-dev] [PATCH 03/10] app/test: add security cpu crypto autotest Fan Zhang
2019-09-06 13:13   ` [dpdk-dev] [PATCH 04/10] app/test: add security cpu crypto perftest Fan Zhang
2019-09-06 13:13   ` [dpdk-dev] [PATCH 05/10] crypto/aesni_mb: add rte_security handler Fan Zhang
2019-09-18 15:20     ` Ananyev, Konstantin
2019-09-06 13:13   ` [dpdk-dev] [PATCH 06/10] app/test: add aesni_mb security cpu crypto autotest Fan Zhang
2019-09-06 13:13   ` [dpdk-dev] [PATCH 07/10] app/test: add aesni_mb security cpu crypto perftest Fan Zhang
2019-09-06 13:13   ` [dpdk-dev] [PATCH 08/10] ipsec: add rte_security cpu_crypto action support Fan Zhang
2019-09-26 23:20     ` Ananyev, Konstantin
2019-09-27 10:38     ` Ananyev, Konstantin
2019-09-06 13:13   ` [dpdk-dev] [PATCH 09/10] examples/ipsec-secgw: add security " Fan Zhang
2019-09-06 13:13   ` [dpdk-dev] [PATCH 10/10] doc: update security cpu process description Fan Zhang
2019-09-09 12:43   ` [dpdk-dev] [PATCH 00/10] security: add software synchronous crypto process Aaron Conole
2019-10-07 16:28   ` [dpdk-dev] [PATCH v2 " Fan Zhang
2019-10-07 16:28     ` [dpdk-dev] [PATCH v2 01/10] security: introduce CPU Crypto action type and API Fan Zhang
2019-10-08 13:42       ` Ananyev, Konstantin
2019-10-07 16:28     ` [dpdk-dev] [PATCH v2 02/10] crypto/aesni_gcm: add rte_security handler Fan Zhang
2019-10-08 13:44       ` Ananyev, Konstantin
2019-10-07 16:28     ` [dpdk-dev] [PATCH v2 03/10] app/test: add security cpu crypto autotest Fan Zhang
2019-10-07 16:28     ` [dpdk-dev] [PATCH v2 04/10] app/test: add security cpu crypto perftest Fan Zhang
2019-10-07 16:28     ` [dpdk-dev] [PATCH v2 05/10] crypto/aesni_mb: add rte_security handler Fan Zhang
2019-10-08 16:23       ` Ananyev, Konstantin
2019-10-09  8:29       ` Ananyev, Konstantin
2019-10-07 16:28     ` [dpdk-dev] [PATCH v2 06/10] app/test: add aesni_mb security cpu crypto autotest Fan Zhang
2019-10-07 16:28     ` [dpdk-dev] [PATCH v2 07/10] app/test: add aesni_mb security cpu crypto perftest Fan Zhang
2019-10-07 16:28     ` [dpdk-dev] [PATCH v2 08/10] ipsec: add rte_security cpu_crypto action support Fan Zhang
2019-10-08 23:28       ` Ananyev, Konstantin
2019-10-07 16:28     ` [dpdk-dev] [PATCH v2 09/10] examples/ipsec-secgw: add security " Fan Zhang
2019-10-07 16:28     ` [dpdk-dev] [PATCH v2 10/10] doc: update security cpu process description Fan Zhang

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2601191342CEEE43887BDE71AB97725801A8C6A3D0@IRSMSX104.ger.corp.intel.com \
    --to=konstantin.ananyev@intel.com \
    --cc=akhil.goyal@nxp.com \
    --cc=anoobj@marvell.com \
    --cc=declan.doherty@intel.com \
    --cc=dev@dpdk.org \
    --cc=pablo.de.lara.guarch@intel.com \
    --cc=roy.fan.zhang@intel.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox