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

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

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, October 16, 2019 9:52 PM
> To: Vamsi Krishna Attunuru <vattunuru@marvell.com>; Yigit, Ferruh
> <ferruh.yigit@linux.intel.com>
> Cc: thomas@monjalon.net; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> olivier.matz@6wind.com; anatoly.burakov@intel.com;
> arybchenko@solarflare.com; Kiran Kumar Kokkilagadda
> <kirankumark@marvell.com>; dev@dpdk.org
> Subject: [EXT] Re: [dpdk-dev] [PATCH v10 0/5] kni: add IOVA=VA support
> 
> External Email
> 
> ----------------------------------------------------------------------
> On 10/16/2019 1:17 PM, Vamsi Krishna Attunuru wrote:
> >
> >
> >> -----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.
> 
> If you were developing a product, would you rely on a KNI API for pkt_mbuf
> allocation? What if there is a problem, will it be as easy as mempool/mbuf
> API to figure out and fix? I am not sure about pushing users to this direction?

Vamsi >  If user wants to run KNI app in iova=va mode, mempool populate needs to ensure mbuf would not be allocated from a page boundary. IMO after having config option/cmd line parameter to enable KNI iova=va mode, existing KNI will be intact and these mempool API only be called upon request. I will document these details in KNI document. Based on earlier comments and discussions with Olivier and Andrew on the mempool populate patch, we arrived at these solution.

Hi Olivier, 
Please let us know your thoughts on these patch. I am open to any of your suggestions/solution to fix mempool populate issue for KNI iova=va use case.
 
> 
> >
> >>
> >>
> >> 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.
> 
> I didn't like the idea of having a flag in mempool library, but perhaps we can
> have it in KNI scope.

Vamsi> I failed to understand KNI scope here, IMO, though we have flag in KNI scope, it needs to be affected in mempool lib right finally. 

> 
> >
> > 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.
> 
> +1
> 
> > 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.
> 
> +1
> 
> > 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...
> >>
> >