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
