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
