From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id 22A094CA7 for ; Sun, 28 Oct 2018 00:00:20 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Oct 2018 15:00:19 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,433,1534834800"; d="scan'208";a="81427885" Received: from irsmsx152.ger.corp.intel.com ([163.33.192.66]) by fmsmga007.fm.intel.com with ESMTP; 27 Oct 2018 15:00:18 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.144]) by IRSMSX152.ger.corp.intel.com ([169.254.6.64]) with mapi id 14.03.0415.000; Sat, 27 Oct 2018 23:00:17 +0100 From: "Ananyev, Konstantin" To: Malvika Gupta CC: "dev@dpdk.org" , "Gavin Hu (Arm Technology China)" , Honnappa Nagarahalli , nd Thread-Topic: [PATCH] test/bpf: use hton instead of __builtin_bswap Thread-Index: AQHUPxBTKpHBSGr7KUCFrtgAI8miMaTgLLtQgAIHVwCACs7bAIA5wb4AgAm0v4CAA4hBgA== Date: Sat, 27 Oct 2018 22:00:16 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725801030618AD@irsmsx105.ger.corp.intel.com> References: <20180828204620.1862-1-Malvika.Gupta@arm.com> <2601191342CEEE43887BDE71AB977258EA9511E7@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772580102FE9DA4@IRSMSX106.ger.corp.intel.com> In-Reply-To: Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiY2FhN2Q3OWUtYmVkZi00NzJmLThiMWEtYzExNWMzZTNiZjgyIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiZmhKVHppQWQ5aERZV1N4U0E3U2F2Rk5ISTFvaW83WW0xdkx6U1wvWGNmdjRPXC9pTmxTNUJibkh3MkcxaCtSNXo2In0= x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] test/bpf: use hton instead of __builtin_bswap X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 27 Oct 2018 22:00:21 -0000 >=20 > Hi Konstantin, >=20 > At time of upstreaming the patch, I had compiled it on Cortex-A72 and it = had worked with the -I parameter. Just curious what is memory ordering on your box (BE/LE)? > But when tried building it on > x86-64, I encountered the error that you were facing. From my understandi= ng of this problem, this is a portability issue. As you pointed out, > gcc and clang implement __builtin_constant_p in a different way on x86 bu= t that may not be true for ARM. >=20 > In such a case, to enable compilation on x86, we may want to write specif= ic hton and ntoh functions. But this would become a tedious task > because, in future, to enable compilation on other architectures besides = x86 and ARM, we may have to repeat this process of writing > analogs for each architecture. Don't see why we need it done for each arch. Obviously all is needed is BE and LE version. > I think the best solution in the current scenario would be to use the -U = parameter to enable compilation on > x86 and later decide if we want to write a new patch with analogs for bui= ltin hton() and ntoh(). I suppose -U__GNUC__ is ok. If you decide to go that way, please update appropriate comments in *.c. Konstantin >=20 > Best, > Malvika >=20 > -----Original Message----- > From: Ananyev, Konstantin > Sent: Friday, October 19, 2018 7:17 AM > To: Malvika Gupta > Cc: dev@dpdk.org; Gavin Hu (Arm Technology China) ; Hon= nappa Nagarahalli ; > nd > Subject: RE: [PATCH] test/bpf: use hton instead of __builtin_bswap >=20 >=20 > Hi Malvika, >=20 > > > > Hi Konstantin, > > > > I installed the clang version 4.0.1 to check for the issue you were > > facing with -O2 compilation. I was able to compile with -O2 and -O0 opt= imization without any errors. Please see the exact command I > used and the following output for your reference: > > > > $ clang -O2 -target bpf -I /usr/include/aarch64-linux-gnu/ -c t1.c $ > > clang -O0 -target bpf -I /usr/include/aarch64-linux-gnu/ -c t1.c $ > > clang -v clang version 4.0.1-10 (tags/RELEASE_401/final) > > Target: aarch64-unknown-linux-gnu > > Thread model: posix > > InstalledDir: /usr/bin > > Found candidate GCC installation: > > /usr/bin/../lib/gcc/aarch64-linux-gnu/6 > > Found candidate GCC installation: > > /usr/bin/../lib/gcc/aarch64-linux-gnu/6.4.0 > > Found candidate GCC installation: > > /usr/bin/../lib/gcc/aarch64-linux-gnu/7 > > Found candidate GCC installation: > > /usr/bin/../lib/gcc/aarch64-linux-gnu/7.3.0 > > Found candidate GCC installation: > > /usr/bin/../lib/gcc/aarch64-linux-gnu/8 > > Found candidate GCC installation: > > /usr/bin/../lib/gcc/aarch64-linux-gnu/8.0.1 > > Found candidate GCC installation: /usr/lib/gcc/aarch64-linux-gnu/6 > > Found candidate GCC installation: /usr/lib/gcc/aarch64-linux-gnu/6.4.0 > > Found candidate GCC installation: /usr/lib/gcc/aarch64-linux-gnu/7 > > Found candidate GCC installation: /usr/lib/gcc/aarch64-linux-gnu/7.3.0 > > Found candidate GCC installation: /usr/lib/gcc/aarch64-linux-gnu/8 > > Found candidate GCC installation: /usr/lib/gcc/aarch64-linux-gnu/8.0.1 > > Selected GCC installation: /usr/bin/../lib/gcc/aarch64-linux-gnu/7.3.0 > > Candidate multilib: .;@m64 > > Selected multilib: .;@m64 > > > > I hope this was helpful to you. >=20 > Sorry for delay. > I tried with clang 6.0, but still seeing same issue. > Digging a bit more, I think the culprit is here: >=20 > #ifdef __GNUC__ > # if __GNUC__ >=3D 2 > # define __bswap_16(x) \ > (__extension__ = \ > ({ unsigned short int __v, __x =3D (unsigned short int) (x); = \ > if (__builtin_constant_p (__x)) = \ <------ evaluates to FALSE !!! > __v =3D __bswap_constant_16 (__x); = \ > else = \ > __asm__ ("rorw $8, %w0" = \ > : "=3Dr" (__v) = \ > : "0" (__x) = \ > : "cc"); = \ > __v; })) > # else > /* This is better than nothing. */ > # define __bswap_16(x) \ > (__extension__ = \ > ({ unsigned short int __x =3D (unsigned short int) (x); = \ > __bswap_constant_16 (__x); })) > # endif > #else > static __inline unsigned short int > __bswap_16 (unsigned short int __bsx) > { > return __bswap_constant_16 (__bsx); > } > #endif >=20 > Seems that gcc and clang implements __builtin_constant_p in a different w= ay. > Let say, the following function: > int bbb1(void) {uint16_t x=3D0; return__bswap_constant_16(x);} when compi= led with gcc would return 1, while when compiled with clang - 0. > Adding 'const' to definition of 'x' cures the problem for clang. > I don't know why there is no 'const' in original __bswap_16() macro. > To overcome that problem, I used: > clang -O2 -U__GNUC__ -target bpf -c t1.c >=20 > Another possible option - define our own analogs for buitin hton*(). > Konstantin >=20 >=20 > > Best Regards, > > Malvika Gupta > > > > > > -----Original Message----- > > From: Malvika Gupta > > Sent: Wednesday, September 5, 2018 4:43 PM > > To: Ananyev, Konstantin > > Cc: dev@dpdk.org; Gavin Hu (Arm Technology China) ; > > Honnappa Nagarahalli ; Brian Brooks > > ; nd > > Subject: RE: [PATCH] test/bpf: use hton instead of __builtin_bswap > > > > Hi Ananyev, > > > > I used clang version 6.0.0. Please see the following output for your re= ference. > > > > $ clang -v > > clang version 6.0.0-1ubuntu2 (tags/RELEASE_600/final) > > Target: aarch64-unknown-linux-gnu > > Thread model: posix > > InstalledDir: /usr/bin > > Found candidate GCC installation: > > /usr/bin/../lib/gcc/aarch64-linux-gnu/6 > > Found candidate GCC installation: > > /usr/bin/../lib/gcc/aarch64-linux-gnu/6.4.0 > > Found candidate GCC installation: > > /usr/bin/../lib/gcc/aarch64-linux-gnu/7 > > Found candidate GCC installation: > > /usr/bin/../lib/gcc/aarch64-linux-gnu/7.3.0 > > Found candidate GCC installation: > > /usr/bin/../lib/gcc/aarch64-linux-gnu/8 > > Found candidate GCC installation: > > /usr/bin/../lib/gcc/aarch64-linux-gnu/8.0.1 > > Found candidate GCC installation: /usr/lib/gcc/aarch64-linux-gnu/6 > > Found candidate GCC installation: /usr/lib/gcc/aarch64-linux-gnu/6.4.0 > > Found candidate GCC installation: /usr/lib/gcc/aarch64-linux-gnu/7 > > Found candidate GCC installation: /usr/lib/gcc/aarch64-linux-gnu/7.3.0 > > Found candidate GCC installation: /usr/lib/gcc/aarch64-linux-gnu/8 > > Found candidate GCC installation: /usr/lib/gcc/aarch64-linux-gnu/8.0.1 > > Selected GCC installation: /usr/bin/../lib/gcc/aarch64-linux-gnu/7.3.0 > > Candidate multilib: .;@m64 > > Selected multilib: .;@m64 > > > > Also, the code compiles with both -O2 and -O0 for me. > > > > I hope this was helpful > > Best, > > Malvika > > > > -----Original Message----- > > From: Ananyev, Konstantin > > Sent: Tuesday, September 4, 2018 8:56 AM > > To: Malvika Gupta > > Cc: dev@dpdk.org; Gavin Hu (Arm Technology China) ; > > Honnappa Nagarahalli ; Brian Brooks > > ; nd > > Subject: RE: [PATCH] test/bpf: use hton instead of __builtin_bswap > > > > Hi, > > > > > > > > Convert host machine endianness to networking endianness for > > > comparison of incoming packets with BPF filter > > > > > > > > > Signed-off-by: Malvika Gupta > > > Reviewed-by: Gavin Hu > > > Reviewed-by: Brian Brooks > > > Suggested-by: Brian Brooks > > > --- > > > test/bpf/t1.c | 7 ++++--- > > > test/bpf/t3.c | 3 ++- > > > 2 files changed, 6 insertions(+), 4 deletions(-) > > > > > > diff --git a/test/bpf/t1.c b/test/bpf/t1.c index > > > 60f9434ab..7943fcf34 > > > 100644 > > > --- a/test/bpf/t1.c > > > +++ b/test/bpf/t1.c > > > @@ -28,24 +28,25 @@ > > > #include > > > #include > > > #include > > > +#include > > > > > > uint64_t > > > entry(void *pkt) > > > { > > > struct ether_header *ether_header =3D (void *)pkt; > > > > > > - if (ether_header->ether_type !=3D __builtin_bswap16(0x0800)) > > > + if (ether_header->ether_type !=3D htons(0x0800)) > > > > Which version of clang do you use? > > With my one I get: > > $ clang -O2 -target bpf -c t1.c > > t1.c:37:34: error: couldn't allocate output register for constraint 'r' > > if (ether_header->ether_type !=3D ntohs(0x0800)) > > ^ > > /usr/include/netinet/in.h:402:21: note: expanded from macro 'ntohs' > > # define ntohs(x) __bswap_16 (x) > > ^ > > /usr/include/bits/byteswap-16.h:31:14: note: expanded from macro '__bsw= ap_16' > > __asm__ ("rorw $8, %w0" > > > > With '-O0' it compiles ok. > > > > $ clang -v > > clang version 4.0.1 (tags/RELEASE_401/final) > > Target: x86_64-unknown-linux-gnu > > Thread model: posix > > InstalledDir: /usr/bin > > Found candidate GCC installation: > > /usr/bin/../lib/gcc/x86_64-redhat-linux/7 > > Found candidate GCC installation: /usr/lib/gcc/x86_64-redhat-linux/7 > > Selected GCC installation: /usr/bin/../lib/gcc/x86_64-redhat-linux/7 > > Candidate multilib: .;@m64 > > Candidate multilib: 32;@m32 > > Selected multilib: .;@m64 > > > > Konstantin > > > > > return 0; > > > > > > struct iphdr *iphdr =3D (void *)(ether_header + 1); > > > if (iphdr->protocol !=3D 17 || (iphdr->frag_off & 0x1ffff) !=3D 0 |= | > > > - iphdr->daddr !=3D __builtin_bswap32(0x1020304)) > > > + iphdr->daddr !=3D htonl(0x1020304)) > > > return 0; > > > > > > int hlen =3D iphdr->ihl * 4; > > > struct udphdr *udphdr =3D (void *)iphdr + hlen; > > > > > > - if (udphdr->dest !=3D __builtin_bswap16(5000)) > > > + if (udphdr->dest !=3D htons(5000)) > > > return 0; > > > > > > return 1; > > > diff --git a/test/bpf/t3.c b/test/bpf/t3.c index > > > 531b9cb8c..24298b7c7 > > > 100644 > > > --- a/test/bpf/t3.c > > > +++ b/test/bpf/t3.c > > > @@ -17,6 +17,7 @@ > > > #include > > > #include > > > #include "mbuf.h" > > > +#include > > > > > > extern void rte_pktmbuf_dump(FILE *, const struct rte_mbuf *, > > > unsigned int); > > > > > > @@ -29,7 +30,7 @@ entry(const void *pkt) > > > mb =3D pkt; > > > eth =3D rte_pktmbuf_mtod(mb, const struct ether_header *); > > > > > > - if (eth->ether_type =3D=3D __builtin_bswap16(ETHERTYPE_ARP)) > > > + if (eth->ether_type =3D=3D htons(ETHERTYPE_ARP)) > > > rte_pktmbuf_dump(stdout, mb, 64); > > > > > > return 1; > > > -- > > > 2.17.1