On 5/31/2016 9:53 AM, Jerin Jacob wrote: > On Mon, May 30, 2016 at 12:27:26PM +0100, Hunt, David wrote: >> New mempool handlers will use rte_mempool_create_empty(), >> rte_mempool_set_handler(), >> then rte_mempool_populate_*(). These three functions are new to this >> release, to no problem > Having separate APIs for external pool-manager create is worrisome in > application perspective. Is it possible to have rte_mempool_[xmem]_create > for the both external and existing SW pool manager and make > rte_mempool_create_empty and rte_mempool_populate_* internal functions. > > IMO, We can do that by selecting specific rte_mempool_set_handler() > based on _flags_ encoding, something like below > > bit 0 - 16 // generic bits uses across all the pool managers > bit 16 - 23 // pool handler specific flags bits > bit 24 - 31 // to select the specific pool manager(Up to 256 different > flavors of > pool managers, For backward compatibility, make '0'(in 24-31) to select > existing SW pool manager. > > and applications can choose the handlers by selecting the flag in > rte_mempool_[xmem]_create, That way it will be easy in testpmd or any other > applications to choose the pool handler from command line etc in future.
There might be issues with the 8-bit handler number, as we'd have to add an api call to first get the index of a given hander by name, then OR it into the flags. That would mean also extra API calls for the non-default external handlers. I do agree with the handler-specific bits though. Having the _empty and _set_handler APIs seems to me to be OK for the moment. Maybe Olivier could comment? > and we can remove "mbuf: get default mempool handler from configuration" > change-set OR just add if RTE_MBUF_DEFAULT_MEMPOOL_HANDLER is defined then set > the same with rte_mempool_set_handler in rte_mempool_[xmem]_create. > > What do you think? The "configuration" patch is to allow users to quickly change the mempool handler by changing RTE_MBUF_DEFAULT_MEMPOOL_HANDLER to another string of a known handler. It could just as easily be left out and use the rte_mempool_create. >> to add a parameter to one of them for the config data. Also since we're >> adding some new >> elements to the mempool structure, how about we add a new pointer for a void >> pointer to a >> config data structure, as defined by the handler. >> >> So, new element in rte_mempool struct alongside the *pool >> void *pool; >> void *pool_config; >> >> Then add a param to the rte_mempool_set_handler function: >> int >> rte_mempool_set_handler(struct rte_mempool *mp, const char *name, void >> *pool_config) > IMO, Maybe we need to have _set_ and _get_.So I think we can have > two separate callback in external pool-manger for that if required. > IMO, For now, We can live with pool manager specific 8 bits(bit 16 -23) > for the configuration as mentioned above and add the new callbacks for > set and get when required? OK, We'll keep the config to the 8 bits of the flags for now. That will also mean I won't add the pool_config void pointer either (for the moment) >>> 2) IMO, It is better to change void *pool in struct rte_mempool to >>> anonymous union type, something like below, so that mempool >>> implementation can choose the best type. >>> union { >>> void *pool; >>> uint64_t val; >>> } >> Could we do this by using the union for the *pool_config suggested above, >> would that give >> you what you need? > It would be an extra overhead for external pool manager to _alloc_ memory > and store the allocated pointer in mempool struct(as *pool) and use pool for > pointing other data structures as some implementation need only > limited bytes to store the external pool manager specific context. > > In order to fix this problem, We may classify fast path and slow path > elements in struct rte_mempool and move all fast path elements in first > cache line and create an empty opaque space in the remaining bytes in the > cache line so that if the external pool manager needs only limited space > then it is not required to allocate the separate memory to save the > per core cache in fast-path > > something like below, > union { > void *pool; > uint64_t val; > uint8_t extra_mem[16] // available free bytes in fast path cache line > > } Something for the future, perhaps? Will the 8-bits in the flags suffice for now? > Other points, > > 1) Is it possible to remove unused is_mp in __mempool_put_bulk > function as it is just a internal function. Fixed > 2) Considering "get" and "put" are the fast-path callbacks for > pool-manger, Is it possible to avoid the extra overhead of the following > _load_ and additional cache line on each call, > rte_mempool_handler_table.handler[handler_idx] > > I understand it is for multiprocess support but I am thing can we > introduce something like ethernet API support for multiprocess and > resolve "put" and "get" functions pointer on init and store in > struct mempool. Some thinking like > > file: drivers/net/ixgbe/ixgbe_ethdev.c > search for if (rte_eal_process_type() != RTE_PROC_PRIMARY) { I'll look at this one before posting the next version of the patch (soon). :) > Jerin > Thanks for your input on this, much appreciated. Dave.