Hi Anoob
Seems your reply is not in plaintext which will make it difficult for inline
response. So, please take care of that when you reply.
Rest, please see my response inline.
From: Anoob Joseph
Sent: Thursday, July 25, 2019 9:46 AM
To: Ayuj Verma <ayve...@marvell.com>; akhil.go...@nxp.com
Cc: arkadiuszx.kusz...@intel.com; Shally Verma <shal...@marvell.com>; Sunila
Sahu <ss...@marvell.com>; Kanaka Durga Kotamarthy <kkotamar...@marvell.com>;
dev@dpdk.org; Fiona Trahe <fiona.tr...@intel.com>
Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable
Hi Ayuj,
I believe there are couple of issues with this patch.
Are these experimental APIs? I believe they were made stable this release and
I'm not sure if it is a right practice to edit an API without deprecation
notice after it is made stable. Especially now that RC2 is done. @Akhil, what
is your take on this?
[Shally] These are experimental still, hence no deprecation notice. We checked
about it with Fiona, Akhil before.
I think, the approach here is wrong. If the lifetime of the session is expected
to be only few packets, then session-less (which I believe is in the pipeline)
would make more sense.
[Shally] See my response further below on this.
If the lifetime of the session is expected to be more than that, then having
this feature/limitation would make application more complicated. Also, since
one asymmetric session can hold both public & private keys, the implicit
assumption would be, the session can be used for multiple kinds of operations.
This change is in contradiction with that.
[Shally] Why the contradiction here? There's no change in session usage from
current version. Currently too, once keys are set on asymmetric session, they
are used with multiple operations using that sessions, example - once RSA xform
is set with keys, then one can perform sign/verify/enc/dec. So, I don't see any
change in that notion with this proposal. All we are changing is, PMD which
does not need to store keys in specific format (like openssl PMD), can just
hold app buffer pointer till session-lifetime (eventually giving same effect as
sessionless). It will help such PMDs to optimize their session setup time by
avoiding unnecessary memcpy of keys buffers.
But my major concern is how this can lead to accidental errors. Making the
argument as const will mean the API won't edit its contents. But if there is a
pointer in that (key happens to be a pointer inside the xform), having const
for xform will not help. This is my understanding. Please correct me if I'm
wrong.
[Shally] This spec says " xform and its buffers remain constant" . So,
intention is to state to apps that buffer passed to xform should be const in
nature and that they should not modify it.
Also, I could have the xform allocated from stack (non const, regular local
variable) and then call the session_init. Would compiler throw an issue in that
case? I doubt so.
void abc(const int t)
{
printf("%d\n", t);
}
void main()
{
int t = 0;
abc(t);
t = 2;
abc(t);
}
To summarize, if this assumption is accepted, then compiler will not be able to
ensure it. And to properly use it, application will have to be drafted
differently. And when similar effect can be achieved by having session-less,
this seems redundant.
[Shally] Compiler may or may not warn on typecast error here. That's why spec
and documentation are put in place to ensure that application don't reuse them
or destroy them once "xform and its buffers" are set on session. And, same will
need to be documented about xform for session-less usage as well.
Even there, we would ensure that application do not re-use or modify xform and
its buffers until dequeue happen. So, practically I see, application would have
to take of these cases in session-less as well.
Since in session-based case, xform are set on it than ops, so we're moving same
definition on session. So for PMDs which support sessions-based implementations
( like ours) , believe it completely make sense to enable sessions to have
sessionless effect. If we don't change spec to enable optimization, then we're
making 1-approach slower than other. PMDs can adopt any approach more suitable
to them. But spec could be made flexible to allow them to experiment with both
approaches for performance. Else , PMDs will be forced to experiment around
sessionless which may be eventually be an unnecessary overhead for them.
Thanks
Shally
So this change is NACK from my side.
Thanks,
Anoob
From: Ayuj Verma
Sent: Wednesday, July 24, 2019 2:23 PM
To: mailto:akhil.go...@nxp.com
Cc: mailto:arkadiuszx.kusz...@intel.com; Shally Verma
<mailto:shal...@marvell.com>; Sunila Sahu <mailto:ss...@marvell.com>; Kanaka
Durga Kotamarthy <mailto:kkotamar...@marvell.com>; Anoob Joseph
<mailto:ano...@marvell.com>; mailto:dev@dpdk.org; Fiona Trahe
<mailto:fiona.tr...@intel.com>
Subject: Re: [PATCH v1 0/2] declare crypto asym xform immutable
+Fiona.
________________________________________
From: Ayuj Verma <mailto:ayve...@marvell.com>
Sent: 24 July 2019 14:21:55
To: mailto:akhil.go...@nxp.com <mailto:akhil.go...@nxp.com>
Cc: mailto:arkadiuszx.kusz...@intel.com <mailto:arkadiuszx.kusz...@intel.com>;
Shally Verma <mailto:shal...@marvell.com>; Sunila Sahu
<mailto:ss...@marvell.com>; Kanaka Durga Kotamarthy
<mailto:kkotamar...@marvell.com>; Anoob Joseph <mailto:ano...@marvell.com>;
mailto:dev@dpdk.org <mailto:dev@dpdk.org>; Ayuj Verma
<mailto:ayve...@marvell.com>
Subject: [PATCH v1 0/2] declare crypto asym xform immutable
Mark asym xform as immutable till lifetime
of session. It will save session setup time for
PMDs, which doesn't require any manipulation of
xform data, by directly using these buffers.
* Updated xform type in session init/configure
API as constant.
* Updated doc with proper transform description.
* Updated openssl PMD with above changes.
Ayuj Verma (2):
lib/crypto: declare crypto asym xform immutable
crypto/openssl: mark asym xform constant
doc/guides/prog_guide/cryptodev_lib.rst | 10 ++++++++++
drivers/crypto/openssl/rte_openssl_pmd_ops.c | 8 ++++----
lib/librte_cryptodev/rte_cryptodev.c | 2 +-
lib/librte_cryptodev/rte_cryptodev.h | 2 +-
lib/librte_cryptodev/rte_cryptodev_pmd.h | 2 +-
5 files changed, 17 insertions(+), 7 deletions(-)
--
1.8.3.1
