From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bruce Richardson Subject: Re: [RFC] More changes for rte_mempool.h:__mempool_get_bulk() Date: Mon, 29 Sep 2014 13:34:15 +0100 Message-ID: <20140929123415.GA16652@BRICHA3-MOBL> References: <3B9A624B-ABBF-4A20-96CD-8D5607006FEA@windriver.com> <2601191342CEEE43887BDE71AB977258213851D2@IRSMSX104.ger.corp.intel.com> <4F9CE4A3-600B-42E0-B5C0-71D3AF7F0CF5@windriver.com> <20140929120613.GG12072@BRICHA3-MOBL> <2601191342CEEE43887BDE71AB97725821387573@IRSMSX104.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Cc: "" To: "Ananyev, Konstantin" Return-path: Content-Disposition: inline In-Reply-To: <2601191342CEEE43887BDE71AB97725821387573-kPTMFJFq+rGvNW/NfzhIbrfspsVTdybXVpNB7YpNyf8@public.gmane.org> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" On Mon, Sep 29, 2014 at 01:25:11PM +0100, Ananyev, Konstantin wrote: >=20 >=20 > > -----Original Message----- > > From: Richardson, Bruce > > Sent: Monday, September 29, 2014 1:06 PM > > To: Wiles, Roger Keith (Wind River) > > Cc: Ananyev, Konstantin; > > Subject: Re: [dpdk-dev] [RFC] More changes for rte_mempool.h:__mempoo= l_get_bulk() > >=20 > > On Sun, Sep 28, 2014 at 11:17:34PM +0000, Wiles, Roger Keith wrote: > > > > > > On Sep 28, 2014, at 5:41 PM, Ananyev, Konstantin wrote: > > > > > > > > > > > > > > >> -----Original Message----- > > > >> From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Wiles, Roge= r Keith > > > >> Sent: Sunday, September 28, 2014 6:52 PM > > > >> To: > > > >> Subject: [dpdk-dev] [RFC] More changes for rte_mempool.h:__mempo= ol_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 =3D=3D 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 t= o 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_mempo= ol/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, v= oid **obj_table, > > > >> unsigned n, int is_mc) > > > >> { > > > >> int ret; > > > >> -#ifdef RTE_LIBRTE_MEMPOOL_DEBUG > > > >> - unsigned n_orig =3D n; > > > >> -#endif > > > > > > > > Yep, as I said in my previous mail n_orig could be removed in tot= al. > > > > 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 +=3D req; > > > >> + cache->len +=3D ret; // Need to adjust len = by ret not req, as (ret !=3D req) > > > >> + > > > > > > > > rte_ring_mc_dequeue_bulk(.., req) at line 971, would either get a= ll 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' se= e line 959. > > > > So no need for that check here. > > > > > > My thinking (at the time) was get_bulk should return =E2=80=99n=E2=80= =99 instead of zero, which I feel is the better coding. You are correct i= t does not > > make sense unless you factor in my thinking at time :-( > > > > > > > >> + /* > > > >> + * Number (ret + cache->len) may not be = >=3D n. As > > > >> + * the 'ret' value maybe zero or less th= en 'req'. > > > >> + * > > > >> + * Note: > > > >> + * An issue of order from the cache and = common pool could > > > >> + * be an issue if (cache->len !=3D 0 and= less then n), but the > > > >> + * normal case it should be OK. If the u= ser needs to preserve > > > >> + * the order of packets then he must set= cache_size =3D=3D 0. > > > >> + */ > > > >> + goto ring_dequeue; > > > >> + } > > > >> } > > > >> > > > >> /* Now fill in the response ... */ > > > >> @@ -1002,9 +1014,12 @@ ring_dequeue: > > > >> ret =3D rte_ring_sc_dequeue_bulk(mp->ring, obj_ta= ble, 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 !=3D n, adding zero s= hould not be a problem. > > > >> + __MEMPOOL_STAT_ADD(mp, get_fail, n - ret); > > > > > > > > As I said above, ret =3D=3D 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 retur= ning =E2=80=99n=E2=80=99 instead of zero on success? It would remove a fe= w test > > IMO in a couple of places. We could also return <0 on the zero case a= s well, just to make sure code did not try to follow the success > > case by mistake. > >=20 > > If you want to have such a function, i think it should align with the > > functions on the rings. In this case, this would mean having a get_bu= rst > > function, which returns less than or equal to the number of elements > > requested. I would not change the behaviour of the existing function > > without also changing the rings "bulk" function to match. >=20 > Do you mean mempool_get_burst() that could return less number of object= s then you requested? > If so, I wonder what will be the usage model for it? > To me it sounds a bit strange - like malloc() (or mmap) that could allo= cate to you only part of the memory you requested? >=20 I would expect it to be as useful as the rings versions. :-) I don't actually see a problem with it. For example: if you have 10 messa= ges=20 you want to create and send, I see no reason why, if there were only 8=20 buffers free, you can't create and send 8 and then try again for the last= 2=20 once you were finished with those 8. That being said, the reason for suggesting that behaviour was to align wi= th=20 the rings APIs. If we are looking at breaking backward compatibility (for= =20 both rings and mempools) other options can be considered too. /Bruce > Konstantin >=20 > =20 > > /Bruce > >=20 > > > > > > > > NACK in summary. > > > > Konstantin > > > > > > Keith Wiles, Principal Technologist with CTO office, Wind River mob= ile 972-213-5533 > > >