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

[Trahe, Fiona]

Hi David,

Answer inline below

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

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