Re: [dpdk-dev] [PATCH v10 0/5] kni: add IOVA=VA support

[Vamsi Krishna Attunuru <vattunuru@marvell.com>]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Yigit, Ferruh
> Sent: Tuesday, October 15, 2019 9:05 PM
> To: Vamsi Krishna Attunuru <vattunuru@marvell.com>; dev@dpdk.org
> Cc: thomas@monjalon.net; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> olivier.matz@6wind.com; ferruh.yigit@intel.com; anatoly.burakov@intel.com;
> arybchenko@solarflare.com; Kiran Kumar Kokkilagadda
> <kirankumark@marvell.com>
> Subject: Re: [dpdk-dev] [PATCH v10 0/5] kni: add IOVA=VA support
> 
> On 8/16/2019 7:12 AM, vattunuru@marvell.com wrote:
> > From: Vamsi Attunuru <vattunuru@marvell.com>
> >
> > ---
> > V10 Changes:
> > * Fixed function return code on failure when min_chunk_size > pg_sz.
> > * Marked new mempool populate routine as EXPERIMENTAL.
> >
> > V9 Changes:
> > * Used rte_mempool_ops_calc_mem_size() instead of default handler in
> > the new mempool populate routine.
> > * Check min_chunk_size and return values.
> > * Removed ethdev_info memset to '0' and moved pci dev_info populate
> > into
> > kni_dev_pci_addr_get() routine.
> > * Addressed misc. review comments.
> >
> > V8 Changes:
> > * Remove default mempool populate() routine changes.
> > * Add kni app specific mempool create & free routines.
> > * Add new mempool populate routine to allocate page-aligned memzones
> > with page size to make sure all mempool objects reside on a page.
> > * Update release notes and map files.
> >
> > V7 Changes:
> > * Removed previously proposed mempool flag and made those page
> > boundary checks default in mempool populate() except for the objects
> > size bigger than the size of page.
> > * Removed KNI example application related changes since pool related
> > requirement is taken care in mempool lib.
> > * All PCI dev related info is moved under rte_eal_iova_mode() == VA check.
> > * Added wrapper functions in KNI module to hide IOVA checks and make
> > address translation routines more readable.
> > * Updated IOVA mode checks that enforcing IOVA=PA mode when IOVA=VA
> > mode is enabled.
> >
> > V6 Changes:
> > * Added new mempool flag to ensure mbuf memory is not scattered across
> > page boundaries.
> > * Added KNI kernel module required PCI device information.
> > * Modified KNI example application to create mempool with new mempool
> > flag.
> >
> > V5 changes:
> > * Fixed build issue with 32b build
> >
> > V4 changes:
> > * Fixed build issues with older kernel versions
> > * This approach will only work with kernel above 4.4.0
> >
> > V3 Changes:
> > * Add new approach to work kni with IOVA=VA mode using
> > iommu_iova_to_phys API.
> >
> > Kiran Kumar K (1):
> >   kni: add IOVA=VA support in KNI module
> >
> > Vamsi Attunuru (4):
> >   mempool: populate mempool with the page sized chunks
> >   kni: add IOVA=VA support in KNI lib
> >   kni: add app specific mempool create and free routines
> >   kni: modify IOVA mode checks to support VA
> 
> Hi Vamsi,
> 
> I am aware that this patchset is around for a long time, and I have seen your
> request to merge in 19.11, but as you can understand the concern I have is to
> break KNI or existing KNI applications while trying to add this new feature.
> 
> In high level, there are two issues,
> 
> 1) kernel modules updates expect there will be a backed device of the KNI which
> is not always true:
> 
>          if (dev_info.iova_mode) {
>  #ifdef HAVE_IOVA_AS_VA_SUPPORT
>                  pci = pci_get_device(dev_info.vendor_id,
>                                       dev_info.device_id, NULL);
>                  if (pci == NULL) {
>                          pr_err("pci dev does not exist\n");
>                          return -ENODEV;
>                  }
> 
> For example this breaks:
> ./build/app/testpmd -w0:0.0 --vdev net_kni0 --vdev net_kni1  -- -i

Vamsi> Yes, these can be fixed by forcing iommu_mode to PA for
vdev or vdev&pdev based KNI usecases. 

> 
> 
> 2) Applications will have to change the API to allocate the mempool.
> If the user upgraded to new version of DPDK, now it is possible to have iova=va
> mode and application should use new KNI API 'rte_kni_pktmbuf_pool_create()'
> to allocate mempool. And most probably application will have datapath and will
> use the KNI only for exception path, will there be any affect using KNI version of
> mempool alloc?

Vamsi> There would not be any affect in using KNI version of mempool.

> 
> 
> I would like to see KNI is enabled via iova=va mode, but can we have it limited to
> a specific command line argument or config option? This increases the test
> surface but at least old application can continue to work by default, what do you
> think?

Vamsi> Yes, it's appropriate to control the mode to ensure old apps work by default.
We are fine with having a command line arg or config option to enable KNI in iova=va mode.
Earlier we thought of having similar approach that also controls mempool allocation using
a newer mempool flag. After multiple reviews, flag has been discard and added a separate
mempool populate routine for these usecase. 

When command line arg/config option is introduced, functionality will be as below. 
Please correct me if any cases are missed or not considered.
Without command:
Existing KNI is intact, iommu mode will be PA.
With  command:
Pdev/vdev's iommu mode is considered and accordingly iova=va/pa is enabled. Application is
supposed to use KNI version of mempool alloc. I think these mempool quirk will go away when
Olivier's mempool patchset(RFC) is merged.

> 
> And I will put a few minor comments to the patches...
>