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

[Trahe, Fiona]

Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal at nxp.com]
> Sent: Monday, June 24, 2019 8:05 AM
> To: Trahe, Fiona <fiona.trahe at intel.com>; Kusztal, ArkadiuszX <arkadiuszx.kusztal at intel.com>;
> dev at dpdk.org; Shally Verma <shallyv at marvell.com>
> Subject: RE: [PATCH] cryptodev: extend api of asymmetric crypto by sessionless
> 
> Hi Fiona
> 
> >
> > Hi Akhil,
> >
> > > -----Original Message-----
> > > From: Akhil Goyal [mailto:akhil.goyal at nxp.com]
> > > Sent: Friday, June 21, 2019 1:23 PM
> > > To: Trahe, Fiona <fiona.trahe at intel.com>; Kusztal, ArkadiuszX
> > <arkadiuszx.kusztal at intel.com>;
> > > dev at dpdk.org; shally.verma at 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 at nxp.com]
> > > > > Sent: Thursday, June 20, 2019 3:17 PM
> > > > > To: Kusztal, ArkadiuszX <arkadiuszx.kusztal at intel.com>; dev at dpdk.org
> > > > > Cc: Trahe, Fiona <fiona.trahe at intel.com>;
> > > > shally.verma at caviumnetworks.com
> > > > > Subject: RE: [PATCH] cryptodev: extend api of asymmetric crypto by
> > > > sessionless
> > > > > >
> > > > > > Asymmetric cryptography algorithms may more likely use
> > > > > > sessionless API so there is need to extend API.
> > > > > >
> > > > > > Signed-off-by: Arek Kusztal <arkadiuszx.kusztal at intel.com>
> > > > > > ---
> > > > > Acked-by: Akhil Goyal <akhil.goyal at nxp.com>
> > > >
> > > > [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 that 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 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.
> 
> > 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.

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