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