>On Mon, Mar 21, 2016 at 3:49 PM, Wiles, Keith <keith.wiles at intel.com> wrote: >>>Hi Lazaros, >>> >>>Thanks for this patch. To me, this is a valuable enhancement. >>>Please find some comments inline. >>> >>>On 03/10/2016 03:44 PM, Lazaros Koromilas wrote: >>>> The mempool cache is only available to EAL threads as a per-lcore >>>> resource. Change this so that the user can create and provide their own >>>> cache on mempool get and put operations. This works with non-EAL threads >>>> too. This commit introduces new API calls with the 'with_cache' suffix, >>>> while the current ones default to the per-lcore local cache. >>>> >>>> Signed-off-by: Lazaros Koromilas <l at nofutznetworks.com> >>>> --- >>>> lib/librte_mempool/rte_mempool.c | 65 +++++- >>>> lib/librte_mempool/rte_mempool.h | 442 >>>> ++++++++++++++++++++++++++++++++++++--- >>>> 2 files changed, 467 insertions(+), 40 deletions(-) >>>> >>>> diff --git a/lib/librte_mempool/rte_mempool.c >>>> b/lib/librte_mempool/rte_mempool.c >>>> index f8781e1..cebc2b7 100644 >>>> --- a/lib/librte_mempool/rte_mempool.c >>>> +++ b/lib/librte_mempool/rte_mempool.c >>>> @@ -375,6 +375,43 @@ rte_mempool_xmem_usage(void *vaddr, uint32_t elt_num, >>>> size_t elt_sz, >>>> return usz; >>>> } >>>> >>>> +#if RTE_MEMPOOL_CACHE_MAX_SIZE > 0 >>> >>>I wonder if this wouldn't cause a conflict with Keith's patch >>>that removes some #ifdefs RTE_MEMPOOL_CACHE_MAX_SIZE. >>>See: http://www.dpdk.org/dev/patchwork/patch/10492/ >> >> Hi Lazaros, >> >> The patch I submitted keeps the mempool cache structure (pointers and >> variables) and only allocates the cache if specified by the caller to use a >> cache. This means to me the caller could fill in the cache pointer and >> values into the mempool structure to get a cache without a lot of extra >> code. If we added a set of APIs to fill in these structure variables would >> that not give you the external cache support. I have not really looked at >> the patch to verify this will work, but it sure seems like it. >> >> So my suggestion the caller can just create a mempool without a cache and >> then call a set of APIs to fill in his cache values, does that not work? >> >> If we can do this it reduces the API and possible the ABI changes to mempool >> as the new cache create routines and APIs could be in a new file I think, >> which just updates the mempool structure correctly. > >Hi Keith, > >The main benefit of having an external cache is to allow mempool users >(threads) to maintain a local cache even though they don't have a >valid lcore_id (non-EAL threads). The fact that cache access is done >by indexing with the lcore_id is what makes it difficult... > >What could happen is only have external caches somehow, but that hurts >the common case where you want an automatic cache. >Or a cache registration mechanism (overkill?). > >So, I'm going to work on the comments and send out a v2 asap. Thanks everyone!
Hi Lazaros, I look forward seeing the next version as I have some concerns, but I do not think I am seeing the big picture or reason for the change. Part of my goal with the patch I sent was to remove ifdefs in the code as much as possible, so please try to reduce the ifdefs too. > >Lazaros. > >> >>> >>>As this patch is already acked for 16.07, I think that your v2 >>>could be rebased on top of it to avoid conflicts when Thomas will apply >>>it. >>> >>>By the way, I also encourage you to have a look at other works in >>>progress in mempool: >>>http://www.dpdk.org/ml/archives/dev/2016-March/035107.html >>>http://www.dpdk.org/ml/archives/dev/2016-March/035201.html >>> >>> >> >> Regards, >> Keith >> >> >> >> > Regards, Keith
