Re: [dpdk-dev] [PATCH v1 0/2] declare crypto asym xform immutable

[Anoob Joseph <anoobj@marvell.com>]

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 <anoobj@marvell.com>; Ayuj Verma
> <ayverma@marvell.com>; akhil.goyal@nxp.com
> Cc: arkadiuszx.kusztal@intel.com; Sunila Sahu <ssahu@marvell.com>; Kanaka
> Durga Kotamarthy <kkotamarthy@marvell.com>; dev@dpdk.org; Fiona
> Trahe <fiona.trahe@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 <ayverma@marvell.com>; akhil.goyal@nxp.com
> Cc: arkadiuszx.kusztal@intel.com; Shally Verma <shallyv@marvell.com>;
> Sunila Sahu <ssahu@marvell.com>; Kanaka Durga Kotamarthy
> <kkotamarthy@marvell.com>; dev@dpdk.org; Fiona Trahe
> <fiona.trahe@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.goyal@nxp.com
> Cc: mailto:arkadiuszx.kusztal@intel.com; Shally Verma
> <mailto:shallyv@marvell.com>; Sunila Sahu <mailto:ssahu@marvell.com>;
> Kanaka Durga Kotamarthy <mailto:kkotamarthy@marvell.com>; Anoob
> Joseph <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org; Fiona Trahe
> <mailto:fiona.trahe@intel.com>
> Subject: Re: [PATCH v1 0/2] declare crypto asym xform immutable
> 
> +Fiona.
> ________________________________________
> From: Ayuj Verma <mailto:ayverma@marvell.com>
> Sent: 24 July 2019 14:21:55
> To: mailto:akhil.goyal@nxp.com <mailto:akhil.goyal@nxp.com>
> Cc: mailto:arkadiuszx.kusztal@intel.com
> <mailto:arkadiuszx.kusztal@intel.com>; Shally Verma
> <mailto:shallyv@marvell.com>; Sunila Sahu <mailto:ssahu@marvell.com>;
> Kanaka Durga Kotamarthy <mailto:kkotamarthy@marvell.com>; Anoob
> Joseph <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org
> <mailto:dev@dpdk.org>; Ayuj Verma <mailto:ayverma@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