Re: [dpdk-dev] [PATCH 1/5] mempool: add external mempool manager support

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

On Wed, Feb 03, 2016 at 02:16:06PM +0000, Hunt, David wrote:
> On 28/01/2016 17:52, Jerin Jacob wrote:
> >On Tue, Jan 26, 2016 at 05:25:51PM +0000, David Hunt wrote:
> >>Adds the new rte_mempool_create_ext api and callback mechanism for
> >>external mempool handlers
> >>
> >>Modifies the existing rte_mempool_create to set up the handler_idx to
> >>the relevant mempool handler based on the handler name:
> >>	ring_sp_sc
> >>	ring_mp_mc
> >>	ring_sp_mc
> >>	ring_mp_sc
> >>
> >>Signed-off-by: David Hunt <david.hunt@intel.com>
> >>---

[snip]

> >>+	if (flags & MEMPOOL_F_USE_STACK)
> >>+		mp->handler_idx = rte_get_mempool_handler("stack");
> >>+	else if (flags & (MEMPOOL_F_SP_PUT | MEMPOOL_F_SC_GET))
> >>+		mp->handler_idx = rte_get_mempool_handler("ring_sp_sc");
> >>+	else if (flags & MEMPOOL_F_SP_PUT)
> >>+		mp->handler_idx = rte_get_mempool_handler("ring_sp_mc");
> >>+	else if (flags & MEMPOOL_F_SC_GET)
> >>+		mp->handler_idx = rte_get_mempool_handler("ring_mp_sc");
> >>+	else
> >>+		mp->handler_idx = rte_get_mempool_handler("ring_mp_mc");
> >>+
> >
> >Why still use flag based selection? Why not "name" based? See below
> >for more description
> 
> 
> The old API does not have a 'name' parameter, so needs to work out which
> handler to use based on the flags. This is not necessary in the new API
> call, and so it uses the "name" based index.
> 
I agree. But the old API to new API mapping still can be done like
below,
rte_mempool_create -> rte_mempool_create_ext(..,"ring_mp_mc")

[snip]

> 
> >>+	/* init the mempool structure */
> >>+	mp = mz->addr;
> >>+	memset(mp, 0, sizeof(*mp));
> >>+	snprintf(mp->name, sizeof(mp->name), "%s", name);
> >>+	mp->phys_addr = mz->phys_addr;
> >>+	mp->size = n;
> >>+	mp->flags = flags;
> >>+	mp->cache_size = cache_size;
> >>+	mp->cache_flushthresh = CALC_CACHE_FLUSHTHRESH(cache_size);
> >>+	mp->private_data_size = private_data_size;
> >>+	mp->handler_idx = handler_idx;
> >>+	mp->elt_size = elt_size;
> >>+	mp->rt_pool = rte_mempool_ext_alloc(mp, name, n, socket_id, flags);
> >
> >
> >IMO, We can avoid the duplicaition of above code with rte_mempool_create.
> >i.e  rte_mempool_create -> rte_mempool_create_ext(..,"ring_mp_mc")
> 
> 
> rte_mempool_create is not really a subset of rte_mempool_create_ext, so
> doing this would not be possible. I did have a look at this before pusing
> the patch, but the code was so different in each case I decided to leave
> them as is. Maybe break out the section that sets up the mempool structure
> in to a separate functinon?

Yes, Their are a lot of common code between 
rte_mempool_create/rte_mempool_xmem_create and rte_mempool_create_ext.

IMO, we need to converge these functions. Otherwise, a new feature in
mempool would call for changing in both places and it's difficult to
maintain.
In my view, both external and internal pool manager should have the same
code for creation and just the backend put/get/alloc function
pointers will be different to maintain the functional consistency.

Thanks, comments appreciated.
Jerin