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

[Hunt, David]

On 5/27/2016 11:33 AM, Jerin Jacob wrote:
> 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

Jerin,
      That chunk of code above would be better moved all right. I'd 
suggest moving it to the
rte_mempool_create function, as that's the one that needs the backward 
compatibility.

On the flags issue, each mempool handler can re-interpret the flags as 
needed. Maybe we
could use the upper half of the bits for different handlers, changing 
the meaning of the
bits depending on which handler is being set up. We can then keep the lower
half for bits that are common across all handlers? That way the user can 
just set the bits they
are interested in for that handler. Also, the alloc function has access 
to the flags, so maybe the
handler specific setup could be handled in the alloc function rather 
than adding a new function pointer?

Of course, that won't help if we need to pass in more data, in which 
case we'd probably need an
opaque data pointer somewhere. It would probably be most useful to pass 
it in with the
alloc, which may need the data. Any suggestions?

Regards,
Dave.