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

[Jan Viktorin <viktorin@rehivetech.com>]

On Tue, 31 May 2016 10:09:42 +0100
"Hunt, David" <david.hunt@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).

> 
> >> +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...

> 
> 
> >> +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?

[...]

> >> +
> >> +/** 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.

> 

[...]

> >> +/**
> >> + * 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:

116 /* set the handler of a mempool */
117 int
118 rte_mempool_set_handler(struct rte_mempool *mp, const char *name)
119 {
120         struct rte_mempool_handler *handler = NULL;
121         unsigned i;
122
123         /* too late, the mempool is already populated */
124         if (mp->flags & MEMPOOL_F_RING_CREATED)
125                 return -EEXIST;

Here, it returns -EEXIST.

126
127         for (i = 0; i < rte_mempool_handler_table.num_handlers; i++) {
128                 if (!strcmp(name, rte_mempool_handler_table.handler[i].name)) {
129                         handler = &rte_mempool_handler_table.handler[i];
130                         break;
131                 }
132         }
133
134         if (handler == NULL)
135                 return -EINVAL;

And here, it returns -EINVAL.

136
137         mp->handler_idx = i;
138         return 0;
139 }

So, it is possible to define those in the doc comment.

> 
> 
> >> + */
> >> +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.

>
 
[...]

> >>   
> >> +
> >> +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.

> 
> 
> >> +#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.

Jan

> 
> Regard,
> Dave.
> 
> 



-- 
   Jan Viktorin                  E-mail: Viktorin@RehiveTech.com
   System Architect              Web:    www.RehiveTech.com
   RehiveTech
   Brno, Czech Republic