Hi,
Some comments below.
On 06/02/2016 03:27 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.
> + * The function should also not touch the given *mp instance.
> + */
> +typedef void *(*rte_mempool_alloc_t)(const struct rte_mempool *mp);
nit: about doxygen comments, it's better to have a one-line description,
then a blank line, then the full description. While there, please also
check the uppercase at the beginning and the dot when relevant.
> +/**
> + * Structure storing the table of registered ops structs, each of which
> contain
> + * the function pointers for the mempool ops functions.
> + * Each process has it's own storage for this ops struct aray so that
it's -> its
aray -> array
> + * the mempools can be shared across primary and secondary processes.
> + * The indices used to access the array are valid across processes, whereas
> + * any function pointers stored directly in the mempool struct would not be.
> + * This results in us simply having "ops_index" in the mempool struct.
> + */
> +struct rte_mempool_ops_table {
> + rte_spinlock_t sl; /**< Spinlock for add/delete. */
> + uint32_t num_ops; /**< Number of used ops structs in the table. */
> + /**
> + * Storage for all possible ops structs.
> + */
> + struct rte_mempool_ops ops[RTE_MEMPOOL_MAX_OPS_IDX];
> +} __rte_cache_aligned;
> +
> +/** Array of registered ops structs */
> +extern struct rte_mempool_ops_table rte_mempool_ops_table;
> +
> +/**
> + * @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)
> +{
> + return &rte_mempool_ops_table.ops[ops_index];
> +}
> +
> +/**
> + * @internal wrapper for external mempool manager alloc callback.
wrapper for mempool_ops alloc callback
?
(same for other functions)
> @@ -922,7 +1124,7 @@ rte_mempool_put(struct rte_mempool *mp, void *obj)
> */
> static inline int __attribute__((always_inline))
> __mempool_get_bulk(struct rte_mempool *mp, void **obj_table,
> - unsigned n, int is_mc)
> + unsigned int n, int is_mc)
> {
> int ret;
> struct rte_mempool_cache *cache;
Despite "unsigned" is not conform to current checkpatch policy, I
don't think it should be modified in this patch.
> --- /dev/null
> +++ b/lib/librte_mempool/rte_mempool_ops.c
> +
> +#include <rte_mempool.h>
> +
> +/* indirect jump table to support external memory pools */
> +struct rte_mempool_ops_table rte_mempool_ops_table = {
> + .sl = RTE_SPINLOCK_INITIALIZER ,
> + .num_ops = 0
> +};
> +
> +/* add a new ops struct in rte_mempool_ops_table, return its index */
> +int
> +rte_mempool_ops_register(const struct rte_mempool_ops *h)
nit: "h" should be "ops" :)
> +{
> + 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;
> + }
> +
> + 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);
I think we should check for truncation here, as it was done in:
85cf00791cca ("mem: avoid memzone/mempool/ring name truncation")
(this should be done before the num_ops++)
Regards,
Olivier
