From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id BBCA82C09 for ; Mon, 21 Mar 2016 18:47:48 +0100 (CET) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga102.jf.intel.com with ESMTP; 21 Mar 2016 10:47:47 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,372,1455004800"; d="scan'208";a="673309236" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by FMSMGA003.fm.intel.com with ESMTP; 21 Mar 2016 10:47:47 -0700 Received: from fmsmsx154.amr.corp.intel.com (10.18.116.70) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.248.2; Mon, 21 Mar 2016 10:47:47 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX154.amr.corp.intel.com (10.18.116.70) with Microsoft SMTP Server (TLS) id 14.3.248.2; Mon, 21 Mar 2016 10:47:46 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.136]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.18]) with mapi id 14.03.0248.002; Tue, 22 Mar 2016 01:47:45 +0800 From: "Xie, Huawei" To: "Richardson, Bruce" , =?iso-8859-1?Q?Mauricio_V=E1squez?= CC: Thomas Monjalon , "dev@dpdk.org" , Olivier Matz , Lazaros Koromilas Thread-Topic: [dpdk-dev] [PATCH v2] ring: check for zero objects mc dequeue / mp enqueue Thread-Index: AdGDmcX2Ii7y6eQHTwq9Vl/aYA88eQ== Date: Mon, 21 Mar 2016 17:47:44 +0000 Message-ID: References: <1458229783-15547-1-git-send-email-l@nofutznetworks.com> <20160318101823.GC4848@bricha3-MOBL3> <56EBD806.8010707@6wind.com> <17186869.jQBbCLbaVI@xps13> <20160318141632.GC12932@bricha3-MOBL3> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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: Mon, 21 Mar 2016 17:47:49 -0000 On 3/18/2016 10:17 PM, Bruce Richardson wrote:=0A= > On Fri, Mar 18, 2016 at 01:47:29PM +0100, Mauricio V=E1squez wrote:=0A= >> Hi,=0A= >>=0A= >>=0A= >> On Fri, Mar 18, 2016 at 11:35 AM, Thomas Monjalon >> wrote:=0A= >>> 2016-03-18 11:27, Olivier Matz:=0A= >>>> On 03/18/2016 11:18 AM, Bruce Richardson wrote:=0A= >>>>>>> + /* Avoid the unnecessary cmpset operation below, which is= =0A= >>> also=0A= >>>>>>> + * potentially harmful when n equals 0. */=0A= >>>>>>> + if (n =3D=3D 0)=0A= >>>>>>>=0A= >>>>>> What about using unlikely here?=0A= >>>>>>=0A= >>>>> Unless there is a measurable performance increase by adding in=0A= >>> likely/unlikely=0A= >>>>> I'd suggest avoiding it's use. In general, likely/unlikely should onl= y=0A= >>> be used=0A= >>>>> for things like catestrophic errors because the penalty for taking th= e=0A= >>> unlikely=0A= >>>>> leg of the code can be quite severe. For normal stuff, where the code= =0A= >>> nearly=0A= >>>>> always goes one way in the branch but occasionally goes the other, th= e=0A= >>> hardware=0A= >>>>> branch predictors generally do a good enough job.=0A= >>>> Do you mean using likely/unlikely could be worst than not using it=0A= >>>> in this case?=0A= >>>>=0A= >>>> To me, using unlikely here is not a bad idea: it shows to the compiler= =0A= >>>> and to the reader of the code that is case is not the usual case.=0A= >>> It would be nice to have a guideline section about likely/unlikely in= =0A= >>> doc/guides/contributing/design.rst=0A= >>>=0A= >>> Bruce gave a talk at Dublin about this kind of things.=0A= >>> I'm sure he could contribute more design guidelines ;)=0A= >>>=0A= >> There is a small explanation in the section "Branch Prediction" of=0A= >> doc/guides/contributing/coding_style.rst, but I do not know if that is= =0A= >> enough to understand when to use them.=0A= >>=0A= >> I've made a fast check and there are many PMDs that use them to check if= =0A= >> number of packets is zero in the transmission function.=0A= > Yeah, and I wonder how many of those are actually necessary too :-)=0A= >=0A= > It's not a big deal either way, I just think the patch is fine as-is with= out=0A= > the extra macros.=0A= =0A= IMO we use likely/unlikely in two cases, catastrophic errors and the=0A= code nearly always goes one way, i.e, preferred/favored fast path.=0A= Likely/unlikely helps not only for branch predication but also for cache=0A= usage. The code generated for the likely path will directly follow the=0A= branch instruction. To me, it is reasonable enough to add unlikely for n=0A= =3D=3D 0, which we don't expect to happen.=0A= I remember with/without likely, compiler could generate three kind of=0A= instructions. Didn't deep dive into it.=0A= =0A= >=0A= > /Bruce=0A= >=0A= =0A=