Re: [PATCH v7 1/5] mempool: support external mempool operations

[Olivier MATZ <olivier.matz@6wind.com>]

On 06/03/2016 12:28 PM, Hunt, David wrote:
> On 6/3/2016 7:38 AM, Jerin Jacob wrote:
>> On Thu, Jun 02, 2016 at 02:27:19PM +0100, David Hunt wrote:
>>> +/**
>>> + * @internal wrapper for external mempool manager 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.
>>> + */
>>> +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->put(mp->pool_data, obj_table, n);
>> Pass by value of "pool_data", On 32 bit systems, casting back to
>> pool_id will
>> be an issue as void* on 32 bit is 4B. IMO, May be can use uint64_t to
>> pass by value and typecast to void* to fix it.
> 
> OK. I see the problem. I'll see 4 callbacks that need to change, free,
> get, put and get_count.
> So the callbacks will be:
> typedef void *(*rte_mempool_alloc_t)(struct rte_mempool *mp);
> typedef void (*rte_mempool_free_t)(uint64_t p);
> typedef int (*rte_mempool_put_t)(uint64_t p, void * const *obj_table,
> unsigned int n);
> typedef int (*rte_mempool_get_t)(uint64_t p, void **obj_table, unsigned
> int n);
> typedef unsigned (*rte_mempool_get_count)(uint64_t p);

I don't quite like the uint64_t argument (I exepect that most handlers
will use a pointer, and they will have to do a cast). What about giving
a (struct rte_mempool *) instead? The handler function would then
select between void * or uint64_t without a cast.
In that case, maybe the prototype of alloc should be:

  typedef int (*rte_mempool_alloc_t)(struct rte_mempool *mp);

It would directly set mp->pool_data and return 0 or -errno.

By the way, I found a strange thing:

> typedef void (*rte_mempool_free_t)(void *p);

[...]

> void
> rte_mempool_ops_free(struct rte_mempool *mp)
> {
> struct rte_mempool_ops *ops;
> 
> ops = rte_mempool_ops_get(mp->ops_index);
> if (ops->free == NULL)
> return;
> return ops->free(mp);
> }
> 

Seems that the free cb expects mp->pool_data, but mp is passed.



Another alternative to the "uint64_t or ptr" question would be to use
a uintptr_t instead of a uint64_t. This is won't be possible if it need
to be a 64 bits value even on 32 bits architectures. We could then keep
the argument as pointer, and cast it to uintptr_t if needed.

Regards,
Olivier