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.
