Re: [PATCH v8 1/3] mempool: support external mempool operations

[Olivier Matz <olivier.matz@6wind.com>]

Hi,

On 06/09/2016 03:09 PM, Jan Viktorin wrote:
>>> My suggestion is to have an additional flag,
>>> 'MEMPOOL_F_PKT_ALLOC', which, if specified, would:
>>> 
>>> ... #define MEMPOOL_F_SC_GET    0x0008 #define
>>> MEMPOOL_F_PKT_ALLOC 0x0010 ...
>>> 
>>> in rte_mempool_create_empty: ... after checking the other
>>> MEMPOOL_F_* flags...
>>> 
>>> if (flags & MEMPOOL_F_PKT_ALLOC) rte_mempool_set_ops_byname(mp,
>>> RTE_MBUF_DEFAULT_MEMPOOL_OPS)
>>> 
>>> And removing the redundant call to rte_mempool_set_ops_byname()
>>> in rte_pktmbuf_create_pool().
>>> 
>>> Thereafter, rte_pktmbuf_pool_create can be changed to:
>>> 
>>> ... mp = rte_mempool_create_empty(name, n, elt_size, cache_size, 
>>> -        sizeof(struct rte_pktmbuf_pool_private), socket_id, 0); 
>>> +        sizeof(struct rte_pktmbuf_pool_private), socket_id, +
>>> MEMPOOL_F_PKT_ALLOC); if (mp == NULL) return NULL;
>> 
>> Yes, this would simplify somewhat the creation of a pktmbuf pool,
>> in that it replaces the rte_mempool_set_ops_byname with a flag bit.
>> However, I'm not sure we want to introduce a third method of
>> creating a mempool to the developers. If we introduced this, we
>> would then have: 1. rte_pktmbuf_pool_create() 2.
>> rte_mempool_create_empty() with MEMPOOL_F_PKT_ALLOC set (which
>> would use the configured custom handler) 3.
>> rte_mempool_create_empty() with MEMPOOL_F_PKT_ALLOC __not__ set
>> followed by a call to rte_mempool_set_ops_byname() (would allow
>> several different custom handlers to be used in one application
>> 
>> Does anyone else have an opinion on this? Oliver, Jerin, Jan?
> 
> I am quite careful about this topic as I don't feel to be very
> involved in all the use cases. My opinion is that the _new API_
> should be able to cover all cases and the _old API_ should be
> backwards compatible, however, built on top of the _new API_.
> 
> I.e. I think, the flags MEMPOOL_F_SP_PUT, MEMPOOL_F_SC_GET (relicts
> of the old API) should be accepted by the old API ONLY. The
> rte_mempool_create_empty should not process them.

The rte_mempool_create_empty() function already processes these flags
(SC_GET, SP_PUT) as of today.

> Similarly for a potential MEMPOOL_F_PKT_ALLOC, I would not polute the
> rte_mempool_create_empty by this anymore.

+1

I think we should stop adding flags. Flags are prefered for independent
features. Here, what would be the behavior with MEMPOOL_F_PKT_ALLOC +
MEMPOOL_F_SP_PUT?

Another reason to not add this flag is the rte_mempool library
should not be aware of mbufs. The mbuf pools rely on mempools, but
not the contrary.


> In overall we would get exactly 2 approaches (and not more):
> 
> * using rte_mempool_create with flags calling the
> rte_mempool_create_empty and rte_mempool_set_ops_byname internally
> (so this layer can be marked as deprecated and removed in the
> future)

Agree. This was one of the objective of my mempool rework patchset:
provide a more flexible API, and avoid functions with 10 to 15
arguments.

> * using rte_mempool_create_empty + rte_mempool_set_ops_byname -
> allowing any customizations but with the necessity to change the
> applications (new preferred API)

Yes.
And if required, maybe a third API is possible in case of mbuf pools.
Indeed, the applications are encouraged to use rte_pktmbuf_pool_create()
to create a pool of mbuf instead of mempool API. If an application
wants to select specific ops for it, we could add:

  rte_pktmbuf_pool_create_<something>(..., name)

instead of using the mempool API.
I think this is what Shreyansh suggests when he says:

  It sounds fine if calls to rte_mempool_* can select an external
  handler *optionally* - but, if we pass it as command line, it would
  be binding (at least, semantically) for rte_pktmbuf_* calls as well.
  Isn't it?


> So, the old applications can stay as they are (OK, with a possible
> new flag MEMPOOL_F_PKT_ALLOC) and the new one can do the same but you
> have to set the ops explicitly.
> 
> The more different ways of using those APIs we have, the greater hell
> we have to maintain.

I'm really not in favor of a MEMPOOL_F_PKT_ALLOC flag in mempool api.

I think David's patch is already a good step forward. Let's do it
step by step. Next step is maybe to update some applications (at least
testpmd) to select a new pool handler dynamically.

Regards,
Olivier