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

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

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/

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


> +static void
> +mempool_cache_init(struct rte_mempool_cache *cache, uint32_t size)
> +{
> +	cache->size = size;
> +	cache->flushthresh = CALC_CACHE_FLUSHTHRESH(size);
> +	cache->len = 0;
> +}
> +
> +/*
> + * Creates and initializes a cache for objects that are retrieved from and
> + * returned to an underlying mempool. This structure is identical to the
> + * structure included inside struct rte_mempool.
> + */

On top of Keith's patch, this comment may be reworked as the cache
structure is not included in the mempool structure anymore.

nit: I think the imperative form is preferred


> +struct rte_mempool_cache *
> +rte_mempool_cache_create(uint32_t size)
> +{
> +	struct rte_mempool_cache *cache;
> +
> +	if (size > RTE_MEMPOOL_CACHE_MAX_SIZE) {
> +		rte_errno = EINVAL;
> +		return NULL;
> +	}
> +
> +	cache = rte_zmalloc("MEMPOOL_CACHE", sizeof(*cache), RTE_CACHE_LINE_SIZE);
> +	if (cache == NULL) {
> +		RTE_LOG(ERR, MEMPOOL, "Cannot allocate mempool cache!\n");

I would remove the '!'



> @@ -587,10 +624,18 @@ rte_mempool_xmem_create(const char *name, unsigned n, unsigned elt_size,
>  	mp->elt_size = objsz.elt_size;
>  	mp->header_size = objsz.header_size;
>  	mp->trailer_size = objsz.trailer_size;
> -	mp->cache_size = cache_size;
> -	mp->cache_flushthresh = CALC_CACHE_FLUSHTHRESH(cache_size);
> +	mp->cache_size = cache_size; /* Keep this for backwards compat. */

I'm wondering if this should be kept for compat or if it makes sense
to keep it. The question is: do we want the cache_size to be a parameter
of the mempool or should it be a parameter of the cache?

I think we could remove this field from the mempool structure.


> @@ -673,13 +718,17 @@ rte_mempool_dump_cache(FILE *f, const struct rte_mempool *mp)
>  #if RTE_MEMPOOL_CACHE_MAX_SIZE > 0
>  	unsigned lcore_id;
>  	unsigned count = 0;
> +	unsigned cache_size;
>  	unsigned cache_count;
>  
>  	fprintf(f, "  cache infos:\n");
> -	fprintf(f, "    cache_size=%"PRIu32"\n", mp->cache_size);
>  	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
> +		cache_size = mp->local_cache[lcore_id].size;
> +		fprintf(f, "    cache_size[%u]=%"PRIu32"\n",
> +			lcore_id, cache_size);
>  		cache_count = mp->local_cache[lcore_id].len;
> -		fprintf(f, "    cache_count[%u]=%u\n", lcore_id, cache_count);
> +		fprintf(f, "    cache_count[%u]=%"PRIu32"\n",
> +			lcore_id, cache_count);
>  		count += cache_count;

Does it still make sense to dump the content of the cache as some
external caches may exist?

If we want to list all caches, we could imagine a sort of cache
registration before using a cache structure. Example:

   cache = rte_mempool_add_cache()
   rte_mempool_del_cache()

All the caches could be browsed internally using a list.

Thoughts?


> --- a/lib/librte_mempool/rte_mempool.h
> +++ b/lib/librte_mempool/rte_mempool.h
> @@ -95,19 +95,19 @@ struct rte_mempool_debug_stats {
>  } __rte_cache_aligned;
>  #endif
>  
> -#if RTE_MEMPOOL_CACHE_MAX_SIZE > 0
>  /**
>   * A structure that stores a per-core object cache.
>   */
>  struct rte_mempool_cache {
> -	unsigned len; /**< Cache len */
> +	uint32_t size;        /**< Size of the cache */
> +	uint32_t flushthresh; /**< Threshold before we flush excess elements */
> +	uint32_t len;         /**< Current cache count */
>  	/*
>  	 * Cache is allocated to this size to allow it to overflow in certain
>  	 * cases to avoid needless emptying of cache.
>  	 */
>  	void *objs[RTE_MEMPOOL_CACHE_MAX_SIZE * 3]; /**< Cache objects */
>  } __rte_cache_aligned;
> -#endif /* RTE_MEMPOOL_CACHE_MAX_SIZE > 0 */
>  
>  /**
>   * A structure that stores the size of mempool elements.
> @@ -185,8 +185,6 @@ struct rte_mempool {
>  	int flags;                       /**< Flags of the mempool. */
>  	uint32_t size;                   /**< Size of the mempool. */
>  	uint32_t cache_size;             /**< Size of per-lcore local cache. */
> -	uint32_t cache_flushthresh;
> -	/**< Threshold before we flush excess elements. */
>  
>  	uint32_t elt_size;               /**< Size of an element. */
>  	uint32_t header_size;            /**< Size of header (before elt). */

Adding and removing these fields in the structure will break
the ABI. Could you please send a deprecation notice for it for
16.04, in the same model as http://dpdk.org/dev/patchwork/patch/11553/

The process is to get 3 acks for this deprecation notice, but I think
this won't be an issue as there are already several changes scheduled
in mempool that will break the ABI in 16.07, so it's the right time to
do it.



>  /**
> + * Put several objects back in the mempool (multi-producers safe).
> + * Use a user-provided mempool cache.
> + *
> + * @param mp
> + *   A pointer to the mempool structure.
> + * @param obj_table
> + *   A pointer to a table of void * pointers (objects).
> + * @param n
> + *   The number of objects to add in the mempool from the obj_table.
> + * @param cache
> + *   A pointer to a mempool cache structure. May be NULL if not needed.
> + */
> +static inline void __attribute__((always_inline))
> +rte_mempool_mp_put_bulk_with_cache(struct rte_mempool *mp,
> +				   void * const *obj_table, unsigned n,
> +				   struct rte_mempool_cache *cache)
> +{
> +	__mempool_check_cookies(mp, obj_table, n, 0);
> +	__mempool_put_bulk_with_cache(mp, obj_table, n, cache, 1);
> +}
> +
> +/**
> + * Put several objects back in the mempool (NOT multi-producers safe).
> + * Use a user-provided mempool cache.
> + *
> + * @param mp
> + *   A pointer to the mempool structure.
> + * @param obj_table
> + *   A pointer to a table of void * pointers (objects).
> + * @param n
> + *   The number of objects to add in the mempool from obj_table.
> + * @param cache
> + *   A pointer to a mempool cache structure. May be NULL if not needed.
> + */
> +static inline void
> +rte_mempool_sp_put_bulk_with_cache(struct rte_mempool *mp,
> +				   void * const *obj_table, unsigned n,
> +				   struct rte_mempool_cache *cache)
> +{
> +	__mempool_check_cookies(mp, obj_table, n, 0);
> +	__mempool_put_bulk_with_cache(mp, obj_table, n, cache, 0);
> +}
>
> [...]

Many functions are added here. As all of these functions are inline,
what would you think to have instead one new functions that would
match all cases:

static inline void
rte_mempool_generic_put(struct rte_mempool *mp,
		void * const *obj_table, unsigned n,
		struct rte_mempool_cache *cache,
		unsigned flags)

We could then consider the deprecation of some functions. Today,
without your patch, we have for put():

	rte_mempool_mp_put_bulk(mp, obj_table, n)
	rte_mempool_sp_put_bulk(mp, obj_table, n)
	rte_mempool_put_bulk(mp, obj_table, n)
	rte_mempool_mp_put(mp, obj)
	rte_mempool_sp_put(mp, obj)
	rte_mempool_put(mp, obj)

	__mempool_put_bulk(mp, obj_table, n, is_mp)  /* internal */

Maybe we should only keep:

	rte_mempool_put()
	rte_mempool_put_bulk(mp, obj_table, n)
	rte_mempool_generic_put(mp, obj_table, n, cache, flags)

and same for *get().



> +/**
> + * Create a user-owned mempool cache. This can be used by non-EAL threads
> + * to enable caching when they interact with a mempool.

nit: the doxygen format needs a one-line title

> + *
> + * @param size
> + *   The size of the mempool cache. See rte_mempool_create()'s cache_size
> + *   parameter description for more information. The same limits and
> + *   considerations apply here too.
> + */
> +struct rte_mempool_cache *
> +rte_mempool_cache_create(uint32_t size);
> +

I think we should also consider a free() function.

Maybe we should explain a bit more in the help of this function that
a mempool embeds a per-lcore cache by default.

Also, it would be great to have a test in app/test_mempool.c to validate
the feature and have an example of use.

And last thing, this is probably out of scope for now, but I'm
wondering if in the future this external cache could be used to
transparently share a pool (typically a mbuf pool), for instance
in case of a secondary process. Each process would have its own cache,
referencing the same shared mempool structure. This would probably
require to update the ethdev and mbuf API, so it's certainly a quite
large modification. If you have any ideas or plans going in this
direction, I would be interested.


Regards,
Olivier
  

Hi lads,

> 
> Hi Lazaros,
> 
> Thanks for this patch. To me, this is a valuable enhancement.
> Please find some comments inline.

Yep, patch seems interesting.
One question - what would be the usage model for get/put_with_cache functions for non-EAL threads?
I meant for each non-EAL thread user will need to maintain an array (or list or some other structure)
of pair <mempool_pointer, mempool_cache> to make sure that the cache always matches the mempool,
correct?     
Again, for non-EAL threads don't we need some sort of invalidate_cache()
that would put all mbufs in the cache back to the pool?
For thread termination case or so?

> 
> 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/
> 
> 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
> 
> 
> > +static void
> > +mempool_cache_init(struct rte_mempool_cache *cache, uint32_t size)
> > +{
> > +	cache->size = size;
> > +	cache->flushthresh = CALC_CACHE_FLUSHTHRESH(size);
> > +	cache->len = 0;
> > +}
> > +
> > +/*
> > + * Creates and initializes a cache for objects that are retrieved from and
> > + * returned to an underlying mempool. This structure is identical to the
> > + * structure included inside struct rte_mempool.
> > + */
> 
> On top of Keith's patch, this comment may be reworked as the cache
> structure is not included in the mempool structure anymore.
> 
> nit: I think the imperative form is preferred
> 
> 
> > +struct rte_mempool_cache *
> > +rte_mempool_cache_create(uint32_t size)

I suppose extra socket_id parameter is needed, so you can call
zmalloc_socket() beneath.

> > +{
> > +	struct rte_mempool_cache *cache;
> > +
> > +	if (size > RTE_MEMPOOL_CACHE_MAX_SIZE) {
> > +		rte_errno = EINVAL;
> > +		return NULL;
> > +	}
> > +
> > +	cache = rte_zmalloc("MEMPOOL_CACHE", sizeof(*cache), RTE_CACHE_LINE_SIZE);
> > +	if (cache == NULL) {
> > +		RTE_LOG(ERR, MEMPOOL, "Cannot allocate mempool cache!\n");
> 
> I would remove the '!'
> 
> 
> 
> > @@ -587,10 +624,18 @@ rte_mempool_xmem_create(const char *name, unsigned n, unsigned elt_size,
> >  	mp->elt_size = objsz.elt_size;
> >  	mp->header_size = objsz.header_size;
> >  	mp->trailer_size = objsz.trailer_size;
> > -	mp->cache_size = cache_size;
> > -	mp->cache_flushthresh = CALC_CACHE_FLUSHTHRESH(cache_size);
> > +	mp->cache_size = cache_size; /* Keep this for backwards compat. */
> 
> I'm wondering if this should be kept for compat or if it makes sense
> to keep it. The question is: do we want the cache_size to be a parameter
> of the mempool or should it be a parameter of the cache?
> 
> I think we could remove this field from the mempool structure.
> 
> 
> > @@ -673,13 +718,17 @@ rte_mempool_dump_cache(FILE *f, const struct rte_mempool *mp)
> >  #if RTE_MEMPOOL_CACHE_MAX_SIZE > 0
> >  	unsigned lcore_id;
> >  	unsigned count = 0;
> > +	unsigned cache_size;
> >  	unsigned cache_count;
> >
> >  	fprintf(f, "  cache infos:\n");
> > -	fprintf(f, "    cache_size=%"PRIu32"\n", mp->cache_size);
> >  	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
> > +		cache_size = mp->local_cache[lcore_id].size;
> > +		fprintf(f, "    cache_size[%u]=%"PRIu32"\n",
> > +			lcore_id, cache_size);
> >  		cache_count = mp->local_cache[lcore_id].len;
> > -		fprintf(f, "    cache_count[%u]=%u\n", lcore_id, cache_count);
> > +		fprintf(f, "    cache_count[%u]=%"PRIu32"\n",
> > +			lcore_id, cache_count);
> >  		count += cache_count;
> 
> Does it still make sense to dump the content of the cache as some
> external caches may exist?
> 
> If we want to list all caches, we could imagine a sort of cache
> registration before using a cache structure. Example:
> 
>    cache = rte_mempool_add_cache()
>    rte_mempool_del_cache()
> 
> All the caches could be browsed internally using a list.
> 
> Thoughts?

Seems a bit excessive to me.
People still can have and use extrenal cache without any registration.
I suppose all we can do here - print 'internal' caches.
Konstantin
  

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

>

>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
  

Hi Konstantin,

On Mon, Mar 21, 2016 at 3:15 PM, Ananyev, Konstantin
<konstantin.ananyev@intel.com> wrote:
>
> Hi lads,
>
>>
>> Hi Lazaros,
>>
>> Thanks for this patch. To me, this is a valuable enhancement.
>> Please find some comments inline.
>
> Yep, patch seems interesting.
> One question - what would be the usage model for get/put_with_cache functions for non-EAL threads?
> I meant for each non-EAL thread user will need to maintain an array (or list or some other structure)
> of pair <mempool_pointer, mempool_cache> to make sure that the cache always matches the mempool,
> correct?

Yes, the user would have to also keep track of the caches. On the use
case, the original idea was to enable caches for threads that run on
the same core. There are cases that this can work, as described in the
programming guide, section on multiple pthreads. Even with two EAL
threads, we currently cannot have multiple caches on a single core,
unless we use the --lcores long option to separate lcore id (which
really is a thread id here) from the actual core affinity.

So the cache should be a member of the thread and not the mempool, in
my opinion. It has to be consistently used with the same mempool
though.

> Again, for non-EAL threads don't we need some sort of invalidate_cache()
> that would put all mbufs in the cache back to the pool?
> For thread termination case or so?

I think also EAL threads benefit from an invalidate() function call.
This can be part of free() that Olivier proposed.

Thanks!
Lazaros.

>
>>
>> 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/
>>
>> 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
>>
>>
>> > +static void
>> > +mempool_cache_init(struct rte_mempool_cache *cache, uint32_t size)
>> > +{
>> > +   cache->size = size;
>> > +   cache->flushthresh = CALC_CACHE_FLUSHTHRESH(size);
>> > +   cache->len = 0;
>> > +}
>> > +
>> > +/*
>> > + * Creates and initializes a cache for objects that are retrieved from and
>> > + * returned to an underlying mempool. This structure is identical to the
>> > + * structure included inside struct rte_mempool.
>> > + */
>>
>> On top of Keith's patch, this comment may be reworked as the cache
>> structure is not included in the mempool structure anymore.
>>
>> nit: I think the imperative form is preferred
>>
>>
>> > +struct rte_mempool_cache *
>> > +rte_mempool_cache_create(uint32_t size)
>
> I suppose extra socket_id parameter is needed, so you can call
> zmalloc_socket() beneath.
>
>> > +{
>> > +   struct rte_mempool_cache *cache;
>> > +
>> > +   if (size > RTE_MEMPOOL_CACHE_MAX_SIZE) {
>> > +           rte_errno = EINVAL;
>> > +           return NULL;
>> > +   }
>> > +
>> > +   cache = rte_zmalloc("MEMPOOL_CACHE", sizeof(*cache), RTE_CACHE_LINE_SIZE);
>> > +   if (cache == NULL) {
>> > +           RTE_LOG(ERR, MEMPOOL, "Cannot allocate mempool cache!\n");
>>
>> I would remove the '!'
>>
>>
>>
>> > @@ -587,10 +624,18 @@ rte_mempool_xmem_create(const char *name, unsigned n, unsigned elt_size,
>> >     mp->elt_size = objsz.elt_size;
>> >     mp->header_size = objsz.header_size;
>> >     mp->trailer_size = objsz.trailer_size;
>> > -   mp->cache_size = cache_size;
>> > -   mp->cache_flushthresh = CALC_CACHE_FLUSHTHRESH(cache_size);
>> > +   mp->cache_size = cache_size; /* Keep this for backwards compat. */
>>
>> I'm wondering if this should be kept for compat or if it makes sense
>> to keep it. The question is: do we want the cache_size to be a parameter
>> of the mempool or should it be a parameter of the cache?
>>
>> I think we could remove this field from the mempool structure.
>>
>>
>> > @@ -673,13 +718,17 @@ rte_mempool_dump_cache(FILE *f, const struct rte_mempool *mp)
>> >  #if RTE_MEMPOOL_CACHE_MAX_SIZE > 0
>> >     unsigned lcore_id;
>> >     unsigned count = 0;
>> > +   unsigned cache_size;
>> >     unsigned cache_count;
>> >
>> >     fprintf(f, "  cache infos:\n");
>> > -   fprintf(f, "    cache_size=%"PRIu32"\n", mp->cache_size);
>> >     for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
>> > +           cache_size = mp->local_cache[lcore_id].size;
>> > +           fprintf(f, "    cache_size[%u]=%"PRIu32"\n",
>> > +                   lcore_id, cache_size);
>> >             cache_count = mp->local_cache[lcore_id].len;
>> > -           fprintf(f, "    cache_count[%u]=%u\n", lcore_id, cache_count);
>> > +           fprintf(f, "    cache_count[%u]=%"PRIu32"\n",
>> > +                   lcore_id, cache_count);
>> >             count += cache_count;
>>
>> Does it still make sense to dump the content of the cache as some
>> external caches may exist?
>>
>> If we want to list all caches, we could imagine a sort of cache
>> registration before using a cache structure. Example:
>>
>>    cache = rte_mempool_add_cache()
>>    rte_mempool_del_cache()
>>
>> All the caches could be browsed internally using a list.
>>
>> Thoughts?
>
> Seems a bit excessive to me.
> People still can have and use extrenal cache without any registration.
> I suppose all we can do here - print 'internal' caches.
> Konstantin
>
  

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!

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

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

> >

> >

> >

> >
  

>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
  

Hi Venky,

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

From my opinion, externalize the cache structure as Lazaros suggests
would make things simpler, especially in case of dynamic threads
allocation/destruction.

If a lcore_id regristration API is added in EAL, we still need a
max lcore value when the mempool is created so the cache can be
allocated. Moreover, the API would not be as simple, especially
if it needs to support secondary processes.


Regards,
Olivier
  

Hello to everybody,

I find this proposal very interesting as It could be used to solve an issue
that has not been mentioned yet: using memory pools shared with ivshmem.
Currently, due to the per lcore cache, it is not possible to allocate and
deallocate packets from the guest as it will cause cache corruption because
DPDK processes in the guest and in the host share some lcores_id.

If there is an API to register caches, the EAL could create and register
its own caches during the ivshmem initialization in the guest, avoiding
possible cache corruption problems.

I look forward to V2 in order to get a clear idea of how can it be used to
solve this issue.

Mauricio V,

On Fri, Mar 25, 2016 at 11:55 AM, Olivier Matz <olivier.matz@6wind.com>
wrote:

> Hi Venky,
>
> >> 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.
>
> From my opinion, externalize the cache structure as Lazaros suggests
> would make things simpler, especially in case of dynamic threads
> allocation/destruction.
>
> If a lcore_id regristration API is added in EAL, we still need a
> max lcore value when the mempool is created so the cache can be
> allocated. Moreover, the API would not be as simple, especially
> if it needs to support secondary processes.
>
>
> Regards,
> Olivier
>
  

> -----Original Message-----

> From: Olivier Matz [mailto:olivier.matz@6wind.com]

> Sent: Friday, March 25, 2016 3:56 AM

> To: Venkatesan, Venky <venky.venkatesan@intel.com>; Lazaros Koromilas

> <l@nofutznetworks.com>; Wiles, Keith <keith.wiles@intel.com>

> Cc: dev@dpdk.org

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

> caches

> 

> Hi Venky,

> 

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

> 

> From my opinion, externalize the cache structure as Lazaros suggests would

> make things simpler, especially in case of dynamic threads

> allocation/destruction.

> 

> If a lcore_id regristration API is added in EAL, we still need a max lcore value

> when the mempool is created so the cache can be allocated. Moreover, the

> API would not be as simple, especially if it needs to support secondary

> processes.

>

Not really - the secondary process is simply another series of threads. They have their own caches. Yes, we will need a max lcore value, but we can make the allocations dynamic as opposed to static. That way, we will have MAX_LCORE pointers to store per mempool. 

The approach that's suggested currently is workable (and if I were solving mempool alone, this is very likely what I would do too), but is limited to the mempool alone. Adding the API to the eal has a rather huge secondary advantage - you now no longer need to create DPDK threads explicitly any more - you can create pthreads, and manage them how you wish. Architecturally speaking, longer term for DPDK that would be bigger win. 
 
> 

> Regards,

> Olivier