Hello,
sorry for top-posting. I vote for passing rte_mempool instead of void *. This
is what I've already pointed at, a kind of type-safety...? Passing uint64_t
just hides the problem. Another way is to name the union and pass it as eg.
union rte_mempool_priv * to the callbacks.
Jan?Viktorin
RehiveTech
Sent?from?a?mobile?device
? P?vodn? zpr?va ?
Od: Olivier MATZ
Odesl?no: p?tek, 3. ?ervna 2016 13:08
Komu: Hunt, David; Jerin Jacob
Kopie: dev at dpdk.org; viktorin at rehivetech.com
P?edm?t: Re: [PATCH v7 1/5] mempool: support external mempool operations
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
