On Sep 28, 2014, at 5:25 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: Saturday, September 27, 2014 7:42 PM >> To: <dev at dpdk.org> >> Subject: [dpdk-dev] [PATCH] Function __mempool_get_bulk() returns wrong >> count. >> >> >> When __mempool_get_bulk() grabs entries from the cache it >> returns zero instead of the number of entries obtained. Plus >> the stats were increased by the wrong count of objects. >> >> Signed-off-by: Keith Wiles <keith.wiles at windriver.com> >> --- >> lib/librte_mempool/rte_mempool.h | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/lib/librte_mempool/rte_mempool.h >> b/lib/librte_mempool/rte_mempool.h >> index 299d4d7..6750e78 100644 >> --- a/lib/librte_mempool/rte_mempool.h >> +++ b/lib/librte_mempool/rte_mempool.h >> @@ -988,9 +988,9 @@ __mempool_get_bulk(struct rte_mempool *mp, void >> **obj_table, >> >> cache->len -= n; >> >> - __MEMPOOL_STAT_ADD(mp, get_success, n_orig); >> + __MEMPOOL_STAT_ADD(mp, get_success, n); > > As I can see n == n_orig. > We can completely remove n_orig, but from other side - I don't see any harm > here. In the RFC patch I sent I remove n_orig. > >> >> - return 0; >> + return n; > > As I can see, __mempool_get_bulk supposed to return 0, > if all n objects were allocated from mbuf, or a negative error code otherwise. > Check all usages of __mempool_get_bulk(), plus the fact that it does below: > ret = rte_ring_mc_dequeue_bulk(mp->ring, obj_table, n); > and rte_ring_mc_dequeue_bulk() is just wrapper for > __rte_ring_mc_do_dequeue(..., n, RTE_RING_QUEUE_FIXED); > I.e. - either allocate all n objects, or return with failure. > So, yes we should return 0 here. > The only thing that probably needs to be done here: fix the comments. > Instead of: > - >=0: Success; number of objects supplied. > Something like: > - 0: Success; n objects supplied. > >> >> ring_dequeue: >> #endif /* RTE_MEMPOOL_CACHE_MAX_SIZE > 0 */ >> @@ -1004,7 +1004,7 @@ ring_dequeue: >> if (ret < 0) >> __MEMPOOL_STAT_ADD(mp, get_fail, n_orig); >> else >> - __MEMPOOL_STAT_ADD(mp, get_success, n_orig); >> + __MEMPOOL_STAT_ADD(mp, get_success, ret); > > That seems incorrect tom me. > ret would be either 0 on success, or negative error value. Notice ?if (ret < 0)? above so ret can not be negative in this case only zero or positive. > > Konstantin > > >> >> return ret; >> } >> -- >> 2.1.0Keith Wiles, Principal Technologist with CTO office, Wind River mobile >> 972-213-5533 > > > As I can see Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
