> -----Original Message----- > From: Wiles, Roger Keith [mailto:keith.wiles at windriver.com] > Sent: Sunday, September 28, 2014 11:57 PM > To: Ananyev, Konstantin > Cc: <dev at dpdk.org> > Subject: Re: [dpdk-dev] [PATCH] Function __mempool_get_bulk() returns wrong > count. > > > 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.
It can't be positive here. Only zero. See above why. > > > > 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 >
