[dpdk-dev] [PATCH v9 1/3] mempool: support external mempool operations

Hunt, David david.hunt at intel.com
Mon Jun 13 15:46:51 CEST 2016


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.



More information about the dev mailing list