Re: [dpdk-dev] [PATCH v6 0/4] add IOVA = VA support in KNI

* Re: [dpdk-dev] [PATCH v6 0/4] add IOVA = VA support in KNI
@ 2019-07-12 11:37 Jerin Jacob Kollanukkaran
  2019-07-12 12:09 ` Burakov, Anatoly
  0 siblings, 1 reply; 23+ messages in thread
From: Jerin Jacob Kollanukkaran @ 2019-07-12 11:37 UTC (permalink / raw)
  To: Burakov, Anatoly, Ferruh Yigit, Vamsi Krishna Attunuru, dev
  Cc: olivier.matz, arybchenko

> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Friday, July 12, 2019 4:19 PM
> To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Ferruh Yigit
> <ferruh.yigit@intel.com>; Vamsi Krishna Attunuru
> <vattunuru@marvell.com>; dev@dpdk.org
> Cc: olivier.matz@6wind.com; arybchenko@solarflare.com
> Subject: [EXT] Re: [dpdk-dev] [PATCH v6 0/4] add IOVA = VA support in KNI
> On 12-Jul-19 11:26 AM, Jerin Jacob Kollanukkaran wrote:
> >>>> What do you think?
> >>>
> >>> IMO, If possible we can avoid extra indirection of new config. In
> >>> worst case We can add it. How about following to not have new config
> >>>
> >>> 1) Make MEMPOOL_F_NO_PAGE_BOUND  as default
> >>> http://patches.dpdk.org/patch/55277/
> >>> There is absolutely zero overhead of this flag considering the huge
> >>> page size are minimum 2MB. Typically 512MB or 1GB.
> >>> Any one has any objection?
> >>
> >> Pretty much zero overhead in hugepage case, not so in non-hugepage
> case.
> >> It's rare, but since we support it, we have to account for it.
> >
> > That is a fair concern.
> > How about enable the flag in mempool ONLY when
> rte_eal_has_hugepages()
> > In the common layer?
> 
> Perhaps it's better to check page size of the underlying memory, because 4K
> pages are not necessarily no-huge mode - they could also be external
> memory. That's going to be a bit hard because there may not be a way to
> know which memory we're allocating from in advance, aside from simple
> checks like `(rte_eal_has_hugepages() ||
> rte_malloc_heap_socket_is_external(socket_id))` - but maybe those would
> be sufficient.

Yes.

> 
> >
> >> (also, i don't really like the name NO_PAGE_BOUND since in memzone
> >> API there's a "bounded memzone" allocation API, and this flag's name
> >> reads like objects would not be bounded by page size, not that they
> >> won't cross page
> >> boundary)
> >
> > No strong opinion for the name. What name you suggest?
> 
> How about something like MEMPOOL_F_NO_PAGE_SPLIT?

Looks good to me.

In summary, Change wrt existing patch"
- Change NO_PAGE_BOUND to MEMPOOL_F_NO_PAGE_SPLIT
- Set this flag in  rte_pktmbuf_pool_create() when rte_eal_has_hugepages() ||
 rte_malloc_heap_socket_is_external(socket_id))

Olivier, Any objection?
Ref: http://patches.dpdk.org/patch/55277/

> 
> >
> >>
> >>>
> >>> 2) Introduce rte_kni_mempool_create() API in kni lib to abstract the
> >>> Mempool requirement for KNI. This will enable portable KNI
> applications.
> >>
> >> This means that using KNI is not a drop-in replacement for any other
> >> PMD. If maintainers of KNI are OK with this then sure :)
> >
> > The PMD  don’t have any dependency on NO_PAGE_BOUND flag. Right?
> > If KNI app is using rte_kni_mempool_create() to create the mempool, In
> > what case do you see problem with specific PMD?
> 
> I'm not saying the PMD's have a dependency on the flag, i'm saying that the
> same code cannot be used with and without KNI because you need to call a
> separate API for mempool creation if you want to use it with KNI.

Yes. Need to call the introduced API from 19.08. If we not choose above(first) approach.
It can be documented in "API changes" in release notes. I prefer to have the first 
solution if there is no downside.

> For KNI, the underlying memory must abide by certain constraints that are
> not there for other PMD's, so either you fix all memory to these constraints,
> or you lose the ability to reuse the code with other PMD's as is.
> 
> That is, unless i'm grossly misunderstanding what you're suggesting here :)
> 
> >
> >>
> >> --
> >> Thanks,
> >> Anatoly
> 
> 
> --
> Thanks,
> Anatoly

^ permalink raw reply	[flat|nested] 23+ messages in thread