Re: [dpdk-dev] [PATCH v2 0/4] introduce multi-function processing support

["Coyle, David" <david.coyle@intel.com>]

Thanks for the detailed review Fiona.

Based on your feedback, we will reduce the scope of our plans for multi-function processing support in DPDK.

We will focus on implementing a rawdev-based AESNI-MB PMD for Crypto-CRC and Crypto-CRC-BIP processing and we will add QAT Crypto-CRC support in a later release.
This functionality is specific to accelerated dataplane processing for DOCSIS and PON MAC workloads.

We also note that there hasn't been much community engagement in the broader scope, so these simpler rawdev PMDs should be sufficient.
If the DPDK community is interested in expanding this concept later, then this can be explored, but it would not seem necessary for now.

We will also remove crypto-perf-tester updates to test rawdev multi-function processing as this would seem like too much code churn on that test tool.


> -----Original Message-----
> From: Trahe, Fiona <fiona.trahe@intel.com>
> Sent: Tuesday, April 7, 2020 7:06 PM
> 
> Hi David, Ferruh,
> 
> > -----Original Message-----
> > From: Coyle, David <david.coyle@intel.com>
> > Sent: Tuesday, April 7, 2020 12:28 PM
> > To: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org
> >
> > Hi Ferruh, see below
> >
> > > >
> > > > While DPDK's rte_cryptodev and rte_compressdev allow many
> > > > cryptographic and compression algorithms to be chained together in
> > > > one operation, there is no way to chain these with any error
> > > > detection or checksum algorithms. And there is no way to chain
> > > > crypto and compression algorithms together. The multi-function
> > > > interface will allow these chains to be created, and also allow
> > > > any future type of
> > > operation to be easily added.
> > >
> > > I was thinking if the cryptodev can be used instead but this
> > > paragraph already seems explained it. But again can you please elaborate
> why rawdev is used?
> >
> > [DC] There are a number of reasons the rawdev approach was ultimately
> chosen:
> >
> > 1) As the paragraph above explains, our primary use-case was to chain
> > a crypto operation with error detection algorithms such as CRC or BIP
> > as this could leverage optimized multi-function implementations such
> > as in the IPSec Multi-Buffer library and have a significant impact on
> performance of network access dataplane processing such as for vCMTS
> (DOCSIS MAC).
> > However such error detection algorithms are not Crypto functions so
> > some early advice we took was that it would not be suitable to add these to
> cryptodev.
> > Also, with a view to the future, the multi-function rawdev approach
> > allows crypto operations to be chained with compression operations.
> > Again, neither cryptodev or compressdev allows this type chaining.
> >
> > 2) An earlier version of multi-function suggested adding a new library
> > called rte_accelerator, as described here
> > http://mails.dpdk.org/archives/dev/2020-February/157045.html
> > We received some comments on the dev mailing list that we should not
> > add yet another acceleration library to DPDK.
> > And we also subsequently felt that the rawdev approach is better - that
> rationale is described below.
> >
> > rte_accelerator was also built on top of crypto and compress devices which
> already existed e.g.
> > drivers/crypto/aesni_mb, drivers/crypto/qat and drivers/compress/qat .
> > We subsequently realized that this was somewhat confusing when
> > performing multi-function type operations. For example, for combined
> > Crypto-Compression operations in the future, it would use either an
> > existing crypto or compress device, but neither really made sense when
> the operations are combined.
> > What was needed was a raw device which allowed an application to
> > configure any type of device and it's queue pairs and send any type of
> operation to that device.
> >
> > For both of these reasons, we decided to go down the rawdev route,
> > with a multi-function interface which can be used by several raw device
> drivers.
> >
> > 3) rawdev is the ideal place to try out a new approach like this to accessing
> devices.
> > Adding it here allows potential consumers of this such as VNF solution
> > providers to study and try out this approach, and take advantage of
> > the multi-function operations already supported in the IPSec
> > Multi-Buffer library such as Crypto-CRC and Crypto-CRC-BIP, all without
> DPDK committing to a new library upfront.
> > We would hope that the multi-function rawdev approach will mature over
> > time (through feedback from customers, new use-cases arising etc.), at
> > which point it could be potentially be moved into the main DPDK library set.
> >
> [Fiona] agree with above, in particular item (2). Just to expand a bit more on
> this: To do a crypto+compression op one would only send one op to one
> device. That device could have been either a crypto device which also
> implemented multi-fn, by adding compression, crc, etc to its capabilities OR a
> compression device which added crypto, crc, bip capabilities.
> Both were confusing and both raised questions about whether one could still
> do "normal" ops on the device, e.g. whether a normal crypto op could be
> interleaved on same qp as a multi-fn op. And how the capabilities would
> reflect what the device could do.
> It seems better to me to have a multifn device, which does explicitly just
> multifn ops.
> 
> Building this on top of rawdev is a good fit in my opinion, for the following
> reasons:
>  * avoids duplication of device APIs, as rawdev configure, start, stop,
> qp_setup, etc are all there already, also a nice set of stats APIs
>  * no impact or dependency added to rawdev lib
>  * avoids breakages on cryptodev or compressdev APIs
>  * avoids code duplication where functionality is already in a lib, e.g. re-uses
> cryptodev and compressdev headers. This adds a dependency, but I think
> that's ok as multi-function inherently depends on these functions.
>  * allows easy extension to add new functionality not currently available in
> any lib (CRC and BIP)
>  * allows evolution - range of useful chains which may emerge is not yet
> clear.
> 
> I do have some concerns, but these are resolvable in my opinion.
>     (A)    as there's no rawdev capability APIs and capabilities are essentially
> opaque to the rawdev API, the application uses explicit device naming to
> create or find a device that it knows will fulfil the multifunction APIs. I can see
> how this works for rawdevs which expect to have only one PMD that will
> fulfil the service, however I'd expect multi-fn to have at least 2 driver types,
> probably more eventually. To be extensible I'd suggest a naming convention
> for a class of devices. E.g. all devices and drivers that implement multi-fn
> should create a rawdev named mfn_xxx, e.g. mfn_aesni_mb, mfn_qat. The
> "mfn_" string should be defined in the mfn hdr. This would allow creation of
> apis like rte_multi_fn_count() which could find rawdevs which implement
> mfn_ without hardcoding specific driver names.
>     (B)    version control of the multi-function APIs. Putting the multifn API into
> the drivers/raw/common directory gives a lot of freedom while it's
> experimental. But can it benefit from API/ABI breakage infrastructure once
> the experimental tag is removed? Is there any reason not to move the
> common files to a lib/librte_multi_fn API?
>     (C)    xstat name strings should be moved from aesni_mb PMD to common
> and maybe use same naming convention, so appl can query same stats from
> any device, e.g. "mfn_successful_enqueues" could be implemented by all
> PMDs. If PMDs want to add driver-specific stats they can add their own
> without the mfn_, instead create their own unique stat name.
>     (D)    The unit test code is not extensible - again probably as based on
> previous rawdevs where there's only 1 implementation. For mfn I'd suggest
> replacing test_rawdev_selftest_aesni_mb() with a
> test_rawdev_selftest_multi_function(), which finds and/or creates all the
> raw PMDs implementing the mfn API and runs a test on each. And move the
> test files from the drivers/raw/aesni_mb dir to app/test and make generic so
> can run against any device named mfn_xxx
>     (E)    the main reason to piggyback onto crypto_perf_tool is to get the
> benefit of parsing and of all the crypto setup.  However this code has been
> inflated a lot, in part due to name diffs like rte_cryptodev_enqueue_burst()
> vs rte_multi_fn_enqueue_burst(). Maybe could be a lot slimmer with
> macros like ENQUEUE_BURST(dev, qp, void *op, burst_size) ? would mean a
> compile time decision to do either multifn OR cryptodev API calls, but I think
> that may work and simplify it.
>     (F)    ok, this is a bit pedantic, (sorry David!) but should the aesni_mb
> rawdev be renamed aesni_mb_mfn throughout (files, fns, dev and driver
> name). I mean it's implementing the mfn type of rawdev. I'm thinking ahead
> to QAT - it can implement a sym device, an asym device, a compression
> device and in future a multi-fn device. I'd propose to name it qat_multifn in
> case there'll be some other kind of rawdev device it could also implement in
> future. So the name qat_raw wouldn't be so helpful. (we made that mistake
> with qat_crypto, which should probably have been qat_sym_crypto - in my
> opinion more specific names are better)
> 
> I have a few minor comment- I'll reply on specific patches.