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

["Venkatesan, Venky" <venky.venkatesan@intel.com>]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Lazaros Koromilas
> Sent: Thursday, March 24, 2016 7:36 AM
> To: Wiles, Keith <keith.wiles@intel.com>
> Cc: Olivier Matz <olivier.matz@6wind.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] mempool: allow for user-owned mempool
> caches
> 
> 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...

Hi Lazaros, 

Alternative suggestion: This could actually be very simply done via creating an EAL API to register and return an lcore_id for a thread wanting to use DPDK services. That way, you could simply create your pthread, call the eal_register_thread() function that assigns an lcore_id to the caller (and internally sets up the per_lcore variable. 

The advantage of doing it this way is that you could extend it to other things other than the mempool that may need an lcore_id setup.

Regards,
-Venky

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