[dpdk-dev] [PATCH v14 1/3] mempool: support mempool handler operations

Jan Viktorin viktorin at rehivetech.com
Fri Jun 17 16:35:51 CEST 2016


Hi David,

still few nits... Do you like the upstreaming process? :) I hope finish this
patchset soon. The major issues seem to be OK.

[...]

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

What about to rename the ops_get to get_ops to not confuse
with the ops wrappers? The thread on this topic has not
been finished. I think, we can live with this name, it's
just a bit weird...

Olivier? Thomas?

> +{
> +	RTE_VERIFY(ops_index < RTE_MEMPOOL_MAX_OPS_IDX);

According to the doc comment:

RTE_VERIFY(ops_index >= 0);

> +
> +	return &rte_mempool_ops_table.ops[ops_index];
> +}

[...]

> +
> +/**
> + * @internal Wrapper for mempool_ops get callback.

This comment is obsolete "get callback" vs. dequeue_bulk.

> + *
> + * @param mp
> + *   Pointer to the memory pool.
> + * @param obj_table
> + *   Pointer to a table of void * pointers (objects).
> + * @param n
> + *   Number of objects to get.
> + * @return
> + *   - 0: Success; got n objects.
> + *   - <0: Error; code of get function.

"get function" seems to be obsolete, too...

> + */
> +static inline int
> +rte_mempool_ops_dequeue_bulk(struct rte_mempool *mp,
> +		void **obj_table, unsigned n)
> +{
> +	struct rte_mempool_ops *ops;
> +
> +	ops = rte_mempool_ops_get(mp->ops_index);
> +	return ops->dequeue(mp, obj_table, n);
> +}
> +
> +/**
> + * @internal wrapper for mempool_ops put callback.

similar: "put callback"

> + *
> + * @param mp
> + *   Pointer to the memory pool.
> + * @param obj_table
> + *   Pointer to a table of void * pointers (objects).
> + * @param n
> + *   Number of objects to put.
> + * @return
> + *   - 0: Success; n objects supplied.
> + *   - <0: Error; code of put function.

similar: "put function"

> + */
> +static inline int
> +rte_mempool_ops_enqueue_bulk(struct rte_mempool *mp, void * const *obj_table,
> +		unsigned n)
> +{
> +	struct rte_mempool_ops *ops;
> +
> +	ops = rte_mempool_ops_get(mp->ops_index);
> +	return ops->enqueue(mp, obj_table, n);
> +}
> +

[...]

> +
> +/* 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->alloc == NULL || h->enqueue == NULL ||
> +			h->dequeue == 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);

The unlock is missing here, isn't?

> +		rte_errno = EEXIST;
> +		return -EEXIST;
> +	}
> +
> +	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);
> +	ops->alloc = h->alloc;
> +	ops->enqueue = h->enqueue;
> +	ops->dequeue = h->dequeue;
> +	ops->get_count = h->get_count;
> +
> +	rte_spinlock_unlock(&rte_mempool_ops_table.sl);
> +
> +	return ops_index;
> +}
> +
> +/* wrapper to allocate an external mempool's private (pool) data. */
> +int
> +rte_mempool_ops_alloc(struct rte_mempool *mp)
> +{
> +	struct rte_mempool_ops *ops;
> +
> +	ops = rte_mempool_ops_get(mp->ops_index);
> +	return ops->alloc(mp);
> +}
> +
> +/* wrapper to free an external pool ops. */
> +void
> +rte_mempool_ops_free(struct rte_mempool *mp)
> +{
> +	struct rte_mempool_ops *ops;
> +
> +	ops = rte_mempool_ops_get(mp->ops_index);

I assume there would never be an rte_mempool_ops_unregister. Otherwise this function can
return NULL and may lead to a NULL pointer exception.

> +	if (ops->free == NULL)
> +		return;
> +	return ops->free(mp);

This return statement here is redundant (void).

> +}
> +
> +/* wrapper to get available objects in an external mempool. */
> +unsigned int
> +rte_mempool_ops_get_count(const struct rte_mempool *mp)

[...]

Regards
Jan


More information about the dev mailing list