Re: [dpdk-dev] [PATCH] cryptodev: extend api of asymmetric crypto by sessionless

["Trahe, Fiona" <fiona.trahe@intel.com>]

Hi Shally,

> -----Original Message-----
> From: Shally Verma [mailto:shallyv@marvell.com]
> Sent: Friday, June 28, 2019 5:11 PM
> To: Trahe, Fiona <fiona.trahe@intel.com>; Akhil Goyal <akhil.goyal@nxp.com>; Kusztal, ArkadiuszX
> <arkadiuszx.kusztal@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.trahe@intel.com>
> > Sent: Thursday, June 27, 2019 5:25 PM
> > To: Akhil Goyal <akhil.goyal@nxp.com>; Kusztal, ArkadiuszX
> > <arkadiuszx.kusztal@intel.com>; dev@dpdk.org; Shally Verma
> > <shallyv@marvell.com>
> > Cc: Trahe, Fiona <fiona.trahe@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.