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

Shally Verma shallyv at marvell.com
Fri Jun 28 18:10:57 CEST 2019


Hi Finoa, Akhil

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

> Note - it's out of the scope of this thread whether the same should be true
> for symmetric crypto ops.
> >
> > >
> > >
> > > > > We'll follow up with SESSIONLESS QAT implementation and UTs in a
> > > > > separate patchset.
> > > > > Also documentation updates should go with this API patch, i.e.
> > > > >  - update section 16.7.2 in the cryptodev programmers guide -
> > > > > and review
> > > 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?
> >
> > I believe you should update the documentation when some PMD support
> sessionless.
> > I think we should hold back this patch, until you have a sample PMD/app
> ready for reference.
> >
> > You may end up adding a few more updates to the lib while actually
> supporting the functionality.
> [Fiona] ok
> 
> >
> > >
> > > >
> > > > >  - fix comment in rte_crypto.h under STATUS_INVALID_SESSION
> > > > >  - release note
> > > > >
> > > > >
> > > >
> > > >
> > > > -Akhil


More information about the dev mailing list