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! 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 > > > >
