Hi Shally,
> -----Original Message----- > From: Shally Verma [mailto:shal...@marvell.com] > Sent: Friday, June 28, 2019 5:11 PM > To: Trahe, Fiona <fiona.tr...@intel.com>; Akhil Goyal <akhil.go...@nxp.com>; > Kusztal, ArkadiuszX > <arkadiuszx.kusz...@intel.com>; dev@dpdk.org > Subject: RE: [PATCH] cryptodev: extend api of asymmetric crypto by sessionless > > Hi Finoa, Akhil > > > -----Original Message----- > > From: Trahe, Fiona <fiona.tr...@intel.com> > > Sent: Thursday, June 27, 2019 5:25 PM > > To: Akhil Goyal <akhil.go...@nxp.com>; Kusztal, ArkadiuszX > > <arkadiuszx.kusz...@intel.com>; dev@dpdk.org; Shally Verma > > <shal...@marvell.com> > > Cc: Trahe, Fiona <fiona.tr...@intel.com> > > Subject: [EXT] RE: [PATCH] cryptodev: extend api of asymmetric crypto by > > sessionless > > > > External Email > > > > ---------------------------------------------------------------------- > ... > > > > > > > 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 > > > > support 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, so there's no advantage to setting up a session and it's > > > > an extra API call so preferable to avoid. > > > > > > Agreed, if Asymmetric is not likely to have sessions, it should not call > > > that > > API. > [Shally] I would rather say, asymmetric are using session based calls but > those are not so useful as unlike > symmetric, session params doesn't say same for larger amount of data. > So, its instead useful to have sessionless support for same. > > > > > > > > > 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 documented 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 occur on the op_enqueue_burst. > > > > > > 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. > > > > > > > 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 same result. > > > > > > 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. > > > > > > > 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 > > > > status of the next op which failed to enqueue. I think it better to > > > > detect this before the op_enqueue by providing a > > > > RTE_CRYPTODEV_FF_ASYM_SESSIONLESS feature flag. > > > > > > On second thought, we can have another value in the enum(op->status) > > > to say 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. > > > > > > I believe that would be simple and will not bother the data path or the > > > busy > > hardware. > > > Anyways in case of sessions also in sym, we make session on arrival of > > > 1st 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 - > > although that's also true - it's that if an op fails on the enqueue, > > applications > > may not check the op.status 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 using 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 ops 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 error the application > > should not resubmit the same op unless the error has been rectified. > > > > [Shally] I would favor to have feature flag instead, to keep it simple. > We're relying too much on documentation here. Any op status, be it > INVALID_OP_SESSION, or > NOT_SUPPORTED does not give > clear reason for failure. Assuming we agree on feature flag, then next > question comes if PMD set > SESSIONLESS feature flag, then does that mean it support *only* sessionless > OR both "session" and > "sessionless" ? > To solve this, we can define it like this: > 1. if PMD does not set _SESSIONLESS feature flag, that implicitly mean it > support session based only > (which is current case) > 2. if PMD does set _SESSIONLESS feature flag, then app can certainly use > sessionless, but if it invokes > session init API, then > - if PMD don't have session support, it should return NOT_SUPPORTED in > session_init() > - if PMD do have session support (which will be our case), then it will > allow session APIs. Then its app > discretion to choose either of these > > Does this sounds okay? > > Thanks > Shally [Fiona] Yes, this is ok for me. Or to paraphrase: sessionless feature flag just informs about sessionless. The response to session_init() informs whether sessions are supported or not.
