> -----Original Message-----
> From: Olivier Matz [mailto:olivier.m...@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
> >
> >
