Re: [dpdk-dev] [PATCH v3 0/2] Dynamically configure mempool handle

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

-----Original Message-----
> Date: Tue, 5 Sep 2017 09:47:26 +0200
> From: Olivier MATZ <olivier.matz@6wind.com>
> To: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
> CC: Santosh Shukla <santosh.shukla@caviumnetworks.com>, dev@dpdk.org,
>  thomas@monjalon.net, jerin.jacob@caviumnetworks.com,
>  hemant.agrawal@nxp.com
> Subject: Re: [dpdk-dev] [PATCH v3 0/2] Dynamically configure mempool handle
> User-Agent: NeoMutt/20170113 (1.7.2)
> 
> On Mon, Sep 04, 2017 at 03:24:38PM +0100, Sergio Gonzalez Monroy wrote:
> > Hi Olivier,
> > 
> > On 04/09/2017 14:34, Olivier MATZ wrote:
> > > Hi Sergio,
> > > 
> > > On Mon, Sep 04, 2017 at 10:41:56AM +0100, Sergio Gonzalez Monroy wrote:
> > > > On 15/08/2017 09:07, Santosh Shukla wrote:
> > > > > * Application programming sequencing would be
> > > > >       char pref_mempool[RTE_MEMPOOL_OPS_NAMESIZE];
> > > > >       rte_eth_dev_get_preferred_pool_ops(ethdev_port_id, pref_mempool /* out */);
> > > > >       rte_mempool_create_empty();
> > > > >       rte_mempool_set_ops_byname( , pref_memppol, );
> > > > >       rte_mempool_populate_default();
> > > > What about introducing an API like:
> > > > rte_pktmbuf_poll_create_with_ops (..., ops_name, config_pool);
> > > > 
> > > > I think that API would help for the case the application wants an mbuf pool
> > > > with ie. stack handler.
> > > > Sure we can do the empty/set_ops/populate sequence, but the only thing we
> > > > want to change from default pktmbuf_pool_create API is the pool handler.
> > > > 
> > > > Application just needs to decide the ops handler to use, either default or
> > > > one suggested by PMD?
> > > > 
> > > > I think ideally we would have similar APIs:
> > > > - rte_mempool_create_with_ops (...)
> > > > - rte_memppol_xmem_create_with_ops (...)
> > > Today, we may only want to change the mempool handler, but if we
> > > need to change something else tomorrow, we would need to add another
> > > parameter again, breaking the ABI.
> > > 
> > > If we pass a config structure, adding a new field in it would also break the
> > > ABI, except if the structure is opaque, with accessors. These accessors would be
> > > functions (ex: mempool_cfg_new, mempool_cfg_set_pool_ops, ...). This is not so
> > > much different than what we have now.
> > > 
> > > The advantage I can see of working on a config structure instead of directly on
> > > a mempool is that the api can be reused to build a default config.
> > > 
> > > That said, I think it's quite orthogonal to this patch since we still require
> > > the ethdev api.
> > 
> > Fair enough.
> > 
> > Just to double check that we are on the same page:
> > - rte_mempool_create is just there for backwards compatibility and a
> > sequence of create_empty -> set_ops (optional) ->init -> populate_default ->
> > obj_iter (optional) is the recommended way of creating mempools.
> 
> Yes, I think rte_mempool_create() has too many arguments.
> 
> > - if application wants to use rte_mempool_xmem_create with different pool
> > handler needs to replicate function and add set_ops step.
> 
> Yes. And now that xen support is going to be removed, I'm wondering if
> this function is still required, given the API is not that easy to
> use. Calling rte_mempool_populate_phys() several times looks more
> flexible. But this is also another topic.
> 
> > Now if rte_pktmbuf_pool_create is still the preferred mechanism for
> > applications to create mbuf pools, wouldn't it make sense to offer the
> > option of either pass the ops_name as suggested before or for the
> > application to just set a different pool handler? I understand the it is
> > either breaking API or introducing a new API, but is the solution to
> > basically "copy" the whole function in the application and add an optional
> > step (set_ops)?
> 
> I was quite reticent about introducing
> rte_pktmbuf_pool_create_with_ops() because for the same reasons, we
> would also want to introduce functions to create a mempool that use a
> different pktmbuf_init() or pool_init() callback, or to create the pool
> in external memory, ... and we would end up with a functions with too
> many arguments.
> 
> I like the approach of having several simple functions, because it's
> easier to read (even if the code is longer), and it's easily extensible.
> 
> Now if we feel that the mempool ops is more important than the other
> parameters, we can consider to add it in rte_pktmbuf_pool_create().

Yes. I agree with Sergio here.

Something we could target for v18.02.