Re: [PATCH v7 1/5] mempool: support external mempool operations

[Olivier MATZ <olivier.matz@6wind.com>]

Hi,

Some comments below.

On 06/02/2016 03:27 PM, David Hunt wrote:
> --- a/lib/librte_mempool/rte_mempool.h
> +++ b/lib/librte_mempool/rte_mempool.h
> +/**
> + * 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 mostlikely point to a different type of data structure, and
> + * will be transparent to the application programmer.
> + * The function should also not touch the given *mp instance.
> + */
> +typedef void *(*rte_mempool_alloc_t)(const struct rte_mempool *mp);

nit: about doxygen comments, it's better to have a one-line description,
then a blank line, then the full description. While there, please also
check the uppercase at the beginning and the dot when relevant.


> +/**
> + * Structure storing the table of registered ops structs, each of which contain
> + * the function pointers for the mempool ops functions.
> + * Each process has it's own storage for this ops struct aray so that

it's -> its
aray -> array


> + * the mempools can be shared across primary and secondary processes.
> + * The indices used to access the array are valid across processes, whereas
> + * any function pointers stored directly in the mempool struct would not be.
> + * This results in us simply having "ops_index" in the mempool struct.
> + */
> +struct rte_mempool_ops_table {
> +	rte_spinlock_t sl;     /**< Spinlock for add/delete. */
> +	uint32_t num_ops;      /**< Number of used ops structs in the table. */
> +	/**
> +	 * Storage for all possible ops structs.
> +	 */
> +	struct rte_mempool_ops ops[RTE_MEMPOOL_MAX_OPS_IDX];
> +} __rte_cache_aligned;
> +
> +/** Array of registered ops structs */
> +extern struct rte_mempool_ops_table rte_mempool_ops_table;
> +
> +/**
> + * @internal Get the mempool ops struct from its index.
> + *
> + * @param ops_index
> + *   The index of the ops struct in the ops struct table. It must be a valid
> + *   index: (0 <= idx < num_ops).
> + * @return
> + *   The pointer to the ops struct in the table.
> + */
> +static inline struct rte_mempool_ops *
> +rte_mempool_ops_get(int ops_index)
> +{
> +	return &rte_mempool_ops_table.ops[ops_index];
> +}
> +
> +/**
> + * @internal wrapper for external mempool manager alloc callback.

wrapper for mempool_ops alloc callback
?
(same for other functions)


> @@ -922,7 +1124,7 @@ rte_mempool_put(struct rte_mempool *mp, void *obj)
>   */
>  static inline int __attribute__((always_inline))
>  __mempool_get_bulk(struct rte_mempool *mp, void **obj_table,
> -		   unsigned n, int is_mc)
> +		   unsigned int n, int is_mc)
>  {
>  	int ret;
>  	struct rte_mempool_cache *cache;

Despite "unsigned" is not conform to current checkpatch policy, I
don't think it should be modified in this patch.


> --- /dev/null
> +++ b/lib/librte_mempool/rte_mempool_ops.c

> +
> +#include <rte_mempool.h>
> +
> +/* indirect jump table to support external memory pools */
> +struct rte_mempool_ops_table rte_mempool_ops_table = {
> +	.sl =  RTE_SPINLOCK_INITIALIZER ,
> +	.num_ops = 0
> +};
> +
> +/* add a new ops struct in rte_mempool_ops_table, return its index */
> +int
> +rte_mempool_ops_register(const struct rte_mempool_ops *h)

nit: "h" should be "ops" :)


> +{
> +	struct rte_mempool_ops *ops;
> +	int16_t ops_index;
> +
> +	rte_spinlock_lock(&rte_mempool_ops_table.sl);
> +
> +	if (rte_mempool_ops_table.num_ops >=
> +			RTE_MEMPOOL_MAX_OPS_IDX) {
> +		rte_spinlock_unlock(&rte_mempool_ops_table.sl);
> +		RTE_LOG(ERR, MEMPOOL,
> +			"Maximum number of mempool ops structs exceeded\n");
> +		return -ENOSPC;
> +	}
> +
> +	if (h->put == NULL || h->get == NULL || h->get_count == NULL) {
> +		rte_spinlock_unlock(&rte_mempool_ops_table.sl);
> +		RTE_LOG(ERR, MEMPOOL,
> +			"Missing callback while registering mempool ops\n");
> +		return -EINVAL;
> +	}
> +
> +	ops_index = rte_mempool_ops_table.num_ops++;
> +	ops = &rte_mempool_ops_table.ops[ops_index];
> +	snprintf(ops->name, sizeof(ops->name), "%s", h->name);

I think we should check for truncation here, as it was done in:
85cf00791cca ("mem: avoid memzone/mempool/ring name truncation")
(this should be done before the num_ops++)


Regards,
Olivier