Re: [dpdk-dev] [PATCH v5 1/3] mempool: support external handler

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

On Fri, May 27, 2016 at 10:52:42AM +0100, Hunt, David wrote:
> 
> 
> On 5/24/2016 4:35 PM, Jerin Jacob wrote:
> > On Thu, May 19, 2016 at 02:44:59PM +0100, David Hunt wrote:
> > > +	/*
> > > +	 * Since we have 4 combinations of the SP/SC/MP/MC examine the flags to
> > > +	 * set the correct index into the handler table.
> > > +	 */
> > > +	if (flags & (MEMPOOL_F_SP_PUT | MEMPOOL_F_SC_GET))
> > > +		rte_mempool_set_handler(mp, "ring_sp_sc");
> > > +	else if (flags & MEMPOOL_F_SP_PUT)
> > > +		rte_mempool_set_handler(mp, "ring_sp_mc");
> > > +	else if (flags & MEMPOOL_F_SC_GET)
> > > +		rte_mempool_set_handler(mp, "ring_mp_sc");
> > > +	else
> > > +		rte_mempool_set_handler(mp, "ring_mp_mc");
> > IMO, We should decouple the implementation specific flags of _a_
> > external pool manager implementation from the generic rte_mempool_create_empty
> > function as going further when we introduce new flags for custom HW accelerated
> > external pool manager then this common code will be bloated.
> 
> These flags are only there to maintain backward compatibility for the
> default handlers. I would not
> envisage adding more flags to this, I would suggest just adding a new
> handler using the new API calls.
> So I would not see this code growing much in the future.

IMHO, For _each_ HW accelerated external pool manager we may need to introduce
specific flag to tune to specific use cases.i.e MEMPOOL_F_* flags for
this exiting pool manager implemented in SW. For instance, when we add
a new HW external pool manager we may need to add MEMPOOL_MYHW_DONT_FREE_ON_SEND
(just a random name) to achieve certain functionally.

So I propose let "unsigned flags" in mempool create to be the opaque type and each
external pool manager can define what it makes sense to that specific
pool manager as there is NO other means to configure the pool manager.

For instance, on HW accelerated pool manager, the flag MEMPOOL_F_SP_PUT may
not make much sense as it can work with MP without any additional
settings in HW.

So instead of adding these checks in common code, IMO, lets move this
to a pool manager specific "function pointer" function and invoke
the function pointer from generic mempool create function.

What do you think?

Jerin