From mboxrd@z Thu Jan 1 00:00:00 1970 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) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga101.fm.intel.com with ESMTP; 18 Mar 2016 03:18:26 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,354,1455004800"; d="scan'208";a="68898071" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.44]) by fmsmga004.fm.intel.com with SMTP; 18 Mar 2016 03:18:24 -0700 Received: by (sSMTP sendmail emulation); Fri, 18 Mar 2016 10:18:24 +0025 Date: Fri, 18 Mar 2016 10:18:24 +0000 From: Bruce Richardson To: Mauricio =?iso-8859-1?Q?V=E1squez?= Cc: Lazaros Koromilas , dev@dpdk.org 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-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Organization: Intel Shannon Ltd. User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH v2] ring: check for zero objects mc dequeue / mp enqueue X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 18 Mar 2016 10:18:27 -0000 On Thu, Mar 17, 2016 at 05:09:08PM +0100, Mauricio Vásquez wrote: > Hi Lazaros, > > On Thu, Mar 17, 2016 at 4:49 PM, Lazaros Koromilas > wrote: > > > Issuing a zero objects dequeue with a single consumer has no effect. > > Doing so with multiple consumers, can get more than one thread to succeed > > the compare-and-set operation and observe starvation or even deadlock in > > the while loop that checks for preceding dequeues. The problematic piece > > of code when n = 0: > > > > cons_next = cons_head + n; > > success = rte_atomic32_cmpset(&r->cons.head, cons_head, cons_next); > > > > 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, void * > > const *obj_table, > > uint32_t mask = r->prod.mask; > > int ret; > > > > + /* Avoid the unnecessary cmpset operation below, which is also > > + * potentially harmful when n equals 0. */ > > + if (n == 0) > > > > What about using unlikely here? > Unless there is a measurable performance increase by adding in likely/unlikely 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 unlikely leg of the code can be quite severe. For normal stuff, where the code nearly always goes one way in the branch but occasionally goes the other, the hardware branch predictors generally do a good enough job. Just my 2c. /Bruce