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)
Splitting the flags into 3 groups, with one not beeing flags but a
pool handler number looks overcomplicated from a user perspective.
>>> 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?).
>>>> 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?
>>>>> 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?
Thanks David for working on this, and thanks Jerin and Jan for
the good comments and suggestions!
Regards
Olivier
