From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bruce Richardson Subject: Re: [PATCH v2] ring: check for zero objects mc dequeue / mp enqueue Date: Fri, 18 Mar 2016 10:18:24 +0000 Message-ID: <20160318101823.GC4848@bricha3-MOBL3> References: <1458229783-15547-1-git-send-email-l@nofutznetworks.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable Cc: Lazaros Koromilas , dev@dpdk.org To: Mauricio =?iso-8859-1?Q?V=E1squez?= Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 81F8F558A for ; Fri, 18 Mar 2016 11:18:27 +0100 (CET) Content-Disposition: inline In-Reply-To: List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Thu, Mar 17, 2016 at 05:09:08PM +0100, Mauricio V=E1squez wrote: > Hi Lazaros, >=20 > On Thu, Mar 17, 2016 at 4:49 PM, Lazaros Koromilas > wrote: >=20 > > Issuing a zero objects dequeue with a single consumer has no effect. > > Doing so with multiple consumers, can get more than one thread to suc= ceed > > the compare-and-set operation and observe starvation or even deadlock= in > > the while loop that checks for preceding dequeues. The problematic p= iece > > of code when n =3D 0: > > > > cons_next =3D cons_head + n; > > success =3D rte_atomic32_cmpset(&r->cons.head, cons_head, cons_ne= xt); > > > > The same is possible on the enqueue path. > > > > Signed-off-by: Lazaros Koromilas > > --- > > lib/librte_ring/rte_ring.h | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h > > index 943c97c..eb45e41 100644 > > --- a/lib/librte_ring/rte_ring.h > > +++ b/lib/librte_ring/rte_ring.h > > @@ -431,6 +431,11 @@ __rte_ring_mp_do_enqueue(struct rte_ring *r, voi= d * > > const *obj_table, > > uint32_t mask =3D r->prod.mask; > > int ret; > > > > + /* Avoid the unnecessary cmpset operation below, which is als= o > > + * potentially harmful when n equals 0. */ > > + if (n =3D=3D 0) > > >=20 > What about using unlikely here? >=20 Unless there is a measurable performance increase by adding in likely/unl= ikely I'd suggest avoiding it's use. In general, likely/unlikely should only be= used for things like catestrophic errors because the penalty for taking the un= likely leg of the code can be quite severe. For normal stuff, where the code nea= rly always goes one way in the branch but occasionally goes the other, the ha= rdware branch predictors generally do a good enough job. Just my 2c. /Bruce