From mboxrd@z Thu Jan 1 00:00:00 1970 From: Neil Horman Subject: Re: [RFC] More changes for rte_mempool.h:__mempool_get_bulk() Date: Sun, 28 Sep 2014 16:42:33 -0400 Message-ID: <20140928204233.GA4012@localhost.localdomain> References: <3B9A624B-ABBF-4A20-96CD-8D5607006FEA@windriver.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Cc: "" To: "Wiles, Roger Keith" Return-path: Content-Disposition: inline In-Reply-To: <3B9A624B-ABBF-4A20-96CD-8D5607006FEA-CWA4WttNNZF54TAoqtyWWQ@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 Sun, Sep 28, 2014 at 05:52:16PM +0000, Wiles, Roger Keith wrote: > Here is a Request for Comment on __mempool_get_bulk() routine. I believ= e 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 handl= ed. >=20 > 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. >=20 ret =3D=3D req should be guaranteed. As documented, rte_ring_mc_dequeue_= bulk, when called with behavior =3D=3D FIXED (which it is internally), returns 0 iff= the entire request was satisfied, so we can safely add req. That said, I've not val= idated that it always does whats documented, but if it doesn't, the fix should l= ikely be internal to the function, not external to it. Neil > The second issue is if you believe the above code then we have to accou= nt for that issue in the stats. >=20 > Let me know what you think? > ++Keith > =E2=80=94=E2=80=94=E2=80=94 >=20 > 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 **o= bj_table, > unsigned n, int is_mc) > { > int ret; > -#ifdef RTE_LIBRTE_MEMPOOL_DEBUG > - unsigned n_orig =3D n; > -#endif > + > #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; > } > =20 > - cache->len +=3D req; > + cache->len +=3D ret; // Need to adjust len by ret = not req, as (ret !=3D req) > + > + if ( cache->len < n ) { > + /* > + * Number (ret + cache->len) may not be >=3D 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 !=3D 0 and less t= hen n), but the > + * normal case it should be OK. If the user nee= ds to preserve > + * the order of packets then he must set cache_= size =3D=3D 0. > + */ > + goto ring_dequeue; > + } > } > =20 > /* Now fill in the response ... */ > @@ -1002,9 +1014,12 @@ ring_dequeue: > ret =3D rte_ring_sc_dequeue_bulk(mp->ring, obj_table, n= ); > =20 > 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 should n= ot be a problem. > + __MEMPOOL_STAT_ADD(mp, get_fail, n - ret); > + } > =20 > return ret; > } >=20 > Keith Wiles, Principal Technologist with CTO office, Wind River mobile = 972-213-5533 >=20 >=20