Hi Olivier, On 13/6/2016 1:16 PM, Olivier Matz wrote: > Hi David, > > Some comments below. > > On 06/10/2016 05:16 PM, David Hunt wrote: >> Until now, the objects stored in a mempool were internally stored in a >> ring. This patch introduces the possibility to register external handlers >> replacing the ring. >> >> The default behavior remains unchanged, but calling the new function >> rte_mempool_set_handler() right after rte_mempool_create_empty() allows >> the user to change the handler that will be used when populating >> the mempool. >> >> This patch also adds a set of default ops (function callbacks) based >> on rte_ring. >> >> Signed-off-by: Olivier Matz <olivier.matz at 6wind.com> >> Signed-off-by: David Hunt <david.hunt at intel.com> >> >> ... >> @@ -386,10 +352,14 @@ rte_mempool_populate_phys(struct rte_mempool *mp, char >> *vaddr, >> 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; >> + if ((mp->flags & MEMPOOL_F_POOL_CREATED) == 0) { >> + rte_errno = 0; >> + ret = rte_mempool_ops_alloc(mp); >> + if (ret != 0) { >> + if (rte_errno == 0) >> + return -EINVAL; >> + return -rte_errno; >> + } >> } > The rte_errno should be removed. Just return the error code from > rte_mempool_ops_alloc() on failure.
Done. >> +/** Structure defining mempool operations structure */ >> +struct rte_mempool_ops { >> + char name[RTE_MEMPOOL_OPS_NAMESIZE]; /**< Name of mempool ops struct */ >> + rte_mempool_alloc_t alloc; /**< Allocate private data */ >> + rte_mempool_free_t free; /**< Free the external pool. */ >> + rte_mempool_put_t put; /**< Put an object. */ >> + rte_mempool_get_t get; /**< Get an object. */ >> + rte_mempool_get_count get_count; /**< Get qty of available objs. */ >> +} __rte_cache_aligned; >> + > Sorry, I missed that in the previous reviews, but since the get/put > functions have been renamed in dequeue/enqueue, I think the same change > should also be done here. Done. I also changed the common_ring_[sc|mc]_put and common_ring_[sc|mc]_get. I didn't go as far as changing the rte_mempool_put or rte_mempool_put_bulk calls, as they are used in several drivers and apps. > >> +/** >> + * Prototype for implementation specific data provisioning function. >> + * >> + * The function should provide the implementation specific memory for >> + * for use by the other mempool ops functions in a given mempool ops struct. >> + * E.g. the default ops provides an instance of the rte_ring for this >> purpose. >> + * it will most likely point to a different type of data structure, and >> + * will be transparent to the application programmer. >> + */ >> +typedef int (*rte_mempool_alloc_t)(struct rte_mempool *mp); > A comment saying that this function should set mp->pool_data > would be nice here, I think. Done >> +/* wrapper to allocate an external mempool's private (pool) data */ >> +int >> +rte_mempool_ops_alloc(struct rte_mempool *mp) >> +{ >> + struct rte_mempool_ops *ops; >> + >> + ops = rte_mempool_ops_get(mp->ops_index); >> + if (ops->alloc == NULL) >> + return -ENOMEM; >> + return ops->alloc(mp); >> +} > Now that we check that ops->alloc != NULL in the register function, > I wonder if we should keep this test or not. Yes, it doesn't hurt, > but for consistency with the other functions (get/put/get_count), > we may remove it. > > This would be a good thing because it would prevent any confusion > with rte_mempool_ops_get(), which returns a pointer to the ops struct > (and has nothing to do with ops->get()). Done. > > Regards, > Olivier Thanks, Dave.