Re: [dpdk-dev] [PATCH] mempool: allow for user-owned mempool caches

["Wiles, Keith" <keith.wiles@intel.com>]

>On Mon, Mar 21, 2016 at 3:49 PM, Wiles, Keith <keith.wiles@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@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