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