On 6/3/2016 12:07 PM, Olivier MATZ wrote:
>
> 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.
I would tend to agree. The uint64 didn't sit well with me :)
I would prefer the rte_mempool*
> By the way, I found a strange thing:
>
>> typedef void (*rte_mempool_free_t)(void *p);
Yes, I spotted that earler, will be fixed in next version
> [...]
>
>> 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.
Working in it.
>
>
> 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.
I had assumed that the requirement was for 64 bits even on 32 bit OS's.
I've implemented
the double cast of the u64 to uintptr_t to struct pointer done to avoid
compiler warnings on
32-bit but I really prefer the solution of passing the rte_mempool
pointer instead. I'll change to
*mp.
Regards,
Dave.
