From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.7 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B7A73C48BD6 for ; Thu, 27 Jun 2019 11:54:59 +0000 (UTC) Received: from dpdk.org (dpdk.org [92.243.14.124]) by mail.kernel.org (Postfix) with ESMTP id 587582086D for ; Thu, 27 Jun 2019 11:54:59 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 587582086D Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=dev-bounces@dpdk.org Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 66F544C93; Thu, 27 Jun 2019 13:54:58 +0200 (CEST) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 7520B4C8B for ; Thu, 27 Jun 2019 13:54:56 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Jun 2019 04:54:55 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,423,1557212400"; d="scan'208";a="185242485" Received: from irsmsx104.ger.corp.intel.com ([163.33.3.159]) by fmsmga004.fm.intel.com with ESMTP; 27 Jun 2019 04:54:53 -0700 Received: from irsmsx101.ger.corp.intel.com ([169.254.1.80]) by IRSMSX104.ger.corp.intel.com ([169.254.5.143]) with mapi id 14.03.0439.000; Thu, 27 Jun 2019 12:54:53 +0100 From: "Trahe, Fiona" To: Akhil Goyal , "Kusztal, ArkadiuszX" , "dev@dpdk.org" , Shally Verma CC: "Trahe, Fiona" Thread-Topic: [PATCH] cryptodev: extend api of asymmetric crypto by sessionless Thread-Index: AQHVGkUHWWASMFfof0mYfFRjYZRUS6akoF0AgAF30JD///qlgIAAHWVggARAxgCABRFeYA== Date: Thu, 27 Jun 2019 11:54:52 +0000 Message-ID: <348A99DA5F5B7549AA880327E580B435897A68FE@IRSMSX101.ger.corp.intel.com> References: <20190603194411.8352-1-arkadiuszx.kusztal@intel.com> <348A99DA5F5B7549AA880327E580B435897A26ED@IRSMSX101.ger.corp.intel.com> <348A99DA5F5B7549AA880327E580B435897A28B6@IRSMSX101.ger.corp.intel.com> In-Reply-To: Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMGMxZmYyZTItZjA3Ny00NDVhLWI5NzQtOTQ0MTMzNDBmY2Q0IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiOHg5N25ObW9LRzNwTjZ5NlVJVVdXSHhPU2lXN3ZOWk9HeHU5UXJRcTY5bHlnUEZzSEFrdkRSYlRcL3VTSERlcEYifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] cryptodev: extend api of asymmetric crypto by sessionless 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 Akhil, > -----Original Message----- > From: Akhil Goyal [mailto:akhil.goyal@nxp.com] > Sent: Monday, June 24, 2019 8:05 AM > To: Trahe, Fiona ; Kusztal, ArkadiuszX ; > dev@dpdk.org; Shally Verma > Subject: RE: [PATCH] cryptodev: extend api of asymmetric crypto by sessio= nless >=20 > Hi Fiona >=20 > > > > Hi Akhil, > > > > > -----Original Message----- > > > From: Akhil Goyal [mailto:akhil.goyal@nxp.com] > > > Sent: Friday, June 21, 2019 1:23 PM > > > To: Trahe, Fiona ; Kusztal, ArkadiuszX > > ; > > > dev@dpdk.org; shally.verma@caviumnetworks.com > > > Subject: RE: [PATCH] cryptodev: extend api of asymmetric crypto by > > sessionless > > > > > > Hi Fiona, > > > > > > > > > > > Hi Akhil, Arek, Shally, > > > > > > > > > -----Original Message----- > > > > > From: Akhil Goyal [mailto:akhil.goyal@nxp.com] > > > > > Sent: Thursday, June 20, 2019 3:17 PM > > > > > To: Kusztal, ArkadiuszX ; dev@dpdk.= org > > > > > Cc: Trahe, Fiona ; > > > > shally.verma@caviumnetworks.com > > > > > Subject: RE: [PATCH] cryptodev: extend api of asymmetric crypto b= y > > > > sessionless > > > > > > > > > > > > Asymmetric cryptography algorithms may more likely use > > > > > > sessionless API so there is need to extend API. > > > > > > > > > > > > Signed-off-by: Arek Kusztal > > > > > > --- > > > > > Acked-by: Akhil Goyal > > > > > > > > [Fiona] The code is ok but I think a little more is needed. > > > > As all PMDs don't support sessionless, this needs to be handled as = an > > optional > > > > capability. > > > > And in future some PMDs may only support SESSIONLESS and some only > > support > > > > WITH_SESSION. > > > > > > > > > I believe this holds true for symmetric crypto as well. But adding a = feature flag > > for everything may beat > > > the purpose > > > Of adding a feature flag. Sessionless crypto operations in symmetric = crypto is > > being used without any > > > issue for a long > > > And nobody feel the need of that as of today. So my question is how > > asymmetric crypto pmds are > > > different that they > > > Need feature flag? > > > > > > If the driver does not support sessionless, then it may give an error= while > > creating it. I don't think that is > > > an issue. It is > > > Already being handled in the rte_crypto_op by an enum which denote th= at the > > 'op' need to be > > > processed with some > > > Session or with xform. > > > > > > > So I propose adding 2 feature flags to the API > > > > RTE_CRYPTODEV_FF_ASYM_WTH_SESSION > > > > RTE_CRYPTODEV_FF_ASYM_SESSIONLESS > > > > and including in this patch the PMD and UT changes to set and test = the first > > flag. > > [Fiona] symmetric crypto is inherently session-based, so all PMDs suppo= rt this. I > > don't know how much real use SESSIONLESS actually gets. > > For asymmetric, my understanding is that sessionless is more likely to = be used. > > Sequences of ops using the same params/keys are an unlikely use-case, s= o > > there's no advantage to setting up a session and it's an extra API call= so > > preferable to avoid. >=20 > Agreed, if Asymmetric is not likely to have sessions, it should not call = that API. >=20 > > That said, I think it would be ok with one feature flag. > > If a PMD doesn't support WITH_SESSION, the session_init API will fail = with - > > ENOTSUP, so giving the app the information it needs. This can be docume= nted > > as a PMD limitation and I'm ok with it not having a feature flag. > > However if a PMD doesn't support SESSIONLESS, then the fail will only o= ccur on > > the op_enqueue_burst. >=20 > Yes on the first enqueue, before actually submitting to the hardware, in = the driver itself > Before sending the request to hardware and return OP_INVALID_SESSION. >=20 > > Failure to enqueue the next op is a typical outcome on a busy hardware = device, > > and the app will likely assume the device is busy and try again with sa= me result. >=20 > The PMD will not be sending the request to hardware if sessionless is not= supported. > And app will not enqueue the op again if the previous error is OP_INVALID= _SESSION. >=20 > > The PMD could change the op.status to OP_STATUS_ERROR or > > OP_INVALID_SESSION but it would still require the app to check the stat= us of the > > next op which failed to enqueue. I think it better to detect this befor= e the > > op_enqueue by providing a RTE_CRYPTODEV_FF_ASYM_SESSIONLESS feature > > flag. >=20 > On second thought, we can have another value in the enum(op->status) to s= ay sessionless > Is not supported if OP_INVALID_SESSION looks ambiguous instead of setting= a feature flag > and making each driver set it if it support sessionless or not. >=20 > I believe that would be simple and will not bother the data path or the b= usy hardware. > Anyways in case of sessions also in sym, we make session on arrival of 1s= t packet. That same logic > Can be applied here also. I don't think that will be an issue. [Fiona] The issue for me isn't that OP_INVALID_SESSION is ambiguous - altho= ugh that's also true - it's that if an op fails on the enqueue, applications may not check the op.statu= s and so not notice the fail and would likely resubmit, assuming the op didn't enqueue because = of a busy device. This could result in an infinite loop. Also in my understanding the enqueue_burst() call is part of the data path,= in which I'd include the PMD processing as well as the hardware processing, so I think adding a = check for this case DOES affect the data-path. But a few extra cycles on the data-path to check the op.status is not a big= performance impact for asymmetric crypto. So I'd suggest adding a generic RTE_CRYPTO_OP_STATUS_NOT_SUPPORTED and usin= g for this case as long as we also document the following on the API: For asymmetric crypto operations, if an op fails to enqueue, the op.status = must be set appropriately and the PMD should return without enqueuing any subsequent op= s in that burst. It's up to the application to check if less than the full burst is enqueued= and in this case to check the status of the first unenqueued op. If still NOT_PROCESSED, it's likely due = to a busy device and a later retry with the same op can be expected to succeed, for any other er= ror the application should not resubmit the same op unless the error has been rectified. Note - it's out of the scope of this thread whether the same should be true= for symmetric crypto ops. >=20 > > > > > > > > We'll follow up with SESSIONLESS QAT implementation and UTs in a se= parate > > > > patchset. > > > > Also documentation updates should go with this API patch, i.e. > > > > - update section 16.7.2 in the cryptodev programmers guide - and r= eview > > that > > > > doc in case other sections need updating. > > > Yes this needs to be updated if the implementation is complete and we= have > > some PMD supporting > > > that. > > [Fiona] Are you saying we need to submit the PMD changes with this API = patch? > > Or can we do > > separately as we'd planned? >=20 > I believe you should update the documentation when some PMD support sessi= onless. > I think we should hold back this patch, until you have a sample PMD/app r= eady for reference. >=20 > You may end up adding a few more updates to the lib while actually suppor= ting the functionality. [Fiona] ok >=20 > > > > > > > > > - fix comment in rte_crypto.h under STATUS_INVALID_SESSION > > > > - release note > > > > > > > > > > > > > > > > > -Akhil