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).
>
> >> +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 at RehiveTech.com
System Architect Web: www.RehiveTech.com
RehiveTech
Brno, Czech Republic
