Hi David, Answer inline below
> -----Original Message----- > From: Coyle, David <david.co...@intel.com> > Sent: Thursday, April 9, 2020 10:26 AM > To: Trahe, Fiona <fiona.tr...@intel.com>; Yigit, Ferruh > <ferruh.yi...@intel.com>; dev@dpdk.org > Cc: Doherty, Declan <declan.dohe...@intel.com>; De Lara Guarch, Pablo > <pablo.de.lara.gua...@intel.com>; Ryan, Brendan <brendan.r...@intel.com>; > shreyansh.j...@nxp.com; hemant.agra...@nxp.com; Akhil Goyal > <akhil.go...@nxp.com>; > O'loingsigh, Mairtin <mairtin.oloings...@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.
