From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jia He Subject: Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(), dsb() Date: Fri, 10 Nov 2017 10:06:14 +0800 Message-ID: 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; format=flowed Content-Transfer-Encoding: 7bit Cc: "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" , Jianbo Liu Return-path: Received: from mail-pg0-f66.google.com (mail-pg0-f66.google.com [74.125.83.66]) by dpdk.org (Postfix) with ESMTP id 4DE481B62C for ; Fri, 10 Nov 2017 03:06:23 +0100 (CET) Received: by mail-pg0-f66.google.com with SMTP id 207so3459418pgc.12 for ; Thu, 09 Nov 2017 18:06:23 -0800 (PST) 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" On 11/9/2017 5:38 PM, 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@caviumnetworks.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 use >>>>>>> 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... > Why not to convert these macros into 'always_inline' functions then? > Konstantin > It makes things more complex opt needs to be redefined with types such as : __attribute__((always_inline)) void dsb( char* opt) and the input paramenter shoud be #define sy "sy" #define ld "ld" And the "#" in asm codes needs to be considerred more. IMO, the kernel way is simple and clean, isn't it? #define dmb(opt) asm volatile("dmb " #opt : : : "memory") Another choice is adding the do/while. @Ananyev @Jianbo Any thoughts? -- Cheers, Jia