Re: [PATCH v12 0/3] mempool: add external mempool manager

["Hunt, David" <david.hunt@intel.com>]

On 15/6/2016 3:47 PM, Jan Viktorin wrote:
> On Wed, 15 Jun 2016 16:10:13 +0200
> Olivier MATZ <olivier.matz@6wind.com> wrote:
>
>> 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)
> Or a third function?
>
> rte_mempool_set_ops_arg(struct rte_mempool, *mp, void *arg)

I think if we tried to add another function, there would be some 
opposition to that.
I think it's reasonable to add it to set_ops_byname, as we're setting 
the ops and
the ops_args in the same call. We use them later in the alloc.

>
> Or isn't it really a task for a kind of rte_mempool_populate_*?

I was leaning towards that, but a different proposal was suggested. I'm 
OK with
adding the *opaque to set_ops_byname

> This is a part of mempool I am not involved in yet.
>
>>>>>> ?
>>>>>>
>>>>>> 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.
> A string? No, I needed to pass a file descriptor or a pointer to some rte_device
> or something like that. So a void * is a way to go.

Apologies, I misread. *opaque it is.

>>> 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,
> void *ops_context, ops_args, ops_data, ...

I think I'll go with ops_args

>>> 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.
> New flags? Does it mean an API extension?

No, I mean new flags within the opaque data, hidden from the mempool 
manager.

>    
>> Yep, agree. But not sure I'm seeing the difference with what I
>> proposed.
> Me neither... I think it is exactly the same :).
>
> Jan

Yes, misread on my part.

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