DPDK patches and discussions
 help / color / Atom feed
From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: 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: Wed, 16 Oct 2019 22:07:13 +0000
Message-ID: <2601191342CEEE43887BDE71AB97725801A8C69C5E@IRSMSX104.ger.corp.intel.com> (raw)
In-Reply-To: <VE1PR04MB66390B766A0DC7CF586DB40CE6970@VE1PR04MB6639.eurprd04.prod.outlook.com>


Hi Akhil,

> > > 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.
     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). 
	 

  parent 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 [this message]
2019-10-17 12:49                                             ` Ananyev, Konstantin
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=2601191342CEEE43887BDE71AB97725801A8C69C5E@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