> -----Original Message----- > From: Jianbo Liu [mailto:jianbo....@arm.com] > Sent: Thursday, November 9, 2017 9:58 AM > To: Ananyev, Konstantin <konstantin.anan...@intel.com> > Cc: Jia He <hejia...@gmail.com>; Richardson, Bruce > <bruce.richard...@intel.com>; jerin.ja...@caviumnetworks.com; dev@dpdk.org; > olivier.m...@6wind.com; hemant.agra...@nxp.com; jia...@hxt-semitech.com > Subject: Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(),dsb() > > The 11/09/2017 09:38, Ananyev, Konstantin wrote: > > > > > > > -----Original Message----- > > > From: Jianbo Liu [mailto:jianbo....@arm.com] > > > Sent: Thursday, November 9, 2017 4:56 AM > > > To: Jia He <hejia...@gmail.com> > > > Cc: Richardson, Bruce <bruce.richard...@intel.com>; > > > jerin.ja...@caviumnetworks.com; dev@dpdk.org; olivier.m...@6wind.com; > > > Ananyev, Konstantin <konstantin.anan...@intel.com>; > > > hemant.agra...@nxp.com; jia...@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 <hejia...@gmail.com> > > > > >>>>>Signed-off-by: jia...@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... > > 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 {}.
That's the preferred DPDK coding style these days :) http://dpdk.org/doc/guides-16.04/contributing/coding_style.html 1.6.2. Control Statements and Loops Include a space after keywords (if, while, for, return, switch). Do not use braces ({ and }) for control statements with zero or just a single statement, unless that statement is more than a single line in which case the braces are permitted. Konstantin > > 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 > > > confidential and may also be privileged. If you are not the > > > intended recipient, please 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. > > -- > IMPORTANT NOTICE: The contents of this email and any attachments are > confidential and may also be privileged. If you are not the > intended recipient, please 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.
