[dpdk-dev] [PATCH v6 1/5] mempool: support external handler

[Jan Viktorin]

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