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

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

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.

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

> 
> 
> >>>> 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)
> > 
> > OK to me.
> 
> I'm not sure I'm getting it. Does it mean having something like
> this ?
> 
> rte_mempool_set_handler(struct rte_mempool *mp, const char *name,
> 	unsigned int flags)
> 
> Or does it mean some of the flags passed to rte_mempool_create*()
> will be specific to some handlers?
> 
> 
> Before adding handler-specific flags or config, can we ensure we
> will need them? What kind of handler-specific configuration/flags
> do you think we will need? Just an idea: what about having a global
> configuration for all mempools using a given handler?

We may need to configure external pool manager like don't free the
packets back to pool  after it has been send out(just an example of
valid external HW pool manager configuration)

> 
> 
> 
> >>>>> 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"?

> 
> 
> Thanks David for working on this, and thanks Jerin and Jan for
> the good comments and suggestions!
> 
> Regards
> Olivier