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. Olivier