On 15/6/2016 3:47 PM, Jan Viktorin wrote: > On Wed, 15 Jun 2016 16:10:13 +0200 > Olivier MATZ <olivier.matz at 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 > >