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

[Thomas Monjalon <thomas.monjalon@6wind.com>]

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@6wind.com>
> Signed-off-by: David Hunt <david.hunt@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.

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

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

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

> +/**
> + * Register mempool operations
> + *
> + * @param h
> + *   Pointer to and ops structure to register

The parameter name and its description are not correct.

> + * @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

> --- 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?

>  	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.