Re: [dpdk-dev] [PATCH v5 1/3] mempool: support external handler

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

On Wed, Jun 01, 2016 at 11:46:20AM +0100, Hunt, David wrote:
> 
> 
> On 5/31/2016 10:11 PM, Jerin Jacob wrote:
> > On Tue, May 31, 2016 at 10:41:00PM +0200, Olivier MATZ wrote:
> > > Hi,
> > > 
> > > On 05/31/2016 06:03 PM, Jerin Jacob wrote:
> > > > On Tue, May 31, 2016 at 04:37:02PM +0100, Hunt, David wrote:
> > > > > 
> > > > > 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.
> > > > That would be an internal API(upper 8 bits to handler name). Right ?
> > > > Seems to be OK for me.
> > > > 
> > > > > Having the _empty and _set_handler  APIs seems to me to be OK for the
> > > > > moment. Maybe Olivier could comment?
> > > > > 
> > > > But need 3 APIs. Right? _empty , _set_handler and _populate ? I believe
> > > > it is better reduce the public API in spec where ever possible ?
> > > > 
> > > > Maybe Olivier could comment ?
> > > Well, I think having 3 different functions is not a problem if the API
> > > is clearer.
> > > 
> > > In my opinion, the following:
> > > 	rte_mempool_create_empty()
> > > 	rte_mempool_set_handler()
> > > 	rte_mempool_populate()
> > > 
> > > is clearer than:
> > > 	rte_mempool_create(15 args)
> > But proposed scheme is not adding any new arguments to
> > rte_mempool_create. It just extending the existing flag.
> > 
> > rte_mempool_create(15 args) is still their as API for internal pool
> > creation.
> > 
> > > Splitting the flags into 3 groups, with one not beeing flags but a
> > > pool handler number looks overcomplicated from a user perspective.
> > I am concerned with seem less integration with existing applications,
> > IMO, Its not worth having separate functions for external vs internal
> > pool creation for application(now each every applications has to added this
> > logic every where for no good reason), just my 2 cents.
> 
> I think that there is always going to be some  extra code in the
> applications
>  that want to use an external mempool. The _set_handler approach does
> create, set_hander, populate. The Flags method queries the handler list to
> get the index, sets the flags bits, then calls create. Both methods will
> work.

I was suggesting flags like TXQ in ethdev where application just
selects the mode. Not sure why application has to get the index first.

some thing like,
#define ETH_TXQ_FLAGS_NOMULTSEGS 0x0001 /**< nb_segs=1 for all mbufs */
#define ETH_TXQ_FLAGS_NOREFCOUNT 0x0002 /**< refcnt can be ignored */
#define ETH_TXQ_FLAGS_NOMULTMEMP 0x0004 /**< all bufs come from same mempool */ 

Anyway, Looks like  no one else much bothered about external pool
manger creation API being different. So, I given up. No objections from my side :-)

> 
> But I think the _set_handler approach is more user friendly, therefore that
> it the method I would lean towards.
> 
> > > > > > 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.
> > > > > 
> > > > Yes, I understand, but I am trying to avoid build time constant. IMO, It
> > > > would be better by default RTE_MBUF_DEFAULT_MEMPOOL_HANDLER is not
> > > > defined in config. and for quick change developers can introduce the build
> > > > with RTE_MBUF_DEFAULT_MEMPOOL_HANDLER="specific handler"
> > > My understanding of the compile-time configuration option was
> > > to allow a specific architecture to define a specific hw-assisted
> > > handler by default.
> > > 
> > > Indeed, if there is no such need for now, we may remove it. But
> > > we need a way to select another handler, at least in test-pmd
> > > (in command line arguments?).
> > like txflags in testpmd, IMO, mempool flags will help to select the handlers
> > seamlessly as suggest above.
> > 
> > If we are _not_ taking the flags based selection scheme then it makes to
> > keep RTE_MBUF_DEFAULT_MEMPOOL_HANDLER
> 
> see comment above

Try to add some means to select the external handler for existing
applications so that we can test existing applications in different modes.

Thanks,
Jerin

> 
> > > > > > > > 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?
> > > > OK. But simple anonymous union for same type should be OK add now? Not
> > > > much change I believe, If its difficult then postpone it
> > > > 
> > > > union {
> > > > 	void *pool;
> > > > 	uint64_t val;
> > > > }
> > > I'm ok with the simple union with (void *) and (uint64_t).
> > > Maybe "val" should be replaced by something more appropriate.
> > > Is "pool_id" a better name?
> > How about "opaque"?
> 
> I think I would lean towards pool_id in this case.
> 
> 
> Regards,
> David.