Re: [dpdk-dev] rte_mempool_get_bulk uses either cache or common pool

["Morten Brørup" <mb@smartsharesystems.com>]

> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Wednesday, October 16, 2019 9:23 AM
> 
> Hi Morten,
> 
> On Fri, Oct 11, 2019 at 01:24:00PM +0200, Morten Brørup wrote:
> > The rte_mempool_get_bulk() documentation says:
> >
> > "If cache is enabled, objects will be retrieved first from cache,
> subsequently from the common pool."
> >
> > But __mempool_generic_get() only uses the cache if the request is
> smaller than the cache size. If not, objects will be retrieved from the
> common pool only.
> >
> > Either the documentation should be corrected, or the implementation
> should behave as described, i.e. retrieve the first of the objects from
> the cache and the remaining objects from the common pool.
> 
> That's correct. I think the documentation could be updated.
> Maybe something like this:
> 
>  * If cache is enabled, objects will be retrieved first from cache,
>  * subsequently from the common pool. If the number of requested
> objects
>  * is higher than the cache size, the objects are directly retrieved
>  * from the common pool.
> 

Agreed.

> The put() suffers from the same problem, but the actual behavior is
> not easy to describe. We could add this:
> 
>  * The objects are added in the cache if it is enabled, and if
>  * the number of objects is smaller than RTE_MEMPOOL_CACHE_MAX_SIZE.
>  * After the operation,	if the cache length is above 1.5 * size,
>  * some	objects	are also returned to the common	pool.

I would phrase the first part of the description like in your get() description:

 * If cache is enabled, the objects are added in the cache. If the number of
 * objects is more than RTE_MEMPOOL_CACHE_MAX_SIZE, the objects are directly
 * returned to the common pool.

And here's a rephrasing of your second part:

 * After the operation, if the cache length reaches the cache flush threshold
 * (which is 1.5 * the cache size), the objects in the cache exceeding the
 * cache size are returned to the common pool.

> 
> But I feel the comment is just a duplication of the code, but in
> english... and there's a risk that they become unsynchronized in the
> future (especially because the comment is above
> rte_mempool_generic_put() and the code is in
> __rte_mempool_generic_put()).

Why do the internal __rte_mempool_generic_put/get() functions even exist? Just collapse them into the public rte_mempool_generic_put/get() functions, and that risk would be reduced. The internal functions are only used here anyway. Alternatively, add the comments to both the public and internal functions.

And it's even worse: The description regarding __mempool_generic_get() also applies to rte_mempool_get_bulk(), where I initially stumbled into the unexpected (undocumented) behavior, and also to rte_mempool_get(). So regarding descriptions of functions that behave the way the functions they call behave... To copy-paste, or not to copy-paste? That is the question.


Another thing is the cache size... When creating the mempool, the cache_size parameter reflects the target cache length, not the actual size of the cache, which is bigger. The documentation doesn't contradict this, but the name of the parameter does. I guess the library has evolved, and the documentation didn't keep up. Which just proves your point above!

And while talking about the cache size:

Closely reading __mempool_generic_put() shows that the cache can hold more than cache->size objects. It can actually hold up to cache->flushthresh - 1 (=cache->size * 1.5 - 1) objects, so you can consider modifying __mempool_generic_get()accordingly:

	/* No cache provided or cannot be satisfied from cache */
-	if (unlikely(cache == NULL || n >= cache->size))
-	if (unlikely(cache == NULL || n >= RTE_MAX(cache->size, cache->len)))
		goto ring_dequeue;

It is probably a theoretical optimization only, as it only benefits requests for certain numbers of elements, which to me seem unlikely. Perhaps adding a comment would be nice. Or just leaving it as is.

> 
> 
> 
> > PS: I stumbled into this while writing the unit test for mbuf bulk
> alloc/free.
> >
> > PPS: It seems unit tests for mempool bulk alloc/free are missing. :-)
> >
> >
> > Med venlig hilsen / kind regards
> > - Morten Brørup
> >
> >