From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jianbo Liu Subject: Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(), dsb() Date: Thu, 9 Nov 2017 17:58:14 +0800 Message-ID: <20171109095813.GB27104@arm.com> References: <1510121832-16439-1-git-send-email-hejianet@gmail.com> <20171108102814.GA7552@bricha3-MOBL3.ger.corp.intel.com> <9086316b-c16b-c42b-2d85-9b01fa2f66e1@gmail.com> <028263d0-44de-bd0c-c495-081588a0ad20@gmail.com> <20171109032145.GA26939@arm.com> <20171109045601.GA27104@arm.com> <2601191342CEEE43887BDE71AB9772585FABB311@irsmsx105.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Cc: Jia He , "Richardson, Bruce" , "jerin.jacob@caviumnetworks.com" , "dev@dpdk.org" , "olivier.matz@6wind.com" , "hemant.agrawal@nxp.com" , "jia.he@hxt-semitech.com" To: "Ananyev, Konstantin" Return-path: Received: from EUR01-DB5-obe.outbound.protection.outlook.com (mail-db5eur01on0072.outbound.protection.outlook.com [104.47.2.72]) by dpdk.org (Postfix) with ESMTP id 795851B5E6 for ; Thu, 9 Nov 2017 10:59:31 +0100 (CET) Content-Disposition: inline In-Reply-To: <2601191342CEEE43887BDE71AB9772585FABB311@irsmsx105.ger.corp.intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" The 11/09/2017 09:38, Ananyev, Konstantin wrote: > > > > -----Original Message----- > > From: Jianbo Liu [mailto:jianbo.liu@arm.com] > > Sent: Thursday, November 9, 2017 4:56 AM > > To: Jia He > > Cc: Richardson, Bruce ; jerin.jacob@caviumn= etworks.com; dev@dpdk.org; olivier.matz@6wind.com; > > Ananyev, Konstantin ; hemant.agrawal@nxp.= com; jia.he@hxt-semitech.com > > Subject: Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(),dsb(= ) > > > > The 11/09/2017 12:43, Jia He wrote: > > > Hi Jianbo > > > > > > > > > On 11/9/2017 11:21 AM, Jianbo Liu Wrote: > > > >The 11/09/2017 11:14, Jia He wrote: > > > >> > > > >>On 11/9/2017 9:22 AM, Jia He Wrote: > > > >>>Hi Bruce > > > >>> > > > >>> > > > >>>On 11/8/2017 6:28 PM, Bruce Richardson Wrote: > > > >>>>On Wed, Nov 08, 2017 at 06:17:10AM +0000, Jia He wrote: > > > >>>>>for the code as follows: > > > >>>>>if (condition) > > > >>>>> rte_smp_rmb(); > > > >>>>>else > > > >>>>> rte_smp_wmb(); > > > >>>>>Without this patch, compiler will report this error: > > > >>>>>error: 'else' without a previous 'if' > > > >>>>> > > > >>>>>Signed-off-by: Jia He > > > >>>>>Signed-off-by: jia.he@hxt-semitech.com > > > >>>>>--- > > > >>>>> lib/librte_eal/common/include/arch/arm/rte_atomic_64.h | 4 ++= -- > > > >>>>> 1 file changed, 2 insertions(+), 2 deletions(-) > > > >>>>> > > > >>>>>diff --git > > > >>>>>a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h > > > >>>>>b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h > > > >>>>>index 0b70d62..38c3393 100644 > > > >>>>>--- a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h > > > >>>>>+++ b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h > > > >>>>>@@ -43,8 +43,8 @@ extern "C" { > > > >>>>> #include "generic/rte_atomic.h" > > > >>>>> -#define dsb(opt) { asm volatile("dsb " #opt : : : "memory")= ; } > > > >>>>>-#define dmb(opt) { asm volatile("dmb " #opt : : : "memory"); } > > > >>>>>+#define dsb(opt) asm volatile("dsb " #opt : : : "memory"); > > > >>>>>+#define dmb(opt) asm volatile("dmb " #opt : : : "memory"); > > > >>>>Need to remove the trailing ";" I too I think. > > > >>>>Alternatively, to keep the braces, the standard practice is to us= e > > > >>>>do { ... } while(0) > > > >>>If trailing ";" is not removed > > > >>>the code: > > > >>>if (condition) > > > >>> rte_smp_rmb(); > > > >>>else > > > >>> anything(); > > > >>> > > > >Sorry, why not use two different functions as your conditions passed= in > > > >are fixed in the calling functions. > > > Do you mean to split update_tail() into update_tail_enqueue() and > > > update_tail_dequeue()? > > > > Yes. So it's not need to change dsb/dmb. > > That's a good idea - but you still might hit the same problem in > Some different place in future... I think there is no any issue if we add {} for if/else sections. - if (enqueue) + if (enqueue) { rte_smp_wmb(); - else + } else { rte_smp_rmb(); + } It's not a good way to ommit {}. Jianbo > Why not to convert these macros into 'always_inline' functions then? > Konstantin > > > > > Jianbo > > IMPORTANT NOTICE: The contents of this email and any attachments are co= nfidential and may also be privileged. If you are not the > > intended recipient, please notify the sender immediately and do not dis= close the contents to any other person, use it for any purpose, or > > store or copy the information in any medium. Thank you. -- IMPORTANT NOTICE: The contents of this email and any attachments are confid= ential and may also be privileged. If you are not the intended recipient, p= lease notify the sender immediately and do not disclose the contents to any= other person, use it for any purpose, or store or copy the information in = any medium. Thank you.