> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Wiles, Roger Keith
> Sent: Sunday, September 28, 2014 6:52 PM
> To: <dev at dpdk.org>
> Subject: [dpdk-dev] [RFC] More changes for rte_mempool.h:__mempool_get_bulk()
>
> Here is a Request for Comment on __mempool_get_bulk() routine. I believe I am
> seeing a few more issues in this routine, please look
> at the code below and see if these seem to fix some concerns in how the ring
> is handled.
>
> The first issue I believe is cache->len is increased by ret and not req as we
> do not know if ret == req. This also means the cache->len
> may still not satisfy the request from the cache.
>
> The second issue is if you believe the above code then we have to account for
> that issue in the stats.
>
> Let me know what you think?
> ++Keith
> ---
>
> diff --git a/lib/librte_mempool/rte_mempool.h
> b/lib/librte_mempool/rte_mempool.h
> index 199a493..b1b1f7a 100644
> --- a/lib/librte_mempool/rte_mempool.h
> +++ b/lib/librte_mempool/rte_mempool.h
> @@ -945,9 +945,7 @@ __mempool_get_bulk(struct rte_mempool *mp, void
> **obj_table,
> unsigned n, int is_mc)
> {
> int ret;
> -#ifdef RTE_LIBRTE_MEMPOOL_DEBUG
> - unsigned n_orig = n;
> -#endif
Yep, as I said in my previous mail n_orig could be removed in total.
Though from other side - it is harmless.
> +
> #if RTE_MEMPOOL_CACHE_MAX_SIZE > 0
> struct rte_mempool_cache *cache;
> uint32_t index, len;
> @@ -979,7 +977,21 @@ __mempool_get_bulk(struct rte_mempool *mp, void
> **obj_table,
> goto ring_dequeue;
> }
>
> - cache->len += req;
> + cache->len += ret; // Need to adjust len by ret not req,
> as (ret != req)
> +
rte_ring_mc_dequeue_bulk(.., req) at line 971, would either get all req objects
from the ring and return 0 (success),
or wouldn't get any entry from the ring and return negative value (failure).
So this change is erroneous.
> + if ( cache->len < n ) {
If n > cache_size, then we will go straight to 'ring_dequeue' see line 959.
So no need for that check here.
> + /*
> + * Number (ret + cache->len) may not be >= n. As
> + * the 'ret' value maybe zero or less then 'req'.
> + *
> + * Note:
> + * An issue of order from the cache and common pool
> could
> + * be an issue if (cache->len != 0 and less then n),
> but the
> + * normal case it should be OK. If the user needs to
> preserve
> + * the order of packets then he must set cache_size
> == 0.
> + */
> + goto ring_dequeue;
> + }
> }
>
> /* Now fill in the response ... */
> @@ -1002,9 +1014,12 @@ ring_dequeue:
> ret = rte_ring_sc_dequeue_bulk(mp->ring, obj_table, n);
>
> if (ret < 0)
> - __MEMPOOL_STAT_ADD(mp, get_fail, n_orig);
> - else
> + __MEMPOOL_STAT_ADD(mp, get_fail, n);
> + else {
> __MEMPOOL_STAT_ADD(mp, get_success, ret);
> + // Catch the case when ret != n, adding zero should not be a
> problem.
> + __MEMPOOL_STAT_ADD(mp, get_fail, n - ret);
As I said above, ret == 0 on success, so need for that change.
Just n (or n_orig) is ok here.
> + }
>
> return ret;
> }
>
> Keith Wiles, Principal Technologist with CTO office, Wind River mobile
> 972-213-5533
NACK in summary.
Konstantin
