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

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

Hi Fiona, see below

> -----Original Message-----
> From: Trahe, Fiona <fiona.trahe@intel.com>
> Sent: Thursday, April 9, 2020 10:37 AM
> 
> Hi David,
> 
> Answer inline below
> 
> > -----Original Message-----
> > From: Coyle, David <david.coyle@intel.com>
> > Sent: Thursday, April 9, 2020 10:26 AM
> >
> > 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.
> 
> [Fiona] That sounds like a good idea. In that case my comments B, D and E are
> not relevant as assuming a broader scope.
> Comments A, C and F can still be considered, but are just suggestions, not
> blockers to this being applied in 20.05, they could easily be done in a later
> release.

[DC] For 20.05, I plan to address A, C and F from below.
We will look to address D in a later release when we add QAT multi-function PMD to see if unit test extensibility can be improved.
And B and E are now no longer applicable due to reduced scope.

> 
> ///snip///
> 
> > > 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.

[DC] The AESNI-MB rawdev will be renamed to rawdev_mfn_aesni_mb.
Keeping "rawdev_" as first prefix keeps this consistent with other rawdevs
Adding "mfn_" allows rawdevs implementing multi-function be found as you suggested

> > >     (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?

[DC] As stated above, this is no longer applicable due to reduced scope

> > >     (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.

[DC] This is a good suggestion as these same stats will also be needed by the QAT PMD.
I will make this change.

> > >     (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

[DC] As stated above we will look at making the unit tests more extensible when we add the QAT PMD
For now, keeping the tests in the drivers/raw/aesni_mb_mfn directory is consistent with existing rawdevs

> > >     (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.

[DC] We will remove support for multi-function from the crypto-perf too due to amount of code churn it required

> > >     (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)

[DC] To keep naming of files, directories, functions etc. consistent with the driver name which now includes the mfn tag,
the files, directories, functions etc. will be renamed to also include 'mfn' e.g. aesni_mb_mfn_rawdev.c/h, aesni_mb_mfn_probe()
A similar naming convention will be used for the QAT driver when we add that.

> > >
> > > I have a few minor comment- I'll reply on specific patches.