On Thu, 2 Jun 2016 12:23:41 +0100
"Hunt, David" <david.hunt at intel.com> wrote:
> On 6/1/2016 6:54 PM, Jan Viktorin wrote:
> >
> > mp->populated_size--;
> > @@ -383,13 +349,16 @@ rte_mempool_populate_phys(struct rte_mempool *mp,
> > char *vaddr,
> > unsigned i = 0;
> > size_t off;
> > struct rte_mempool_memhdr *memhdr;
> > - int ret;
> >
> > /* create the internal ring if not already done */
> > if ((mp->flags & MEMPOOL_F_RING_CREATED) == 0) {
> > - ret = rte_mempool_ring_create(mp);
> > - if (ret < 0)
> > - return ret;
> > + rte_errno = 0;
> > + mp->pool = rte_mempool_ops_alloc(mp);
> > + if (mp->pool == NULL) {
> > + if (rte_errno == 0)
> > + return -EINVAL;
> > + return -rte_errno;
> > Are you sure the rte_errno is a positive value?
>
> If the user returns EINVAL, or similar, we want to return negative, as
> per the rest of DPDK.
Oh, yes... you're right.
>
> >> @@ -204,9 +205,13 @@ struct rte_mempool_memhdr {
> >> struct rte_mempool {
> >> char name[RTE_MEMPOOL_NAMESIZE]; /**< Name of mempool. */
> >> struct rte_ring *ring; /**< Ring to store objects. */
> >> + union {
> >> + void *pool; /**< Ring or pool to store objects */
> > What about calling this pdata or priv? I think, it can improve some doc
> > comments.
> > Describing a "pool" may refer to both the rte_mempool itself or to the
> > mp->pool
> > pointer. The "priv" would help to understand the code a bit.
> >
> > Then the rte_mempool_alloc_t can be called rte_mempool_priv_alloc_t. Or
> > something
> > similar. It's than clear enough, what the function should do...
>
> I'd lean towards pdata or maybe pool_data. Not sure about the function
> name change though,
> the function does return a pool_data pointer, which we put into
> mp->pool_data.
Yes. But from the name of the function, it is difficult to understand what is
its
purpose.
>
> >> + uint64_t *pool_id; /**< External mempool identifier */
> > Why is pool_id a pointer? Is it a typo? I've understood it should be 64b
> > wide
> > from the discussion (Olivier, Jerin, David):
>
[...]
>
> >> +typedef void (*rte_mempool_free_t)(void *p);
> >> +
> >> +/**
> >> + * Put an object in the external pool.
> >> + * The *p pointer is the opaque data for a given mempool handler (ring,
> >> + * array, linked list, etc)
> > The obj_table is not documented. Is it really a table? I'd called an array
> > instead.
>
> You're probably right, but it's always been called obj_table, and I'm
> not sure
> this patchset is the place to change it. Maybe a follow up patch?
In that case, it's OK.
>
> >> + */
> >> +typedef int (*rte_mempool_put_t)(void *p, void * const *obj_table,
> >> unsigned n);
> > unsigned int
> Done
> >
> >> +
> >> +/** Get an object from the external pool. */
> >> +typedef int (*rte_mempool_get_t)(void *p, void **obj_table, unsigned n);
> > unsigned int
> Done
> >
> >> +
> >> +/** Return the number of available objects in the external pool. */
> > Is the number of available objects or the total number of all objects
> > (so probably a constant value)?
>
> It's intended to be the umber of available objects
OK, forget it.
>
> >
> >> +typedef unsigned (*rte_mempool_get_count)(void *p);
> > Should it be const void *p?
>
> Yes, it could be. Changed.
>
> >
[...]
>
> >> + * @return
> >> + * - 0: Sucess; the new handler is configured.
> >> + * - EINVAL - Invalid handler name provided
> >> + * - EEXIST - mempool already has a handler assigned
> > They are returned as -EINVAL and -EEXIST.
> >
> > IMHO, using "-" here is counter-intuitive:
> >
> > - EINVAL
> >
> > does it mean a positive with "-" or negative value?
>
> EINVAL is positive, so it's returning negative. Common usage in DPDK,
> afaics.
Yes, of course. But it is not so clear from the doc. I've already wrote
a code checking the positive error codes. That was because of no "minus
sign" in the doc. So, my code was wrong and it took me a while to find
out the reason... I supposed, the positive value was intentional. Finally,
I had to lookup the source code (the calling path) to verify...
>
>
> >> + */
> >> +int
> >> +rte_mempool_set_handler(struct rte_mempool *mp, const char *name);
> > rte_mempool_set_ops
> >
> > What about rte_mempool_set_ops_byname? Not a big deal...
>
> I agree. rte_mempool_set_ops_byname
>
> >> +
> >> +/**
> >> + * Register an external pool handler.
> > Register mempool operations
>
> Done
>
> >> + *
> >> + * @param h
> >> + * Pointer to the external pool handler
> >> + * @return
> >> + * - >=0: Sucess; return the index of the handler in the table.
> >> + * - EINVAL - missing callbacks while registering handler
> >> + * - ENOSPC - the maximum number of handlers has been reached
> > - -EINVAL
> > - -ENOSPC
>
> :)
Similar as above... If it's a DPDK standard to write it like this, then I am
OK with that.
>
[...]
--
Jan Viktorin E-mail: Viktorin at RehiveTech.com
System Architect Web: www.RehiveTech.com
RehiveTech
Brno, Czech Republic
