[dpdk-dev] [PATCH v12 0/3] mempool: add external mempool manager

[Olivier MATZ]

Hi David,

On 06/15/2016 02:38 PM, Hunt, David wrote:
>
>
> On 15/6/2016 1:03 PM, Olivier MATZ wrote:
>> Hi,
>>
>> On 06/15/2016 01:47 PM, Hunt, David wrote:
>>>
>>>
>>> On 15/6/2016 11:13 AM, Jan Viktorin wrote:
>>>> Hi,
>>>>
>>>> I've got one last question. Initially, I was interested in creating
>>>> my own external memory provider based on a Linux Kernel driver.
>>>> So, I've got an opened file descriptor that points to a device which
>>>> can mmap a memory regions for me.
>>>>
>>>> ...
>>>> int fd = open("/dev/uio0" ...);
>>>> ...
>>>> rte_mempool *pool = rte_mempool_create_empty(...);
>>>> rte_mempool_set_ops_byname(pool, "uio_allocator_ops");
>>>>
>>>> I am not sure how to pass the file descriptor pointer. I thought it
>>>> would
>>>> be possible by the rte_mempool_alloc but it's not... Is it possible
>>>> to solve this case?
>>>>
>>>> The allocator is device-specific.
>>>>
>>>> Regards
>>>> Jan
>>>
>>> This particular use case is not covered.
>>>
>>> We did discuss this before, and an opaque pointer was proposed, but did
>>> not make it in.
>>> http://article.gmane.org/gmane.comp.networking.dpdk.devel/39821
>>> (and following emails in that thread)
>>>
>>> So, the options for this use case are as follows:
>>> 1. Use the pool_data to pass data in to the alloc, then set the
>>> pool_data pointer before coming back from alloc. (It's a bit of a hack,
>>> but means no code change).
>>> 2. Add an extra parameter to the alloc function. The simplest way I can
>>> think of doing this is to
>>> take the *opaque passed into rte_mempool_populate_phys, and pass it on
>>> into the alloc function.
>>> This will have minimal impact on the public API,s as there is already an
>>> opaque there in the _populate_ funcs, we're just
>>> reusing it for the alloc.
>>>
>>> Do others think option 2 is OK to add this at this late stage? Even if
>>> the patch set has already been ACK'd?
>>
>> Jan's use-case looks to be relevant.
>>
>> What about changing:
>>
>>   rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name)
>>
>> into:
>>
>>  rte_mempool_set_ops(struct rte_mempool *mp, const char *name,
>>     void *opaque)
>>
>> ?
>>
>> The opaque pointer would be saved in mempool structure, and used
>> when the mempool is populated (calling mempool_ops_alloc).
>> The type of the structure pointed by the opaque has to be defined
>> (and documented) into each mempool_ops manager.
>>
>
> Yes, that was another option, which has the additional impact of needing an
> opaque added to the mempool struct. If we use the opaque from the
> _populate_
> function, we use it straight away in the alloc, no storage needed.
>
> Also, do you think we need to go ahead with this change, or can we add
> it later as an
> improvement?

The opaque in populate_phys() is already used for something else
(i.e. the argument for the free callback of the memory chunk).
I'm afraid it could cause confusion to have it used for 2 different
things.

About the change, I think it could be good to have it in 16.11,
because it will probably change the API, and we should avoid to
change it each version ;)

So I'd vote to have it in the patchset for consistency.


Olivier