On 17/6/2016 3:35 PM, Jan Viktorin wrote:
> 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?
I'll change it, if you think it's weird.
-rte_mempool_ops_get(int ops_index)
+rte_mempool_get_ops(int ops_index)
>> +{
>> + RTE_VERIFY(ops_index < RTE_MEMPOOL_MAX_OPS_IDX);
> According to the doc comment:
>
> RTE_VERIFY(ops_index >= 0);
Fixed.
- RTE_VERIFY(ops_index < RTE_MEMPOOL_MAX_OPS_IDX);
+ RTE_VERIFY((ops_index >= 0) && (ops_index <
RTE_MEMPOOL_MAX_OPS_IDX));
>> +
>> + return &rte_mempool_ops_table.ops[ops_index];
>> +}
> [...]
>
>> +
>> +/**
>> + * @internal Wrapper for mempool_ops get callback.
> This comment is obsolete "get callback" vs. dequeue_bulk.
Done.
- * @internal Wrapper for mempool_ops get callback.
+ * @internal Wrapper for mempool_ops dequeue 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 get.
>> + * @return
>> + * - 0: Success; got n objects.
>> + * - <0: Error; code of get function.
> "get function" seems to be obsolete, too...
Done.
- * - <0: Error; code of get function.
+ * - <0: Error; code of dequeue function.
>> + */
>> +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"
Done.
- * @internal wrapper for mempool_ops put callback.
+ * @internal wrapper for mempool_ops enqueue 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"
Done.
- * - <0: Error; code of put function.
+ * - <0: Error; code of enqueue 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?
Yes, well spotted. Fixed.
+ rte_spinlock_unlock(&rte_mempool_ops_table.sl);
>> + 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.
I've put in a check for NULL.
>> + if (ops->free == NULL)
>> + return;
>> + return ops->free(mp);
> This return statement here is redundant (void).
Removed.
>> +}
>> +
>> +/* wrapper to get available objects in an external mempool. */
>> +unsigned int
>> +rte_mempool_ops_get_count(const struct rte_mempool *mp)
> [...]
>
> Regards
> Jan
Regards,
David.
