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

[Olivier Matz]

Hi David,

Please find some comments below.

On 06/03/2016 04:58 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.
> + */
> +typedef void *(*rte_mempool_alloc_t)(struct rte_mempool *mp);

In http://dpdk.org/ml/archives/dev/2016-June/040233.html, I suggested
to change the prototype to return an int (-errno) and directly set
the set mp->pool_data (void *) or mp->pool_if (uint64_t). No cast
would be required in this latter case.

By the way, there is a typo in the comment:
"mostlikely" -> "most likely"

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

> +static void
> +common_ring_free(struct rte_mempool *mp)
> +{
> +	rte_ring_free((struct rte_ring *)mp->pool_data);
> +}

I don't think the cast is needed here.
(same in other functions)


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

> +/* add a new ops struct in rte_mempool_ops_table, return its index */
> +int
> +rte_mempool_ops_register(const struct rte_mempool_ops *h)
> +{
> +	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;
> +	}
> +
> +	if (strlen(h->name) >= sizeof(ops->name) - 1) {
> +		RTE_LOG(DEBUG, EAL, "%s(): mempool_ops <%s>: name too long\n",
> +				__func__, h->name);
> +		rte_errno = EEXIST;
> +		return NULL;
> +	}

This has already been noticed by Shreyansh, but in case of:

rte_mempool_ops.c:75:10: error: return makes integer from pointer
without a cast [-Werror=int-conversion]
   return NULL;
          ^


> +/* sets mempool ops previously registered by rte_mempool_ops_register */
> +int
> +rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name)


When I compile with shared libraries enabled, I get the following error:

librte_reorder.so: undefined reference to `rte_mempool_ops_table'
librte_mbuf.so: undefined reference to `rte_mempool_set_ops_byname'
...

The new functions and global variables must be in
rte_mempool_version.map. This was in v5
( http://dpdk.org/ml/archives/dev/2016-May/039365.html ) but
was dropped in v6.




Regards,
Olivier