[dpdk-dev] [RFC] More changes for rte_mempool.h:__mempool_get_bulk()
Wiles, Roger Keith
keith.wiles at windriver.com
Mon Sep 29 01:17:34 CEST 2014
On Sep 28, 2014, at 5:41 PM, Ananyev, Konstantin <konstantin.ananyev at intel.com> wrote:
>
>
>> -----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.
Sorry, I combined my thoughts on changing the get_bulk behavior and you would be correct for the current design. This is why I decided to make it an RFC :-)
>
>> + 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.
My thinking (at the time) was get_bulk should return ’n’ instead of zero, which I feel is the better coding. You are correct it does not make sense unless you factor in my thinking at time :-(
>
>> + /*
>> + * 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
Do we think it is worth it to change the behavior of get_bulk returning ’n’ instead of zero on success? It would remove a few test IMO in a couple of places. We could also return <0 on the zero case as well, just to make sure code did not try to follow the success case by mistake.
>
> NACK in summary.
> Konstantin
Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
More information about the dev
mailing list