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

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

Thanks David,
Sounds good.
I look forward to the v3.
Fiona

> -----Original Message-----
> From: Coyle, David <david.coyle@intel.com>
> Sent: Thursday, April 9, 2020 12:56 PM
> To: Trahe, Fiona <fiona.trahe@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org
> Cc: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; Ryan, Brendan <brendan.ryan@intel.com>;
> shreyansh.jain@nxp.com; hemant.agrawal@nxp.com; Akhil Goyal <akhil.goyal@nxp.com>;
> O'loingsigh, Mairtin <mairtin.oloingsigh@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 0/4] introduce multi-function processing support
> 
> 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.