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

["Hunt, David" <david.hunt@intel.com>]

Hi Jan,

On 10/6/2016 9:49 AM, Jan Viktorin wrote:
> On Fri, 10 Jun 2016 09:29:44 +0200
> Olivier Matz <olivier.matz@6wind.com> wrote:
>
>> 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.
> Yes, I consider it quite strange. When thinking more about the mempool API,
> I'd move the flags processing to the rte_mempool_create. Semantically, it
> makes more sense as the "empty" clearly describes that it is empty. But with
> the flags, it is not... What is the reason to have those flags there?

Yes, they should be in rte_mempool_create. There were in an earlier 
patch, but regressed.
I'll have them in rte_mempool_create in the next patch.


[...]


Rgds,
Dave.