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

[Olivier MATZ]

On 06/15/2016 04:02 PM, Hunt, David wrote:
>
>
> On 15/6/2016 2:50 PM, Olivier MATZ wrote:
>> 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.
>
> Sure, we should avoid breaking API just after we created it. :)
>
> OK, here's a slightly different proposal.
>
> If we add a string, to the _ops_byname, yes, that will work for Jan's case.
> However, if we add a void*, that allow us the flexibility of passing
> anything we
> want. We can then store the void* in the mempool struct as void
> *pool_config,
> so that when the alloc gets called, it can access whatever is stored at
> *pool_config
> and do the correct initialisation/allocation. In Jan's use case, this
> can simply be typecast
> to a string. In future cases, it can be a struct, which could including
> new flags.

Yep, agree. But not sure I'm seeing the difference with what I
proposed.

>
> I think that change is minimal enough to be low risk at this stage.
>
> Thoughts?

Agree. Thanks!


Olivier