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

[Hunt, David]

On 5/31/2016 1:06 PM, Jan Viktorin wrote:
> On Tue, 31 May 2016 10:09:42 +0100
> "Hunt, David" <david.hunt at intel.com> wrote:
>
>> Hi Jan,
>>
> [...]
>
>>>> +typedef void *(*rte_mempool_alloc_t)(struct rte_mempool *mp);
>>>> +
>>>> +/** Free the external pool. */
>>> What is the purpose of this callback?
>>> What exactly does it allocate?
>>> Some rte_mempool internals?
>>> Or the memory?
>>> What does it return?
>> This is the main allocate function of the handler. It is up to the
>> mempool handlers control.
>> The handler's alloc function does whatever it needs to do to grab memory
>> for this handler, and places
>> a pointer to it in the *pool opaque pointer in the rte_mempool struct.
>> In the default handler, *pool
>> points to a ring, in other handlers, it will mostlikely point to a
>> different type of data structure. It will
>> be transparent to the application programmer.
> Thanks for explanation. Please, add doc comments there.
>
> Should the *mp be const here? It is not clear to me whether the callback is
> expected to modify the mempool or not (eg. set the pool pointer).

Comment added. Not const, as the *pool is set.

>>>> +typedef void (*rte_mempool_free_t)(void *p);
>>>> +
>>>> +/** Put an object in the external pool. */
>>> Why this *_free callback does not accept the rte_mempool param?
>>>   
>> We're freeing the pool opaque data here.
> Add doc comments...

Done.

>>
>>>> +typedef int (*rte_mempool_put_t)(void *p, void * const *obj_table, unsigned n);
>>> What is the *p pointer?
>>> What is the obj_table?
>>> Why is it void *?
>>> Why is it const?
>>>   
>> The *p pointer is the opaque data for a given mempool handler (ring,
>> array, linked list, etc)
> Again, doc comments...
>
> I don't like the obj_table representation to be an array of void *. I could see
> it already in DPDK for defining Ethernet driver queues, so, it's probably not
> an issue. I just say, I would prefer some basic type safety like
>
> struct rte_mempool_obj {
> 	void *p;
> };
>
> Is there somebody with different opinions?
>
> [...]

Comments added. I've left as a void* for the moment.

>>>> +
>>>> +/** Structure defining a mempool handler. */
>>> Later in the text, I suggested to rename rte_mempool_handler to rte_mempool_ops.
>>> I believe that it explains the purpose of this struct better. It would improve
>>> consistency in function names (the *_ext_* mark is very strange and inconsistent).
>> I agree. I've gone through all the code and renamed to
>> rte_mempool_handler_ops.
> Ok. I meant rte_mempool_ops because I find the word "handler" to be redundant.

I prefer the use of the word handler, unless others also have opinions 
either way?

>
>>>> +/**
>>>> + * Set the handler of a mempool
>>>> + *
>>>> + * This can only be done on a mempool that is not populated, i.e. just after
>>>> + * a call to rte_mempool_create_empty().
>>>> + *
>>>> + * @param mp
>>>> + *   Pointer to the memory pool.
>>>> + * @param name
>>>> + *   Name of the handler.
>>>> + * @return
>>>> + *   - 0: Sucess; the new handler is configured.
>>>> + *   - <0: Error (errno)
>>> Should this doc be more specific about the possible failures?
>>>
>>> The body of rte_mempool_set_handler does not set errno at all.
>>> It returns e.g. -EEXIST.
>> This is up to the handler. We cannot know what codes will be returned at
>> this stage.
>> errno was a cut-and paste error, this is now fixed.
> I don't think so. The rte_mempool_set_handler is not handler-specific:
[...]
> So, it is possible to define those in the doc comment.

Ah, I see now. My mistake, I assumed a different function. Added now.

>>
>>>> + */
>>>> +int
>>>> +rte_mempool_set_handler(struct rte_mempool *mp, const char *name);
>>>> +
>>>> +/**
>>>> + * Register an external pool handler.
>>>> + *
>>>> + * @param h
>>>> + *   Pointer to the external pool handler
>>>> + * @return
>>>> + *   - >=0: Sucess; return the index of the handler in the table.
>>>> + *   - <0: Error (errno)
>>> Should this doc be more specific about the possible failures?
>> This is up to the handler. We cannot know what codes will be returned at
>> this stage.
>> errno was a cut-and paste error, this is now fixed.
> Same, here... -ENOSPC, -EINVAL are returned in certain cases. And again,
> this call is not handler-specific.

Yes, Added now to comments.

>   
> [...]
>
>>>>    
>>>> +
>>>> +static struct rte_mempool_handler handler_mp_mc = {
>>>> +	.name = "ring_mp_mc",
>>>> +	.alloc = common_ring_alloc,
>>>> +	.free = common_ring_free,
>>>> +	.put = common_ring_mp_put,
>>>> +	.get = common_ring_mc_get,
>>>> +	.get_count = common_ring_get_count,
>>>> +};
>>>> +
>>>> +static struct rte_mempool_handler handler_sp_sc = {
>>>> +	.name = "ring_sp_sc",
>>>> +	.alloc = common_ring_alloc,
>>>> +	.free = common_ring_free,
>>>> +	.put = common_ring_sp_put,
>>>> +	.get = common_ring_sc_get,
>>>> +	.get_count = common_ring_get_count,
>>>> +};
>>>> +
>>>> +static struct rte_mempool_handler handler_mp_sc = {
>>>> +	.name = "ring_mp_sc",
>>>> +	.alloc = common_ring_alloc,
>>>> +	.free = common_ring_free,
>>>> +	.put = common_ring_mp_put,
>>>> +	.get = common_ring_sc_get,
>>>> +	.get_count = common_ring_get_count,
>>>> +};
>>>> +
>>>> +static struct rte_mempool_handler handler_sp_mc = {
>>>> +	.name = "ring_sp_mc",
>>>> +	.alloc = common_ring_alloc,
>>>> +	.free = common_ring_free,
>>>> +	.put = common_ring_sp_put,
>>>> +	.get = common_ring_mc_get,
>>>> +	.get_count = common_ring_get_count,
>>>> +};
>>>> +
>>> Introducing those handlers can go as a separate patch. IMHO, that would simplify
>>> the review process a lot. First introduce the mechanism, then add something
>>> inside.
>>>
>>> I'd also note that those handlers are always available and what kind of memory
>>> do they use...
>> Done. Now added as a separate patch.
>>
>> They don't use any memory unless they are used.
> Yes, that is what I've meant... What is the source of the allocations? Where does
> the common_ring_sc_get get memory from?
>
> Anyway, any documentation describing the goal of those four declarations
> would be helpful.

For these handlers, the allocations are as per the original code before 
this patch. For new handlers,
hardware allocators, stack allocators, etc.

I've added comments on the 4 declarations.

>>
>>>> +#include <stdio.h>
>>>> +#include <string.h>
>>>> +
>>>> +#include <rte_mempool.h>
>>>> +
>>>> +/* indirect jump table to support external memory pools */
>>>> +struct rte_mempool_handler_table rte_mempool_handler_table = {
>>>> +	.sl =  RTE_SPINLOCK_INITIALIZER ,
>>>> +	.num_handlers = 0
>>>> +};
>>>> +
>>>> +/* add a new handler in rte_mempool_handler_table, return its index */
>>> It seems to me that there is no way how to put some opaque pointer into the
>>> handler. In such case I would expect I can do something like:
>>>
>>> struct my_handler {
>>> 	struct rte_mempool_handler h;
>>> 	...
>>> } handler;
>>>
>>> rte_mempool_handler_register(&handler.h);
>>>
>>> But I cannot because you copy the contents of the handler. By the way, this
>>> should be documented.
>>>
>>> How can I pass an opaque pointer here? The only way I see is through the
>>> rte_mempool.pool.
>> I think have addressed this in a later patch, in the discussions with
>> Jerin on the list. But
>> rather than passing data at register time, I pass it at create time (or
>> rather set_handler).
>> And a new config void has been added to the mempool struct for this
>> purpose.
> Ok, sounds promising. It just should be well specified to avoid situations
> when accessing the the pool_config before it is set.
>
>>>    In that case, what about renaming the rte_mempool_handler
>>> to rte_mempool_ops? Because semantically, it is not a handler, it just holds
>>> the operations.
>>>
>>> This would improve some namings:
>>>
>>> rte_mempool_ext_alloc -> rte_mempool_ops_alloc
>>> rte_mempool_ext_free -> rte_mempool_ops_free
>>> rte_mempool_ext_get_count -> rte_mempool_ops_get_count
>>> rte_mempool_handler_register -> rte_mempool_ops_register
>>>
>>> seems to be more readable to me. The *_ext_* mark does not say anything valuable.
>>> It just scares a bit :).
>> Agreed. Makes sense. The ext was intended to be 'external', but that's a
>> bit too generic, and not
>> very intuitive. the 'ops' tag seems better to me. I've change this in
>> the latest patch.
> Again, note that I've suggested to avoid the word _handler_ entirely.
>
> [...]
>
>>> Regards
>>> Jan
>> Thanks, Jan. Very comprehensive.  I'll hopefully be pushing the latest
>> patch to the list later today (Tuesday 31st)
> Cool, please CC me.

Will do.


Rgds,
Dave.