From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 3715DA3295 for ; Wed, 23 Oct 2019 12:05:48 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id F2A1C1C038; Wed, 23 Oct 2019 12:05:47 +0200 (CEST) Received: from EUR02-AM5-obe.outbound.protection.outlook.com (mail-eopbgr00055.outbound.protection.outlook.com [40.107.0.55]) by dpdk.org (Postfix) with ESMTP id A775C1C037 for ; Wed, 23 Oct 2019 12:05:46 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=I672Q9kfHRchvAGin4M5vNohqzBp8h5zS+iyFLEAWklQn/0uLnKIeFOJktouFQZtz1jYnGcfiNwKCMwLXQbhprdvLCy6kUM6vtPnXwsF1uksp/WeCqhtex9TPxmbtoNtE9O+W5UoVM4yWlop8e14K+ddvfoh1iiAEWiqfVRnZ2Q+hP9Bs9ISq3oGZFFP6N8AUWjbsbaFpCLokZs+3DPV9SpQaIejgMRb4mKJ9EGiwZEkJ/uYIqT/IDm6AE2N08pOXegrahOhmrim7GSphVqQNLUuJyY1kfaZYBIGsjeJmFSRmZJWGc/MhcAG8TW3w2tdNNb3+nWP7Z2FOk42mhcmQQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=3DAvBGv//SOB327Y8ZVWP/Y6puMALbNUSN20Zou8gZM=; b=oUFvWzBASpDHqEEfj0DHlFdTPuOoI22855UFFD9W0FqOdJAQWEAbE+Q8dZApNJxkR1DAUBUDFXpoNzM83L0itmtl1oV4e5kWZQDKSiM3IVHtb9fyJlarAwsbmYCoFTJeaGqdKZFs65itsyMU4d4Nz5QRQc/UetRZLxoPo/ibAAVTjSKDp+jt55t42mmGoLKVJv9VKmNa+gqcvz8w/vMpm01fyrsgQjF3dFksQ8yUy+nZeKYJJxynLPXj8jYhhdRtvsXQmkbTtSTp0Eb9U+nSBY2QHw7y3f8mSUWDIkeZSueiyliWO46hobVAiKJ6zbMzrXlS2x2DyOs3WEDcWmeeMw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=nxp.com; dmarc=pass action=none header.from=nxp.com; dkim=pass header.d=nxp.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nxp.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=3DAvBGv//SOB327Y8ZVWP/Y6puMALbNUSN20Zou8gZM=; b=WXahv8lErMxx01kfZTJZiLTIYgpS3S0Fyt5YKdA1NlLJ0Xjm2jYPDyezgTx0Z1SOSlOu2QDL3Sq5fUmFLbPNsuw4WUAC2TKIGHxL5MeygS8Nw4GIucTNaLVbGKefwxEtklABFP6+fm0vySF2A/bSaFh+MJNcktNCZvS7mHAhX+Y= Received: from VE1PR04MB6639.eurprd04.prod.outlook.com (10.255.118.11) by VE1PR04MB6717.eurprd04.prod.outlook.com (10.255.118.17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2387.20; Wed, 23 Oct 2019 10:05:45 +0000 Received: from VE1PR04MB6639.eurprd04.prod.outlook.com ([fe80::9dc:aa5c:2bb8:b561]) by VE1PR04MB6639.eurprd04.prod.outlook.com ([fe80::9dc:aa5c:2bb8:b561%6]) with mapi id 15.20.2367.022; Wed, 23 Oct 2019 10:05:45 +0000 From: Akhil Goyal To: "Ananyev, Konstantin" , "'dev@dpdk.org'" , "De Lara Guarch, Pablo" , 'Thomas Monjalon' , "Zhang, Roy Fan" , "Doherty, Declan" CC: 'Anoob Joseph' , Hemant Agrawal Thread-Topic: [RFC PATCH 1/9] security: introduce CPU Crypto action type and API Thread-Index: AQHVYm4LqyJkewM9NkuUWAfAmrqx1acbUiZggAAsN4CAAtsIgIAAT02AgAYXC5CAAbSDgIABbRGggAaWxgCAAPjG4IABs/OAgAuzNYCAAoY34IAE8G8AgAAH4mCAAbN9gIADA/pwgAZJhgCAAr+oMIAAcucAgAMT0cCACHlagIACi3iggATErYCAAV8CAIAAdXYAgAD56qA= Date: Wed, 23 Oct 2019 10:05:45 +0000 Message-ID: References: <20190903154046.55992-1-roy.fan.zhang@intel.com> <20190903154046.55992-2-roy.fan.zhang@intel.com> <9F7182E3F746AB4EA17801C148F3C6043369D686@IRSMSX101.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772580191926A17@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772580191962CD5@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772580191966116@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772580191966C23@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258019196A767@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258019196D53D@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258019196F386@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258019197206C@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258019197446B@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725801A8C69C5E@IRSMSX104.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725801A8C6D6AC@IRSMSX104.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725801A8C6E152@IRSMSX104.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB97725801A8C6E152@IRSMSX104.ger.corp.intel.com> Accept-Language: en-IN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=akhil.goyal@nxp.com; x-originating-ip: [92.120.1.65] x-ms-publictraffictype: Email x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: 167eb00c-e66a-4bb5-58f6-08d757a09242 x-ms-traffictypediagnostic: VE1PR04MB6717:|VE1PR04MB6717: x-ms-exchange-purlcount: 1 x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:8882; x-forefront-prvs: 019919A9E4 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(4636009)(346002)(396003)(376002)(39860400002)(136003)(366004)(189003)(199004)(54906003)(9686003)(55016002)(102836004)(6506007)(71200400001)(6306002)(5660300002)(14444005)(15650500001)(99286004)(81156014)(186003)(33656002)(3846002)(316002)(6116002)(229853002)(76176011)(26005)(7696005)(110136005)(71190400001)(6436002)(86362001)(25786009)(2906002)(8676002)(966005)(8936002)(478600001)(81166006)(66476007)(66446008)(66946007)(305945005)(6246003)(64756008)(66066001)(7736002)(256004)(66556008)(44832011)(11346002)(45080400002)(52536014)(14454004)(74316002)(476003)(76116006)(446003)(486006)(4326008)(30864003)(156664002)(921003)(1121003)(491001)(579004); DIR:OUT; SFP:1101; SCL:1; SRVR:VE1PR04MB6717; H:VE1PR04MB6639.eurprd04.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; MX:1; A:1; received-spf: None (protection.outlook.com: nxp.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: Fkrj1ot4s0HQW8PnTSg498ko3Wm7LcvEOr4BdmdBuVOTLmomGhBd+zwhZ88ohdCpzt9Y+Thb+51TQN9V0W7B3VwxjHj3E4XZRVqkVDh95d9GNMFUVVCbc5gqf0zxTf+TSw1P92Hpt+0DmEd0AKlMmmBKdIVGGZZwNj8UgQXG9UpvDMfpVT3N/fjI/F+BqNTY6SabeXZia1xktzDK09D9Va3ARlxl5AU4BdJjRyqv/bXNLj9WsaBcM/7X2Y558Lsda5ucRirrUsSHByIxdHArOgY2W2T5ThUCRFmlY1Xqouo3yE1AR84/gQISpQPAnnp3IebYa9KiTq4kOAGJXFbnneP8NMErbj4td4OPJJLr2mgEX2gVv9Sj/ieDeTZSx8OS+qqQhWvPixpWEpu2XLEpl7CW8UpG+6q7el8DZmpykAAlxxHgJlIUO0ottkCzepYkFZG0BRZwk4MEJa3Ssm3Z3zjpHzauIJ1NwVR/MgeabYg= Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: nxp.com X-MS-Exchange-CrossTenant-Network-Message-Id: 167eb00c-e66a-4bb5-58f6-08d757a09242 X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Oct 2019 10:05:45.4461 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: sCIgeAVLB+GtlZ8mANa77/B1gFPXagBdH45GALEIzTvDEEfi+chu9P1A/sRcPPqW43fSvmZZ0dM6p7EAvszJNw== X-MS-Exchange-Transport-CrossTenantHeadersStamped: VE1PR04MB6717 Subject: Re: [dpdk-dev] [RFC PATCH 1/9] security: introduce CPU Crypto action type and API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Konstantin, >=20 > Hi Akhil, >=20 >=20 > > > > Added my comments inline with your draft. > > > > [snip].. > > > > > > > > > > > > > > Ok, then my suggestion: > > > > > Let's at least write down all points about crypto-dev approach wh= ere 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 w= ell. > > > > > > Good point. > > > > > > > What I understood from the patchset, > > > > "You need a synchronous API to perform crypto operations on raw dat= a > 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) > > > > > > Yes, this is correct, I think. > > > > > > > > > > > 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 'al= go' > 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 opaq= ue > > > 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_xf= orm > > > *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), PM= D > > > writer is totally free > > > > with it format and contents. > > > > > > > > [Akhil] It will have breakage at some point till we don't hit the u= nion size. > > > > > > Not sure, what union you are talking about? > > > > Union of xforms in rte_security_session_conf >=20 > Hmm, how does it relates here? > I thought we discussing pure rte_cryptodev_sym_session, no? >=20 > > > > > > > > > Rather I don't suspect there will be more parameters added. > > > > Or do we really care about the ABI breakage when the argument is ab= out > > > > the correct place to add a piece of code or do we really agree to a= dd code > > > > anywhere just to avoid that breakage. > > > > > > I am talking about maintaining it in future. > > > if your struct is not seen externally, no chances to introduce ABI br= eakage. > > > > > > > > > > > 2) each session entity is self-contained, user doesn't need to bri= ng 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. > > > > > > True, but it doesn't mean we can't/shouldn't have it. > > > > We can have it if it is not adding complexity for the user. Creating 2 = different > code > > Paths for user is not desirable for the stack developers. > > > > > > > > > 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 s= ession > > > basis, > > > > or on a per group of same sessions, or... > > > > > > > > [Akhil] Will the user really care which process API should be calle= d from the > > > PMD. > > > > Rather it should be driver's responsibility to store that in the se= ssion private > > > data > > > > which would be opaque to the user. As per my suggestion same proces= s > > > function can > > > > be added to multiple sessions or a single session can be managed in= side the > > > PMD. > > > > > > In that case we either need to have a function per session (stored in= ternally), > > > or make decision (branches) at run-time. > > > But as I said in other mail - I am ok to add small shim structure her= e: > > > either rte_crypto_cpu_sym_session { void *ses; struct > > > rte_crypto_cpu_sym_session_ops ops; } > > > or 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). > > > > Again that will be a separate API call from the user perspective which = is not > good. > > > > > > > > > > > > > > > > > 4) No mandatory mempools for private sessions. User can allocate > > > memory for cpu-crypto > > > > session whenever he likes. > > > > > > > > [Akhil] you mean session private data? > > > > > > Yes. > > > > > > > You would need that memory anyways, user will be > > > > allocating that already. You do not need to manage that. > > > > > > What I am saying - right now user has no choice but to allocate it vi= a > mempool. > > > Which is probably not the best options for all cases. > > > > > > > > > > > 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 fo= r 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. > > > > > > I think majority of code for both paths will be common, plus even we'= ll reuse > > > current sym_session_init() - > > > changes in PMD session_init() code will be unavoidable. > > > But yes, it will be new entry in devops, that PMD will have to suppor= t. > > > Ok to add it as 7) to the list. > > > > > > > - Stacks which will be using these new APIs also need to maintain t= wo > > > > code path for the same processing while doing session initializatio= n > > > > for sync and async > > > > > > That's the same as #5 above, I think. > > > > > > > > > > > > > > > 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 privat= e > > > 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 mod= es. > > > > For some implementations that would probably mean that under t= he > > > hood PMD would create > > > > 2 different session structs (sync/async) and then use one or a= nother > > > 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 spec= ify cpu-crypto > > > related parameters too, > > > > even if they don't plan to use that mode - i.e. behavior ch= ange, > > > existing app change. > > > > - might cause extra space overhead. > > > > > > > > [Akhil] It will not contradict with #1, you will only have few chec= ks in the > > > session init PMD > > > > Which support this mode, find appropriate values and set the approp= riate > > > process() in it. > > > > User should be able to call, legacy enq-deq as well as the new proc= ess() > > > 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 f= or the user. > > > > > > Ok, but that's what I am saying - if PMD would *always* have to creat= e a > > > session that can handle > > > both modes (sync/async), then user would *always* have to provide > parameters > > > for both modes too. > > > Otherwise if let say user didn't setup sync specific parameters at al= l, what > PMD > > > should do? > > > - return with error? > > > - init session that can be used with async path only? > > > My current assumption is #1. > > > If #2, then how user will be able to distinguish is that session vali= d for both > > > modes, or only for one? > > > > I would say a 3rd option, do nothing if sync params are not set. > > Probably have a debug print in the PMD(which support sync mode) to spec= ify > that > > session is not configured properly for sync mode. >=20 > So, just print warning and proceed with init session that can be used wit= h async > path only? > Then it sounds the same as #2 above. > Which actually means that sync mode parameters for sym_session_init() > becomes optional. > Then we need an API to provide to the user information what modes > (sync+async/async only) is supported by that session for given dev_id. > And user would have to query/retain this information at control-path, > and store it somewhere in user-space together with session pointer and de= v_ids > to use later at data-path (same as we do now for session type). > That definitely requires changes in control-path to start using it. > Plus the fact that this value can differ for different dev_ids for the sa= me session - > doesn't make things easier here. API wont be required to specify that. Feature flag will be sufficient, not = a big change >From the application perspective. Here is some pseudo code just to elaborate my understanding. This will need= some >From application, If(dev_info->feature_flags & RTE_CRYPTODEV_FF_SYNC) { /* set additional params in crypto xform */ } Now in the driver, pmd_sym_session_configure(dev,xform,sess,mempool) { ... If(dev_info->feature_flags & RTE_CRYPTODEV_FF_SYNC && xform->/*sync params are set*/) { /*Assign process function pointer in sess->priv_data*/ } /* It may return error if FF_SYNC is set and params are not correct. It would be upto the driver whether it support both SYNC and ASYNC= .*/ } Now the new sync API pmd_process(...) { If(dev_info->feature_flags & RTE_CRYPTODEV_FF_SYNC && sess_priv->process !=3D NULL) sess_priv->process(...); else ASSERT("sync mode not configured properly or not supported"); } In the data path, there is no extra processing happening. Even in case of your suggestion, you should have these type of error checks= , You cannot blindly trust on the application that the pointers are correct. >=20 > > Internally the PMD will not store the process() API in the session priv= data > > And while calling the first packet, devops->process will give an assert= that > session > > Is not configured for sync mode. The session validation would be done i= n any > case > > your suggestion or mine. So no extra overhead at runtime. >=20 > I believe that after session_init() user should get either an error or > valid session handler that he can use at runtime. > Pushing session validation to runtime doesn't seem like a good idea. >=20 It may get a warning from the PMD, that FF_SYNC is set but params are not Correct/available. See above. > > > > > > > > > > > > > > > > > > > > 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. > > > > > > I don't think there is an easy way to fix that with existing API. > > > > > > > > > > > > > > > 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 (*proc= ess) 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 wo= uld be > inline > > > with the > > > > current DPDK methodology. > > > > > > If we'll add process() into rte_cryptodev itself (same as we have > > > enqueue_burst/dequeue_burst), > > > then it will be an ABI breakage. > > > Also there are discussions to get rid of that approach completely: > > > > https://eur01.safelinks.protection.outlook.com/?url=3Dhttp%3A%2F%2Fmails.= dpd > k.org%2Farchives%2Fdev%2F2019- > September%2F144674.html&data=3D02%7C01%7Cakhil.goyal%40nxp.com%7 > C1859dc1d29cd45a51e9908d7571784bb%7C686ea1d3bc2b4c6fa92cd99c5c301 > 635%7C0%7C0%7C637073630835415165&sdata=3DBz9jgisyVzRJNt1BijtvSlurh > JU1vXBbynNwlMDjaco%3D&reserved=3D0 > > > So I am not sure this is a recommended way these days. > > > > We can either have it in rte_cryptodev or in rte_cryptodev_ops whicheve= r > > is good for you. > > > > Whether it is ABI breakage or not, as per your requirements, this is th= e correct > > approach. Do you agree with this or not? >=20 > I think it is possible approach, but not the best one: > it looks quite flakey to me (see all these uncertainty with sym_session_i= nit > above), > plus introduces extra overhead at data-path. Uncertainties can be handled appropriately using a feature flag And As per my understanding there is no extra overhead in data path. >=20 > > > > Now handling the API/ABI breakage is a separate story. In 19.11 release= we > > Are not much concerned about the ABI breakages, this was discussed in > > community. So adding a new dev_ops wouldn't have been an issue. > > Now since we are so close to RC1 deadline, we should come up with some > > other solution for next release. May be having a pmd API in 20.02 and > > converting it into formal one in 20.11 > > > > > > > > > > > What you are suggesting is a new way to get the things done without= much > > > benefit. > > > > > > Would help with ABI stability plus better performance, isn't it enoug= h? > > > > > > > Also I don't see any performance difference as crypto workload is h= eavier > than > > > > Code cycles, so that wont matter. > > > > > > It depends. > > > Suppose function call costs you ~30 cycles. > > > If you have burst of big packets (let say crypto for each will take ~= 2K cycles) > that > > > belong > > > to the same session, then yes you wouldn't notice these extra 30 cycl= es at all. > > > If you have burst of small packets (let say crypto for each will take= ~300 > cycles) > > > each > > > belongs to different session, then it will cost you ~10% extra. > > > > Let us do some profiling on openssl with both the approaches and find o= ut the > > difference. > > > > > > > > > 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_op= s 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(se= s, ...); > > > > /*and then inside PMD specifc process: */ > > > > pmd_private_session =3D sess- > >sess_data[this_pmd_driver_id].data; > > > > /* and then most likely either */ > > > > pmd_private_session->process(pmd_private_sessi= on, ...); > > > > /* 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 da= ta 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, > > > > > > I don't think it is good idea. > > > PMD specific API is sort of deprecated path, also there is no clean w= ay to use > it > > > within the libraries. > > > > I know that this is a deprecated path, we can use it until we are not a= llowed > > to break ABI/API > > > > > > > > > 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 li= ke openssl > > > which is used by all > > > > vendors. > > > > > > I feel quite opposite, from my perspective majority of SW backed PMDs= will > > > benefit from it. > > > And I don't see anything Intel specific in my proposals above. > > > About openssl PMD: I am not an expert here, but looking at the code, = I think > it > > > will fit really well. > > > Look yourself at its internal functions: > > > process_openssl_auth_op/process_openssl_crypto_op, > > > I think they doing exactly the same - they use sync API underneath, a= nd they > are > > > session based > > > (AFAIK you don't need any device/queue data, everything that needed f= or > > > crypto/auth is stored inside session). > > > > > By vendor specific, I mean, > > - no PMD would like to have 2 different variants of session Init APIs f= or doing > the same stuff. > > - stacks will become vendor specific while using 2 separate session cre= ate APIs. > No stack would > > Like to support 2 variants of session create- one for HW PMDs and one f= or SW > PMDs. >=20 > I think what you refer on has nothing to do with 'vendor specific'. > I would name it 'extra overhead for PMD and stack writers'. > Yes, for sure there is extra overhead (as always with new API) - > for both producer (PMD writer) and consumer (stack writer): > New function(s) to support, probably more tests to create/run, etc. > Though this API is optional - if PMD/stack maintainer doesn't see > value in it, they are free not to support it. > From other side, re-using rte_cryptodev_sym_session_init() > wouldn't help anyway - both data-path and control-path would differ > from async mode anyway. > BTW, right now to support different HW flavors > we do have 4 different control and data-paths for both > ipsec-secgw and librte_ipsec: > lkds-none/lksd-proto/inline-crypto/inline-proto. > And that is considered to be ok. No that is not ok. We cannot add new paths for every other case. Those 4 are controlled using 2 set of APIs. We should try our best to Have minimum overhead to the application writer. This pain was also discuss= ed In the one of DPDK conference as well. DPDK is not a standalone entity, there are stacks running over it always. We should not add API for every other use case when we have an alternative Approach with the existing API set. Now introducing another one would add to that pain and a lot of work for Both producer and consumer. It would be interesting to see how much performance difference will be ther= e in the Two approaches. As per my understanding it wont be much as compared to the Extra work that you will be inducing. -Akhil > Honestly, I don't understand why SW backed implementations > can't have their own path that would suite them most. > Konstantin >=20 >=20 >=20 >=20 >=20