[dpdk-dev] [PATCH 1/3] eal/arm64: remove the braces {} for dmb(), dsb()

[Ananyev, Konstantin]

Hi Jia,

> -----Original Message-----
> From: Jia He [mailto:hejianet at gmail.com]
> Sent: Friday, November 10, 2017 2:06 AM
> To: Ananyev, Konstantin <konstantin.ananyev at intel.com>; Jianbo Liu <jianbo.liu at arm.com>
> Cc: Richardson, Bruce <bruce.richardson at intel.com>; jerin.jacob at caviumnetworks.com; dev at dpdk.org; olivier.matz at 6wind.com;
> hemant.agrawal at nxp.com; jia.he at hxt-semitech.com
> Subject: Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(),dsb()
> 
> 
> 
> On 11/9/2017 5:38 PM, Ananyev, Konstantin Wrote:
> >
> >> -----Original Message-----
> >> From: Jianbo Liu [mailto:jianbo.liu at arm.com]
> >> Sent: Thursday, November 9, 2017 4:56 AM
> >> To: Jia He <hejianet at gmail.com>
> >> Cc: Richardson, Bruce <bruce.richardson at intel.com>; jerin.jacob at caviumnetworks.com; dev at dpdk.org; olivier.matz at 6wind.com;
> >> Ananyev, Konstantin <konstantin.ananyev at intel.com>; hemant.agrawal at nxp.com; jia.he at 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 <hejianet at gmail.com>
> >>>>>>>> Signed-off-by: jia.he at 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")

Fine by me.
Konstantin

> Another choice is adding the do/while.
> 
> @Ananyev @Jianbo
> Any thoughts?
> 
> --
> Cheers,
> Jia