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.
I think that change is minimal enough to be low risk at this stage.
Thoughts?
Dave.
