Re: [PATCH v14 1/3] mempool: support mempool handler operations

["Hunt, David" <david.hunt@intel.com>]

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.