Re: [PATCH v2 1/3] mempool: add stack (lifo) mempool handler

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

Hi Dave,

On 06/20/2016 02:59 PM, Hunt, David wrote:
> Hi Olivier,
>
> On 20/6/2016 9:17 AM, Olivier Matz wrote:
>> Hi David,
>>
>> On 06/17/2016 04:18 PM, Hunt, David wrote:
>>>> After reading it, I realize that it's nearly exactly the same code than
>>>> in "app/test: test external mempool handler".
>>>> http://patchwork.dpdk.org/dev/patchwork/patch/12896/
>>>>
>>>> We should drop one of them. If this stack handler is really useful for
>>>> a performance use-case, it could go in librte_mempool. At the first
>>>> read, the code looks like a demo example : it uses a simple spinlock
>>>> for
>>>> concurrent accesses to the common pool. Maybe the mempool cache hides
>>>> this cost, in this case we could also consider removing the use of the
>>>> rte_ring.
>>> While I agree that the code is similar, the handler in the test is a
>>> ring based handler,
>>> where as this patch adds an array based handler.
>> Not sure I'm getting what you are saying. Do you mean stack instead
>> of array?
>
> Yes, apologies, stack.
>
>> Actually, both are stacks when talking about bulks of objects. If we
>> consider each objects one by one, that's true the order will differ.
>> But as discussed in [1], the cache code already reverses the order of
>> objects when doing a mempool_get(). I'd say the reversing in cache code
>> is not really needed (only the order of object bulks should remain the
>> same). A rte_memcpy() looks to be faster, but it would require to do
>> some real-life tests to validate or unvalidate this theory.
>>
>> So to conclude, I still think both code in app/test and lib/mempool are
>> quite similar, and only one of them should be kept.
>>
>> [1] http://www.dpdk.org/ml/archives/dev/2016-May/039873.html
>
> OK, so we will probably remove the test app portion in the future is if
> is not needed,
> and if we apply the stack handler proposed in this patch set.

Back on this thread. Maybe I misunderstood what you were saying
here (because I see this comment is not addressed in v3).

I don't think we should add similar code at two different places
and then remove it later in another patchset. I feel it's better to
have only one instance of the stack handler, either in test, or
in librte_mempool.

If you plan to do a v4, I think this is something that could go in 
16.07-rc2.

Regards,
Olivier