* [dpdk-dev] [PATCH] Function __mempool_get_bulk() returns wrong count. @ 2014-09-27 18:41 Wiles, Roger Keith 2014-09-28 0:38 ` Neil Horman 2014-09-28 22:25 ` Ananyev, Konstantin 0 siblings, 2 replies; 5+ messages in thread From: Wiles, Roger Keith @ 2014-09-27 18:41 UTC (permalink / raw) To: <dev@dpdk.org> 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@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); - return 0; + return n; 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); return ret; } -- 2.1.0Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-dev] [PATCH] Function __mempool_get_bulk() returns wrong count. 2014-09-27 18:41 [dpdk-dev] [PATCH] Function __mempool_get_bulk() returns wrong count Wiles, Roger Keith @ 2014-09-28 0:38 ` Neil Horman 2014-09-28 22:25 ` Ananyev, Konstantin 1 sibling, 0 replies; 5+ messages in thread From: Neil Horman @ 2014-09-28 0:38 UTC (permalink / raw) To: Wiles, Roger Keith; +Cc: <dev@dpdk.org> On Sat, Sep 27, 2014 at 06:41:41PM +0000, Wiles, Roger Keith wrote: > > 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@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); > > - return 0; > + return n; > > 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); > > return ret; > } > -- > 2.1.0Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533 > > Acked-by: Neil Horman <nhorman@tuxdriver.com> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-dev] [PATCH] Function __mempool_get_bulk() returns wrong count. 2014-09-27 18:41 [dpdk-dev] [PATCH] Function __mempool_get_bulk() returns wrong count Wiles, Roger Keith 2014-09-28 0:38 ` Neil Horman @ 2014-09-28 22:25 ` Ananyev, Konstantin 2014-09-28 22:57 ` Wiles, Roger Keith 1 sibling, 1 reply; 5+ messages in thread From: Ananyev, Konstantin @ 2014-09-28 22:25 UTC (permalink / raw) To: Wiles, Roger Keith (Wind River), <dev@dpdk.org> > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wiles, Roger Keith > Sent: Saturday, September 27, 2014 7:42 PM > To: <dev@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@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. > > - 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. Konstantin > > return ret; > } > -- > 2.1.0Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533 As I can see ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-dev] [PATCH] Function __mempool_get_bulk() returns wrong count. 2014-09-28 22:25 ` Ananyev, Konstantin @ 2014-09-28 22:57 ` Wiles, Roger Keith 2014-09-28 23:00 ` Ananyev, Konstantin 0 siblings, 1 reply; 5+ messages in thread From: Wiles, Roger Keith @ 2014-09-28 22:57 UTC (permalink / raw) To: ANANYEV, KONSTANTIN; +Cc: <dev@dpdk.org> On Sep 28, 2014, at 5:25 PM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote: > > >> -----Original Message----- >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wiles, Roger Keith >> Sent: Saturday, September 27, 2014 7:42 PM >> To: <dev@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@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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-dev] [PATCH] Function __mempool_get_bulk() returns wrong count. 2014-09-28 22:57 ` Wiles, Roger Keith @ 2014-09-28 23:00 ` Ananyev, Konstantin 0 siblings, 0 replies; 5+ messages in thread From: Ananyev, Konstantin @ 2014-09-28 23:00 UTC (permalink / raw) To: Wiles, Roger Keith (Wind River); +Cc: <dev@dpdk.org> > -----Original Message----- > From: Wiles, Roger Keith [mailto:keith.wiles@windriver.com] > Sent: Sunday, September 28, 2014 11:57 PM > To: Ananyev, Konstantin > Cc: <dev@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@intel.com> wrote: > > > > > > >> -----Original Message----- > >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wiles, Roger Keith > >> Sent: Saturday, September 27, 2014 7:42 PM > >> To: <dev@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@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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-09-28 22:53 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-09-27 18:41 [dpdk-dev] [PATCH] Function __mempool_get_bulk() returns wrong count Wiles, Roger Keith 2014-09-28 0:38 ` Neil Horman 2014-09-28 22:25 ` Ananyev, Konstantin 2014-09-28 22:57 ` Wiles, Roger Keith 2014-09-28 23:00 ` Ananyev, Konstantin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).