Hi Shally,
Oh yeah! Should've noticed. My settings is to use the incoming mail's
formatting. Mail Ayuj created the issue.
Thanks,
Anoob
> -----Original Message-----
> From: Shally Verma
> Sent: Thursday, July 25, 2019 10:52 AM
> To: Anoob Joseph <ano...@marvell.com>; Ayuj Verma
> <ayve...@marvell.com>; akhil.go...@nxp.com
> Cc: arkadiuszx.kusz...@intel.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 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
