Hi Konstantin,
Added my comments inline with your draft.
>
>
> Hi Akhil,
>
> > > 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.
[Akhil] This structure is serving some use case which is agreed upon in the
Community, we cannot just remove a feature altogether. Rather it is Intel's
Use case only.
>
> 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.
>
[snip]..
>
> 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?
Agreed
>
> List is below.
> Please add/correct me, if I missed something.
>
> Konstantin
Before going into comparison, we should define the requirement as well.
What I understood from the patchset,
"You need a synchronous API to perform crypto operations on raw data using SW
PMDs"
So,
- no crypto-ops,
- no separate enq-deq, only single process API for data path
- Do not need any value addition to the session parameters.
(You would need some parameters from the crypto-op which
Are constant per session and since you wont use crypto-op,
You need some place to store that)
Now as per your mail, the comparison
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.
[Akhil] Agreed, no issues.
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.
[Akhil] It will have breakage at some point till we don't hit the union size.
Rather I don't suspect there will be more parameters added.
Or do we really care about the ABI breakage when the argument is about
the correct place to add a piece of code or do we really agree to add code
anywhere just to avoid that breakage.
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.).
[Akhil] There is nothing called as session ops in current DPDK. What you are
proposing
is a new concept which doesn't have any extra benefit, rather it is adding
complexity
to have two different code paths for session create.
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...
[Akhil] Will the user really care which process API should be called from the
PMD.
Rather it should be driver's responsibility to store that in the session
private data
which would be opaque to the user. As per my suggestion same process function
can
be added to multiple sessions or a single session can be managed inside the PMD.
4) No mandatory mempools for private sessions. User can allocate memory
for cpu-crypto
session whenever he likes.
[Akhil] you mean session private data? You would need that memory anyways, user
will be
allocating that already. You do not need to manage that.
Disadvantages:
5) Extra changes in control path
6) User has to store session_ops pointer explicitly.
[Akhil] More disadvantages:
- All supporting PMDs will need to maintain TWO types of session for the
same crypto processing. Suppose a fix or a new feature(or algo) is added, PMD
owner
will need to add code in both the session create APIs. Hence more maintenance
and
error prone.
- Stacks which will be using these new APIs also need to maintain two
code path for the same processing while doing session initialization
for sync and async
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 the 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.
[Akhil] It will not contradict with #1, you will only have few checks in the
session init PMD
Which support this mode, find appropriate values and set the appropriate
process() in it.
User should be able to call, legacy enq-deq as well as the new process()
without any issue.
User would be at runtime will be able to change the datapath.
So this is not a disadvantage, it would be additional flexibility for the user.
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.
[Akhil] So lets omit this for current discussion. And I hope we can find some
way to deal with it.
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
[Akhil] I don't see any overhead of carrying dev id, at least it would be
inline with the
current DPDK methodology.
What you are suggesting is a new way to get the things done without much
benefit.
Also I don't see any performance difference as crypto workload is heavier than
Code cycles, so that wont matter.
So IMO, there is no advantage in your suggestion as well.
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).
Having said all this, if the disagreements cannot be resolved, you can go for a
pmd API specific
to your PMDs, because as per my understanding the solution doesn't look
scalable to other PMDs.
Your approach is aligned only to Intel, will not benefit others like openssl
which is used by all
vendors.
Regards,
Akhil
