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

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

[Morten Brørup]

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.


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

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

[Olivier Matz]

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.

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.

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



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

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

[Morten Brørup]

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