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

[Hunt, David]

Hi Thomas,

On 14/6/2016 1:55 PM, Thomas Monjalon wrote:
> Hi David,
>
> 2016-06-14 10:46, David Hunt:
>> 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>
> Glad to see we are close to have this feature integrated.
>
> I've just looked into few details before pushing.
> One of them are the comments. In mempool they were all ended by a dot.
> Please check the new comments.

Do you mean the rte_mempool struct definition, or all comments? Shall I 
leave the
old comments the way they were before the change, or will I clean up?
If I clean up, I'd suggest I add a separate patch for that.

> The doc/guides/rel_notes/deprecation.rst must be updated to remove
> the deprecation notice in this patch.

Will do. As a separate patch in the set?

> Isn't there some explanations to add in
> doc/guides/prog_guide/mempool_lib.rst?

Yes, I'll adapt some of the cover letter, and add as a separate patch.

> Isn't there a better name than "default" for the default implementation?
> I don't think the filename rte_mempool_default.c is meaningful.

I could call it rte_mempool_ring.c? Since the default handler is ring based?

>> +/**
>> + * Register mempool operations
>> + *
>> + * @param h
>> + *   Pointer to and ops structure to register
> The parameter name and its description are not correct.

Will fix.

>> + * @return
>> + *   - >=0: Success; return the index of the ops struct in the table.
>> + *   - -EINVAL - some missing callbacks while registering ops struct
>> + *   - -ENOSPC - the maximum number of ops structs has been reached
>> + */
>> +int rte_mempool_ops_register(const struct rte_mempool_ops *ops);
> You can check the doc with doxygen:
> 	make doc-api-html

Will do.


>> --- a/lib/librte_mempool/rte_mempool_version.map
>> +++ b/lib/librte_mempool/rte_mempool_version.map
>> @@ -19,6 +19,8 @@ DPDK_2.0 {
>>   DPDK_16.7 {
>>   	global:
>>   
>> +	rte_mempool_ops_table;
>> +
> Why this empty line?

No particular reason. I will remove.

>>   	rte_mempool_check_cookies;
>>   	rte_mempool_obj_iter;
>>   	rte_mempool_mem_iter;
>> @@ -29,6 +31,8 @@ DPDK_16.7 {
>>   	rte_mempool_populate_default;
>>   	rte_mempool_populate_anon;
>>   	rte_mempool_free;
>> +	rte_mempool_set_ops_byname;
>> +	rte_mempool_ops_register;
> Please keep it in alphabetical order.
> It seems the order was not respected before in mempool.

I will fix this also.

Regards,
Dave.