* Re: [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error [not found] ` <1417606099-3489-1-git-send-email-michael.qiu@intel.com> @ 2014-12-03 11:42 ` Bruce Richardson 2014-12-03 13:59 ` Qiu, Michael ` (2 more replies) 0 siblings, 3 replies; 32+ messages in thread From: Bruce Richardson @ 2014-12-03 11:42 UTC (permalink / raw) To: Michael Qiu; +Cc: dev On Wed, Dec 03, 2014 at 07:28:19PM +0800, Michael Qiu wrote: > app/test-pmd/csumonly.c: In function ‘get_psd_sum’: > build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’ > does break strict-aliasing rules > build/include/rte_ip.h:157: note: initialized from here > ... > > The root cause is that, compile enable strict aliasing by default, > while in function rte_raw_cksum() try to convert 'const char *' > to 'const uint16_t *'. > What compiler version is this with? Is there any other way to fix this other than disabling the compiler warnings. Turning off strict aliasing may affect performance as it reduces the number of optimizations that the compiler can perform. /Bruce > Need to add CFLAG '-Wno-strict-aliasing' to avoid this issue. > > Signed-off-by: Michael Qiu <michael.qiu@intel.com> > --- > app/test-pmd/Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/app/test-pmd/Makefile b/app/test-pmd/Makefile > index 97dc2e6..995c874 100644 > --- a/app/test-pmd/Makefile > +++ b/app/test-pmd/Makefile > @@ -38,7 +38,7 @@ ifeq ($(CONFIG_RTE_TEST_PMD),y) > # > APP = testpmd > > -CFLAGS += -O3 > +CFLAGS += -O3 -Wno-strict-aliasing > CFLAGS += $(WERROR_FLAGS) > > ifeq ($(CONFIG_RTE_LIBRTE_PMD_PCAP),y) > -- > 1.9.3 > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error 2014-12-03 11:42 ` [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error Bruce Richardson @ 2014-12-03 13:59 ` Qiu, Michael 2014-12-03 14:51 ` Bruce Richardson 2014-12-03 15:24 ` [dpdk-dev] [PATCH] " Olivier MATZ 2014-12-03 15:57 ` Dayu Qiu 2 siblings, 1 reply; 32+ messages in thread From: Qiu, Michael @ 2014-12-03 13:59 UTC (permalink / raw) To: Richardson, Bruce, Michael Qiu; +Cc: dev On 2014/12/3 19:43, Richardson, Bruce wrote: > On Wed, Dec 03, 2014 at 07:28:19PM +0800, Michael Qiu wrote: >> app/test-pmd/csumonly.c: In function ‘get_psd_sum’: >> build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’ >> does break strict-aliasing rules >> build/include/rte_ip.h:157: note: initialized from here >> ... >> >> The root cause is that, compile enable strict aliasing by default, >> while in function rte_raw_cksum() try to convert 'const char *' >> to 'const uint16_t *'. >> > What compiler version is this with? Is there any other way to fix this > other than disabling the compiler warnings. Turning off strict aliasing may > affect performance as it reduces the number of optimizations that the compiler > can perform. The compile version is: $ gcc -v Using built-in specs. Target: x86_64-redhat-linux ... gcc version 4.4.7 20120313 (Red Hat 4.4.7-4) (GCC) The OS is centos6.5 x86_64 Actually, another possible solution is, as gcc manual shows, use union. In function rte_raw_cksum() of lib/librte_net/rte_ip.h: static inline uint16_t rte_raw_cksum(const char *buf, size_t len){ union { const char *ubuf; const uint16_t *uu16; } convert; convert.ubuf = buf; const uint16_t *u16 = convert.uu16; ... } This may be work, but not test yet. Any comments about this solution? Thanks, Michael > > /Bruce > >> Need to add CFLAG '-Wno-strict-aliasing' to avoid this issue. >> >> Signed-off-by: Michael Qiu <michael.qiu@intel.com> >> --- >> app/test-pmd/Makefile | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/app/test-pmd/Makefile b/app/test-pmd/Makefile >> index 97dc2e6..995c874 100644 >> --- a/app/test-pmd/Makefile >> +++ b/app/test-pmd/Makefile >> @@ -38,7 +38,7 @@ ifeq ($(CONFIG_RTE_TEST_PMD),y) >> # >> APP = testpmd >> >> -CFLAGS += -O3 >> +CFLAGS += -O3 -Wno-strict-aliasing >> CFLAGS += $(WERROR_FLAGS) >> >> ifeq ($(CONFIG_RTE_LIBRTE_PMD_PCAP),y) >> -- >> 1.9.3 >> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error 2014-12-03 13:59 ` Qiu, Michael @ 2014-12-03 14:51 ` Bruce Richardson 2014-12-03 15:19 ` Qiu, Michael 0 siblings, 1 reply; 32+ messages in thread From: Bruce Richardson @ 2014-12-03 14:51 UTC (permalink / raw) To: Qiu, Michael; +Cc: dev, Michael Qiu On Wed, Dec 03, 2014 at 01:59:58PM +0000, Qiu, Michael wrote: > On 2014/12/3 19:43, Richardson, Bruce wrote: > > On Wed, Dec 03, 2014 at 07:28:19PM +0800, Michael Qiu wrote: > >> app/test-pmd/csumonly.c: In function ‘get_psd_sum’: > >> build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’ > >> does break strict-aliasing rules > >> build/include/rte_ip.h:157: note: initialized from here > >> ... > >> > >> The root cause is that, compile enable strict aliasing by default, > >> while in function rte_raw_cksum() try to convert 'const char *' > >> to 'const uint16_t *'. > >> > > What compiler version is this with? Is there any other way to fix this > > other than disabling the compiler warnings. Turning off strict aliasing may > > affect performance as it reduces the number of optimizations that the compiler > > can perform. > > The compile version is: > > $ gcc -v > Using built-in specs. > Target: x86_64-redhat-linux > ... > gcc version 4.4.7 20120313 (Red Hat 4.4.7-4) (GCC) > > > The OS is centos6.5 x86_64 > > > Actually, another possible solution is, as gcc manual shows, use union. > In function rte_raw_cksum() of lib/librte_net/rte_ip.h: > > static inline uint16_t > rte_raw_cksum(const char *buf, size_t len){ > union { > const char *ubuf; > const uint16_t *uu16; > } convert; > > convert.ubuf = buf; > const uint16_t *u16 = convert.uu16; > ... > } > > This may be work, but not test yet. > > Any comments about this solution? what happens if you make rte_raw_cksum take a void * (or const void *) parameter instead of "const char *"? /Bruce ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error 2014-12-03 14:51 ` Bruce Richardson @ 2014-12-03 15:19 ` Qiu, Michael 2014-12-03 15:36 ` Bruce Richardson 0 siblings, 1 reply; 32+ messages in thread From: Qiu, Michael @ 2014-12-03 15:19 UTC (permalink / raw) To: Richardson, Bruce; +Cc: dev, Michael Qiu On 2014/12/3 22:51, Richardson, Bruce wrote: > On Wed, Dec 03, 2014 at 01:59:58PM +0000, Qiu, Michael wrote: >> On 2014/12/3 19:43, Richardson, Bruce wrote: >>> On Wed, Dec 03, 2014 at 07:28:19PM +0800, Michael Qiu wrote: >>>> app/test-pmd/csumonly.c: In function ‘get_psd_sum’: >>>> build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’ >>>> does break strict-aliasing rules >>>> build/include/rte_ip.h:157: note: initialized from here >>>> ... >>>> >>>> The root cause is that, compile enable strict aliasing by default, >>>> while in function rte_raw_cksum() try to convert 'const char *' >>>> to 'const uint16_t *'. >>>> >>> What compiler version is this with? Is there any other way to fix this >>> other than disabling the compiler warnings. Turning off strict aliasing may >>> affect performance as it reduces the number of optimizations that the compiler >>> can perform. >> The compile version is: >> >> $ gcc -v >> Using built-in specs. >> Target: x86_64-redhat-linux >> ... >> gcc version 4.4.7 20120313 (Red Hat 4.4.7-4) (GCC) >> >> >> The OS is centos6.5 x86_64 >> >> >> Actually, another possible solution is, as gcc manual shows, use union. >> In function rte_raw_cksum() of lib/librte_net/rte_ip.h: >> >> static inline uint16_t >> rte_raw_cksum(const char *buf, size_t len){ >> union { >> const char *ubuf; >> const uint16_t *uu16; >> } convert; >> >> convert.ubuf = buf; >> const uint16_t *u16 = convert.uu16; >> ... >> } >> >> This may be work, but not test yet. >> >> Any comments about this solution? > what happens if you make rte_raw_cksum take a void * (or const void *) parameter > instead of "const char *"? "size_t len" is for type char, it is the the array size(for char array is byte numbers), if we use void *, the meaning maybe confuse I think. But it should work with other code change. Thanks, Michael > > /Bruce > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error 2014-12-03 15:19 ` Qiu, Michael @ 2014-12-03 15:36 ` Bruce Richardson 2014-12-04 2:38 ` Qiu, Michael 0 siblings, 1 reply; 32+ messages in thread From: Bruce Richardson @ 2014-12-03 15:36 UTC (permalink / raw) To: Qiu, Michael; +Cc: dev, Michael Qiu On Wed, Dec 03, 2014 at 03:19:34PM +0000, Qiu, Michael wrote: > On 2014/12/3 22:51, Richardson, Bruce wrote: > > On Wed, Dec 03, 2014 at 01:59:58PM +0000, Qiu, Michael wrote: > >> On 2014/12/3 19:43, Richardson, Bruce wrote: > >>> On Wed, Dec 03, 2014 at 07:28:19PM +0800, Michael Qiu wrote: > >>>> app/test-pmd/csumonly.c: In function ‘get_psd_sum’: > >>>> build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’ > >>>> does break strict-aliasing rules > >>>> build/include/rte_ip.h:157: note: initialized from here > >>>> ... > >>>> > >>>> The root cause is that, compile enable strict aliasing by default, > >>>> while in function rte_raw_cksum() try to convert 'const char *' > >>>> to 'const uint16_t *'. > >>>> > >>> What compiler version is this with? Is there any other way to fix this > >>> other than disabling the compiler warnings. Turning off strict aliasing may > >>> affect performance as it reduces the number of optimizations that the compiler > >>> can perform. > >> The compile version is: > >> > >> $ gcc -v > >> Using built-in specs. > >> Target: x86_64-redhat-linux > >> ... > >> gcc version 4.4.7 20120313 (Red Hat 4.4.7-4) (GCC) > >> > >> > >> The OS is centos6.5 x86_64 > >> > >> > >> Actually, another possible solution is, as gcc manual shows, use union. > >> In function rte_raw_cksum() of lib/librte_net/rte_ip.h: > >> > >> static inline uint16_t > >> rte_raw_cksum(const char *buf, size_t len){ > >> union { > >> const char *ubuf; > >> const uint16_t *uu16; > >> } convert; > >> > >> convert.ubuf = buf; > >> const uint16_t *u16 = convert.uu16; > >> ... > >> } > >> > >> This may be work, but not test yet. > >> > >> Any comments about this solution? > > what happens if you make rte_raw_cksum take a void * (or const void *) parameter > > instead of "const char *"? > > "size_t len" is for type char, it is the the array size(for char array > is byte numbers), if we use void *, the meaning maybe confuse I think. That shouldn't be a big issue. We can rename it to "size" instead of "len" if needed. > But it should work with other code change. > > Thanks, > Michael > > > > /Bruce > > > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error 2014-12-03 15:36 ` Bruce Richardson @ 2014-12-04 2:38 ` Qiu, Michael 2014-12-04 3:28 ` [dpdk-dev] [PATCH v2] " Michael Qiu 0 siblings, 1 reply; 32+ messages in thread From: Qiu, Michael @ 2014-12-04 2:38 UTC (permalink / raw) To: Richardson, Bruce; +Cc: dev, Michael Qiu On 12/3/2014 11:36 PM, Richardson, Bruce wrote: > On Wed, Dec 03, 2014 at 03:19:34PM +0000, Qiu, Michael wrote: >> On 2014/12/3 22:51, Richardson, Bruce wrote: >>> On Wed, Dec 03, 2014 at 01:59:58PM +0000, Qiu, Michael wrote: >>>> On 2014/12/3 19:43, Richardson, Bruce wrote: >>>>> On Wed, Dec 03, 2014 at 07:28:19PM +0800, Michael Qiu wrote: >>>>>> app/test-pmd/csumonly.c: In function ‘get_psd_sum’: >>>>>> build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’ >>>>>> does break strict-aliasing rules >>>>>> build/include/rte_ip.h:157: note: initialized from here >>>>>> ... >>>>>> >>>>>> The root cause is that, compile enable strict aliasing by default, >>>>>> while in function rte_raw_cksum() try to convert 'const char *' >>>>>> to 'const uint16_t *'. >>>>>> >>>>> What compiler version is this with? Is there any other way to fix this >>>>> other than disabling the compiler warnings. Turning off strict aliasing may >>>>> affect performance as it reduces the number of optimizations that the compiler >>>>> can perform. >>>> The compile version is: >>>> >>>> $ gcc -v >>>> Using built-in specs. >>>> Target: x86_64-redhat-linux >>>> ... >>>> gcc version 4.4.7 20120313 (Red Hat 4.4.7-4) (GCC) >>>> >>>> >>>> The OS is centos6.5 x86_64 >>>> >>>> >>>> Actually, another possible solution is, as gcc manual shows, use union. >>>> In function rte_raw_cksum() of lib/librte_net/rte_ip.h: >>>> >>>> static inline uint16_t >>>> rte_raw_cksum(const char *buf, size_t len){ >>>> union { >>>> const char *ubuf; >>>> const uint16_t *uu16; >>>> } convert; >>>> >>>> convert.ubuf = buf; >>>> const uint16_t *u16 = convert.uu16; >>>> ... >>>> } >>>> >>>> This may be work, but not test yet. >>>> >>>> Any comments about this solution? >>> what happens if you make rte_raw_cksum take a void * (or const void *) parameter >>> instead of "const char *"? >> "size_t len" is for type char, it is the the array size(for char array >> is byte numbers), if we use void *, the meaning maybe confuse I think. > That shouldn't be a big issue. We can rename it to "size" instead of "len" if > needed. As I tested in my env just, void * is still not work, the same errors post when compile. Then the solution for DPDK should be: - As Olivier suggest, add the -Wno-strict-aliasing only for gcc 4.4 in the Makefile - As gcc manual suggest, use "union" to work around this issue. Thanks, Michael >> But it should work with other code change. >> >> Thanks, >> Michael >>> /Bruce >>> ^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH v2] test-pmd: Fix pointer aliasing error 2014-12-04 2:38 ` Qiu, Michael @ 2014-12-04 3:28 ` Michael Qiu 2014-12-04 4:16 ` [dpdk-dev] [PATCH v3] " Michael Qiu 2014-12-04 12:54 ` [dpdk-dev] [PATCH v2] " Ananyev, Konstantin 0 siblings, 2 replies; 32+ messages in thread From: Michael Qiu @ 2014-12-04 3:28 UTC (permalink / raw) To: dev app/test-pmd/csumonly.c: In function ‘get_psd_sum’: build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’ does break strict-aliasing rules build/include/rte_ip.h:157: note: initialized from here ... The root cause is that, compile enable strict aliasing by default, while in function rte_raw_cksum() try to convert 'const char *' to 'const uint16_t *'. This patch is one workaround fix. Signed-off-by: Michael Qiu <michael.qiu@intel.com> --- lib/librte_net/rte_ip.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h index 61e4457..f1c7087 100644 --- a/lib/librte_net/rte_ip.h +++ b/lib/librte_net/rte_ip.h @@ -154,7 +154,8 @@ struct ipv4_hdr { static inline uint16_t rte_raw_cksum(const char *buf, size_t len) { - const uint16_t *u16 = (const uint16_t *)buf; + unsigned long ptr = (unsigned long)buf; + const uint16_t *u16 = (const uint16_t *)ptr; uint32_t sum = 0; while (len >= (sizeof(*u16) * 4)) { -- 1.9.3 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH v3] test-pmd: Fix pointer aliasing error 2014-12-04 3:28 ` [dpdk-dev] [PATCH v2] " Michael Qiu @ 2014-12-04 4:16 ` Michael Qiu 2014-12-05 5:34 ` Qiu, Michael 2014-12-08 1:30 ` Qiu, Michael 2014-12-04 12:54 ` [dpdk-dev] [PATCH v2] " Ananyev, Konstantin 1 sibling, 2 replies; 32+ messages in thread From: Michael Qiu @ 2014-12-04 4:16 UTC (permalink / raw) To: dev app/test-pmd/csumonly.c: In function ‘get_psd_sum’: build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’ does break strict-aliasing rules build/include/rte_ip.h:157: note: initialized from here ... The root cause is that, compile enable strict aliasing by default, while in function rte_raw_cksum() try to convert 'const char *' to 'const uint16_t *'. This patch is one workaround fix. Signed-off-by: Michael Qiu <michael.qiu@intel.com> --- v3 --> v2: use uintptr_t instead of unsigned long to save pointer. v2 --> v1: Workaround solution instead of shut off the gcc params. lib/librte_net/rte_ip.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h index 61e4457..cda3436 100644 --- a/lib/librte_net/rte_ip.h +++ b/lib/librte_net/rte_ip.h @@ -154,7 +154,8 @@ struct ipv4_hdr { static inline uint16_t rte_raw_cksum(const char *buf, size_t len) { - const uint16_t *u16 = (const uint16_t *)buf; + uintptr_t ptr = (uintptr_t)buf; + const uint16_t *u16 = (const uint16_t *)ptr; uint32_t sum = 0; while (len >= (sizeof(*u16) * 4)) { -- 1.9.3 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v3] test-pmd: Fix pointer aliasing error 2014-12-04 4:16 ` [dpdk-dev] [PATCH v3] " Michael Qiu @ 2014-12-05 5:34 ` Qiu, Michael 2014-12-05 9:24 ` Thomas Monjalon 2014-12-08 1:30 ` Qiu, Michael 1 sibling, 1 reply; 32+ messages in thread From: Qiu, Michael @ 2014-12-05 5:34 UTC (permalink / raw) To: Michael Qiu, dev Any comments about this version? a new workaround solution :) Thanks, Michael On 12/4/2014 9:35 PM, Michael Qiu wrote: > app/test-pmd/csumonly.c: In function ‘get_psd_sum’: > build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’ > does break strict-aliasing rules > build/include/rte_ip.h:157: note: initialized from here > ... > > The root cause is that, compile enable strict aliasing by default, > while in function rte_raw_cksum() try to convert 'const char *' > to 'const uint16_t *'. > > This patch is one workaround fix. > > Signed-off-by: Michael Qiu <michael.qiu@intel.com> > --- > v3 --> v2: > use uintptr_t instead of unsigned long to > save pointer. > > v2 --> v1: > Workaround solution instead of shut off the > gcc params. > > lib/librte_net/rte_ip.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h > index 61e4457..cda3436 100644 > --- a/lib/librte_net/rte_ip.h > +++ b/lib/librte_net/rte_ip.h > @@ -154,7 +154,8 @@ struct ipv4_hdr { > static inline uint16_t > rte_raw_cksum(const char *buf, size_t len) > { > - const uint16_t *u16 = (const uint16_t *)buf; > + uintptr_t ptr = (uintptr_t)buf; > + const uint16_t *u16 = (const uint16_t *)ptr; > uint32_t sum = 0; > > while (len >= (sizeof(*u16) * 4)) { ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v3] test-pmd: Fix pointer aliasing error 2014-12-05 5:34 ` Qiu, Michael @ 2014-12-05 9:24 ` Thomas Monjalon 2014-12-08 1:28 ` Qiu, Michael 0 siblings, 1 reply; 32+ messages in thread From: Thomas Monjalon @ 2014-12-05 9:24 UTC (permalink / raw) To: Qiu, Michael; +Cc: dev 2014-12-05 05:34, Qiu, Michael: > Any comments about this version? a new workaround solution :) Yes, one comment: I think it's ugly :) These aliasing errors are not reliable so I think we can disable it (like Linux does). But in case you don't want to disable the warning, please add a comment to your workaround to explain it is caused by GCC strict-aliasing check. > > - const uint16_t *u16 = (const uint16_t *)buf; > > + uintptr_t ptr = (uintptr_t)buf; > > + const uint16_t *u16 = (const uint16_t *)ptr; Thanks -- Thomas ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v3] test-pmd: Fix pointer aliasing error 2014-12-05 9:24 ` Thomas Monjalon @ 2014-12-08 1:28 ` Qiu, Michael 0 siblings, 0 replies; 32+ messages in thread From: Qiu, Michael @ 2014-12-08 1:28 UTC (permalink / raw) To: Thomas Monjalon; +Cc: dev On 12/5/2014 5:26 PM, Thomas Monjalon wrote: > 2014-12-05 05:34, Qiu, Michael: >> Any comments about this version? a new workaround solution :) > Yes, one comment: I think it's ugly :) > These aliasing errors are not reliable so I think we can disable it (like > Linux does). > But in case you don't want to disable the warning, please add a comment to your > workaround to explain it is caused by GCC strict-aliasing check. Yes, really ugly .... But lots of reviewer voted against with disable it as my first version is disable. I could not think out a better solution for all reviewers can accept. I will add the comment with the thread of v3 patch. Thanks, Michael >>> - const uint16_t *u16 = (const uint16_t *)buf; >>> + uintptr_t ptr = (uintptr_t)buf; >>> + const uint16_t *u16 = (const uint16_t *)ptr; > Thanks ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v3] test-pmd: Fix pointer aliasing error 2014-12-04 4:16 ` [dpdk-dev] [PATCH v3] " Michael Qiu 2014-12-05 5:34 ` Qiu, Michael @ 2014-12-08 1:30 ` Qiu, Michael 2014-12-10 3:44 ` Qiu, Michael 2014-12-11 0:54 ` Thomas Monjalon 1 sibling, 2 replies; 32+ messages in thread From: Qiu, Michael @ 2014-12-08 1:30 UTC (permalink / raw) To: Michael Qiu, dev On 12/4/2014 9:35 PM, Michael Qiu wrote: > app/test-pmd/csumonly.c: In function ‘get_psd_sum’: > build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’ > does break strict-aliasing rules > build/include/rte_ip.h:157: note: initialized from here > ... > > The root cause is that, compile enable strict aliasing by default, > while in function rte_raw_cksum() try to convert 'const char *' > to 'const uint16_t *'. > > This patch is one workaround fix. > > Signed-off-by: Michael Qiu <michael.qiu@intel.com> > --- > v3 --> v2: > use uintptr_t instead of unsigned long to > save pointer. > > v2 --> v1: > Workaround solution instead of shut off the > gcc params. > > lib/librte_net/rte_ip.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h > index 61e4457..cda3436 100644 > --- a/lib/librte_net/rte_ip.h > +++ b/lib/librte_net/rte_ip.h > @@ -154,7 +154,8 @@ struct ipv4_hdr { > static inline uint16_t > rte_raw_cksum(const char *buf, size_t len) > { > - const uint16_t *u16 = (const uint16_t *)buf; > + uintptr_t ptr = (uintptr_t)buf; > + const uint16_t *u16 = (const uint16_t *)ptr; > uint32_t sum = 0; > > while (len >= (sizeof(*u16) * 4)) { This workaround is to solve the compile issue of GCC strict-aliasing(Two different type pointers should not be point to the same memory address). For GCC 4.4.7 it will definitely occurs if flags "-fstrict-aliasing" and "-Wall" used. Thanks, Michael ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v3] test-pmd: Fix pointer aliasing error 2014-12-08 1:30 ` Qiu, Michael @ 2014-12-10 3:44 ` Qiu, Michael 2014-12-11 0:54 ` Thomas Monjalon 1 sibling, 0 replies; 32+ messages in thread From: Qiu, Michael @ 2014-12-10 3:44 UTC (permalink / raw) To: Michael Qiu, dev, Thomas.monjalon@6wind.com Hi Thomas, What's going on with this patch? I really do not have other better solution. Thanks, Michael On 12/8/2014 9:30 AM, Qiu, Michael wrote: > On 12/4/2014 9:35 PM, Michael Qiu wrote: >> app/test-pmd/csumonly.c: In function ‘get_psd_sum’: >> build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’ >> does break strict-aliasing rules >> build/include/rte_ip.h:157: note: initialized from here >> ... >> >> The root cause is that, compile enable strict aliasing by default, >> while in function rte_raw_cksum() try to convert 'const char *' >> to 'const uint16_t *'. >> >> This patch is one workaround fix. >> >> Signed-off-by: Michael Qiu <michael.qiu@intel.com> >> --- >> v3 --> v2: >> use uintptr_t instead of unsigned long to >> save pointer. >> >> v2 --> v1: >> Workaround solution instead of shut off the >> gcc params. >> >> lib/librte_net/rte_ip.h | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h >> index 61e4457..cda3436 100644 >> --- a/lib/librte_net/rte_ip.h >> +++ b/lib/librte_net/rte_ip.h >> @@ -154,7 +154,8 @@ struct ipv4_hdr { >> static inline uint16_t >> rte_raw_cksum(const char *buf, size_t len) >> { >> - const uint16_t *u16 = (const uint16_t *)buf; >> + uintptr_t ptr = (uintptr_t)buf; >> + const uint16_t *u16 = (const uint16_t *)ptr; >> uint32_t sum = 0; >> >> while (len >= (sizeof(*u16) * 4)) { > This workaround is to solve the compile issue of GCC strict-aliasing(Two > different type pointers should not be point to the same memory address). > > For GCC 4.4.7 it will definitely occurs if flags "-fstrict-aliasing" > and "-Wall" used. > > Thanks, > Michael > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v3] test-pmd: Fix pointer aliasing error 2014-12-08 1:30 ` Qiu, Michael 2014-12-10 3:44 ` Qiu, Michael @ 2014-12-11 0:54 ` Thomas Monjalon 2014-12-11 17:51 ` r k 1 sibling, 1 reply; 32+ messages in thread From: Thomas Monjalon @ 2014-12-11 0:54 UTC (permalink / raw) To: Qiu, Michael, dev > > app/test-pmd/csumonly.c: In function ‘get_psd_sum’: > > build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’ > > does break strict-aliasing rules > > build/include/rte_ip.h:157: note: initialized from here > > ... > > > > The root cause is that, compile enable strict aliasing by default, > > while in function rte_raw_cksum() try to convert 'const char *' > > to 'const uint16_t *'. > > > > This patch is one workaround fix. > > > > Signed-off-by: Michael Qiu <michael.qiu@intel.com> > > --- > > v3 --> v2: > > use uintptr_t instead of unsigned long to > > save pointer. > > > > v2 --> v1: > > Workaround solution instead of shut off the > > gcc params. > > This workaround is to solve the compile issue of GCC strict-aliasing(Two > different type pointers should not be point to the same memory address). > > For GCC 4.4.7 it will definitely occurs if flags "-fstrict-aliasing" > and "-Wall" used. Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>> Applied with a comment in the code. Thanks -- Thomas ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v3] test-pmd: Fix pointer aliasing error 2014-12-11 0:54 ` Thomas Monjalon @ 2014-12-11 17:51 ` r k 2014-12-12 6:49 ` Qiu, Michael 0 siblings, 1 reply; 32+ messages in thread From: r k @ 2014-12-11 17:51 UTC (permalink / raw) To: Thomas Monjalon; +Cc: dev Thomas, Michael, Wouldn't it cause unaligned memory access (new changes as well as the previous code)? Wondering if get_unaligned/put_unaligned macros similar to the ones used in kernel be ported to user-space? Thanks, Ravi On Wed, Dec 10, 2014 at 4:54 PM, Thomas Monjalon <thomas.monjalon@6wind.com> wrote: > > > > app/test-pmd/csumonly.c: In function 'get_psd_sum': > > > build/include/rte_ip.h:161: error: dereferencing pointer 'u16' > > > does break strict-aliasing rules > > > build/include/rte_ip.h:157: note: initialized from here > > > ... > > > > > > The root cause is that, compile enable strict aliasing by default, > > > while in function rte_raw_cksum() try to convert 'const char *' > > > to 'const uint16_t *'. > > > > > > This patch is one workaround fix. > > > > > > Signed-off-by: Michael Qiu <michael.qiu@intel.com> > > > --- > > > v3 --> v2: > > > use uintptr_t instead of unsigned long to > > > save pointer. > > > > > > v2 --> v1: > > > Workaround solution instead of shut off the > > > gcc params. > > > > This workaround is to solve the compile issue of GCC strict-aliasing(Two > > different type pointers should not be point to the same memory address). > > > > For GCC 4.4.7 it will definitely occurs if flags "-fstrict-aliasing" > > and "-Wall" used. > > Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>> > > Applied with a comment in the code. > > Thanks > -- > Thomas > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v3] test-pmd: Fix pointer aliasing error 2014-12-11 17:51 ` r k @ 2014-12-12 6:49 ` Qiu, Michael 0 siblings, 0 replies; 32+ messages in thread From: Qiu, Michael @ 2014-12-12 6:49 UTC (permalink / raw) To: r k, Thomas Monjalon; +Cc: dev On 2014/12/12 1:51, r k wrote: Thomas, Michael, Wouldn't it cause unaligned memory access (new changes as well as the previous code)? Wondering if get_unaligned/put_unaligned macros similar to the ones used in kernel be ported to user-space? I think it will not, as all buf point to are struct udp_hdr/struct tcp_hdr/struct ipv6_psd_header/struct ipv4_psd_header, they are all aligned with uint16_t. Thanks Michael Thanks, Ravi On Wed, Dec 10, 2014 at 4:54 PM, Thomas Monjalon <thomas.monjalon@6wind.com<mailto:thomas.monjalon@6wind.com>> wrote: > > app/test-pmd/csumonly.c: In function ‘get_psd_sum’: > > build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’ > > does break strict-aliasing rules > > build/include/rte_ip.h:157: note: initialized from here > > ... > > > > The root cause is that, compile enable strict aliasing by default, > > while in function rte_raw_cksum() try to convert 'const char *' > > to 'const uint16_t *'. > > > > This patch is one workaround fix. > > > > Signed-off-by: Michael Qiu <michael.qiu@intel.com<mailto:michael.qiu@intel.com>> > > --- > > v3 --> v2: > > use uintptr_t instead of unsigned long to > > save pointer. > > > > v2 --> v1: > > Workaround solution instead of shut off the > > gcc params. > > This workaround is to solve the compile issue of GCC strict-aliasing(Two > different type pointers should not be point to the same memory address). > > For GCC 4.4.7 it will definitely occurs if flags "-fstrict-aliasing" > and "-Wall" used. Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com<mailto:thomas.monjalon@6wind.com>>> Applied with a comment in the code. Thanks -- Thomas ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v2] test-pmd: Fix pointer aliasing error 2014-12-04 3:28 ` [dpdk-dev] [PATCH v2] " Michael Qiu 2014-12-04 4:16 ` [dpdk-dev] [PATCH v3] " Michael Qiu @ 2014-12-04 12:54 ` Ananyev, Konstantin 1 sibling, 0 replies; 32+ messages in thread From: Ananyev, Konstantin @ 2014-12-04 12:54 UTC (permalink / raw) To: Michael Qiu, dev > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Michael Qiu > Sent: Thursday, December 04, 2014 3:29 AM > To: dev@dpdk.org > Subject: [dpdk-dev] [PATCH v2] test-pmd: Fix pointer aliasing error > > > app/test-pmd/csumonly.c: In function ‘get_psd_sum’: > build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’ > does break strict-aliasing rules > build/include/rte_ip.h:157: note: initialized from here > ... > > The root cause is that, compile enable strict aliasing by default, > while in function rte_raw_cksum() try to convert 'const char *' > to 'const uint16_t *'. > > This patch is one workaround fix. > > Signed-off-by: Michael Qiu <michael.qiu@intel.com> > --- > lib/librte_net/rte_ip.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h > index 61e4457..f1c7087 100644 > --- a/lib/librte_net/rte_ip.h > +++ b/lib/librte_net/rte_ip.h > @@ -154,7 +154,8 @@ struct ipv4_hdr { > static inline uint16_t > rte_raw_cksum(const char *buf, size_t len) > { > - const uint16_t *u16 = (const uint16_t *)buf; > + unsigned long ptr = (unsigned long)buf; I think better use uintptr_t here. It is specially designed to be long enough to hold pointer value. > + const uint16_t *u16 = (const uint16_t *)ptr; > uint32_t sum = 0; > > while (len >= (sizeof(*u16) * 4)) { > -- > 1.9.3 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error 2014-12-03 11:42 ` [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error Bruce Richardson 2014-12-03 13:59 ` Qiu, Michael @ 2014-12-03 15:24 ` Olivier MATZ 2014-12-03 16:03 ` Dayu Qiu 2014-12-03 15:57 ` Dayu Qiu 2 siblings, 1 reply; 32+ messages in thread From: Olivier MATZ @ 2014-12-03 15:24 UTC (permalink / raw) To: Bruce Richardson, Michael Qiu; +Cc: dev Hi Bruce, On 12/03/2014 12:42 PM, Bruce Richardson wrote: > On Wed, Dec 03, 2014 at 07:28:19PM +0800, Michael Qiu wrote: >> app/test-pmd/csumonly.c: In function ‘get_psd_sum’: >> build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’ >> does break strict-aliasing rules >> build/include/rte_ip.h:157: note: initialized from here >> ... >> >> The root cause is that, compile enable strict aliasing by default, >> while in function rte_raw_cksum() try to convert 'const char *' >> to 'const uint16_t *'. >> > > What compiler version is this with? Is there any other way to fix this > other than disabling the compiler warnings. Turning off strict aliasing may > affect performance as it reduces the number of optimizations that the compiler > can perform. I can reproduce the issue with a gcc-4.4.6 toolchain. But I think it's a toolchain bug as the warning does not occur with other versions I've tested. If it's the case, we could either: - do nothing: in this case the user need to upgrade its toolchain, or pass the -Wno-strict-aliasing manually in EXTRA_CFLAGS - add the -Wno-strict-aliasing only for gcc 4.4 in the Makefile What do you think? Regards, Olivier ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error 2014-12-03 15:24 ` [dpdk-dev] [PATCH] " Olivier MATZ @ 2014-12-03 16:03 ` Dayu Qiu 0 siblings, 0 replies; 32+ messages in thread From: Dayu Qiu @ 2014-12-03 16:03 UTC (permalink / raw) To: Olivier MATZ; +Cc: dev Hi Olivier, You can check gcc manual. -fstrict-aliasing Allow the compiler to assume the strictest aliasing rules applicable to the language being compiled. For C (and C++), this activates optimizations based on the type of expressions. In particular, an object of one type is assumed never to reside at the same address as an object of a different type, unless the types are almost the same. For example, an "unsigned int" can alias an "int", but not a "void*" or a "double". A character type may alias any other type. So it should not be a bug, but I have no idea about why other version does not reproduce. Thanks, Michael On Wed, Dec 3, 2014 at 11:24 PM, Olivier MATZ <olivier.matz@6wind.com> wrote: > Hi Bruce, > > On 12/03/2014 12:42 PM, Bruce Richardson wrote: > >> On Wed, Dec 03, 2014 at 07:28:19PM +0800, Michael Qiu wrote: >> >>> app/test-pmd/csumonly.c: In function ‘get_psd_sum’: >>> build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’ >>> does break strict-aliasing rules >>> build/include/rte_ip.h:157: note: initialized from here >>> ... >>> >>> The root cause is that, compile enable strict aliasing by default, >>> while in function rte_raw_cksum() try to convert 'const char *' >>> to 'const uint16_t *'. >>> >>> >> What compiler version is this with? Is there any other way to fix this >> other than disabling the compiler warnings. Turning off strict aliasing >> may >> affect performance as it reduces the number of optimizations that the >> compiler >> can perform. >> > > I can reproduce the issue with a gcc-4.4.6 toolchain. But I think it's > a toolchain bug as the warning does not occur with other versions I've > tested. > > If it's the case, we could either: > > - do nothing: in this case the user need to upgrade its toolchain, or > pass the -Wno-strict-aliasing manually in EXTRA_CFLAGS > > - add the -Wno-strict-aliasing only for gcc 4.4 in the Makefile > > What do you think? > > Regards, > Olivier > > -- Thanks & Best Regards Mike ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error 2014-12-03 11:42 ` [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error Bruce Richardson 2014-12-03 13:59 ` Qiu, Michael 2014-12-03 15:24 ` [dpdk-dev] [PATCH] " Olivier MATZ @ 2014-12-03 15:57 ` Dayu Qiu 2 siblings, 0 replies; 32+ messages in thread From: Dayu Qiu @ 2014-12-03 15:57 UTC (permalink / raw) To: Bruce Richardson; +Cc: dev Just re-post this mail as Thomas said it missed in mail list. On Wed, Dec 3, 2014 at 7:42 PM, Bruce Richardson <bruce.richardson@intel.com > wrote: > On Wed, Dec 03, 2014 at 07:28:19PM +0800, Michael Qiu wrote: > > app/test-pmd/csumonly.c: In function ‘get_psd_sum’: > > build/include/rte_ip.h:161: error: dereferencing pointer ‘u16’ > > does break strict-aliasing rules > > build/include/rte_ip.h:157: note: initialized from here > > ... > > > > The root cause is that, compile enable strict aliasing by default, > > while in function rte_raw_cksum() try to convert 'const char *' > > to 'const uint16_t *'. > > > > What compiler version is this with? Is there any other way to fix this > other than disabling the compiler warnings. Turning off strict aliasing may > affect performance as it reduces the number of optimizations that the > compiler > can perform. > > The compile version is: $ gcc -v Using built-in specs. Target: x86_64-redhat-linux ... gcc version 4.4.7 20120313 (Red Hat 4.4.7-4) (GCC) The OS is centos6.5 x86_64 Actually, another possible solution is, as gcc manual shows, use union. In function rte_raw_cksum() of lib/librte_net/rte_ip.h: static inline uint16_t rte_raw_cksum(const char *buf, size_t len){ union { const char *ubuf; const uint16_t *uu16; } convert; convert.ubuf = buf; const uint16_t *u16 = convert.uu16; ... } This may be work, but not test yet. Thanks, Michael > /Bruce > > > Need to add CFLAG '-Wno-strict-aliasing' to avoid this issue. > > > > Signed-off-by: Michael Qiu <michael.qiu@intel.com> > > --- > > app/test-pmd/Makefile | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/app/test-pmd/Makefile b/app/test-pmd/Makefile > > index 97dc2e6..995c874 100644 > > --- a/app/test-pmd/Makefile > > +++ b/app/test-pmd/Makefile > > @@ -38,7 +38,7 @@ ifeq ($(CONFIG_RTE_TEST_PMD),y) > > # > > APP = testpmd > > > > -CFLAGS += -O3 > > +CFLAGS += -O3 -Wno-strict-aliasing > > CFLAGS += $(WERROR_FLAGS) > > > > ifeq ($(CONFIG_RTE_LIBRTE_PMD_PCAP),y) > > -- > > 1.9.3 > > > -- Thanks & Best Regards Mike ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] test-pmd: Fix "__BYTE_ORDER__" not defined error [not found] <1417606044-3432-1-git-send-email-michael.qiu@intel.com> [not found] ` <1417606099-3489-1-git-send-email-michael.qiu@intel.com> @ 2014-12-03 16:26 ` Qiu, Michael 2014-12-03 19:59 ` Thomas Monjalon 1 sibling, 1 reply; 32+ messages in thread From: Qiu, Michael @ 2014-12-03 16:26 UTC (permalink / raw) To: Michael Qiu, dev Hi all, What about this patch? It may be some network or mail client issue of me, so not sure this patch posted to mail list successful. If failed, I will re-post it later. Thanks, Michael On 2014/12/3 19:28, Michael Qiu wrote: > app/test-pmd/csumonly.c:84:5: error: "__BYTE_ORDER__" is not defined > app/test-pmd/csumonly.c:84:23: error: "__ORDER_LITTLE_ENDIAN__" is not defined > > This because old gcc version, like 4.4.7, does not has these buildin macros. > > $ gcc -v > Using built-in specs. > Target: x86_64-redhat-linux > ... > gcc version 4.4.7 20120313 (Red Hat 4.4.7-4) (GCC) > > $ echo | gcc -dM -E -| grep "LITTLE" > (none) > > In this situation, we should back to use the macros defined in > <endian.h> > > Signed-off-by: Michael Qiu <michael.qiu@intel.com> > --- > app/test-pmd/csumonly.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c > index 6f43761..3fa81a2 100644 > --- a/app/test-pmd/csumonly.c > +++ b/app/test-pmd/csumonly.c > @@ -81,7 +81,13 @@ > > /* we cannot use htons() from arpa/inet.h due to name conflicts, and we > * cannot use rte_cpu_to_be_16() on a constant in a switch/case */ > -#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ > +#ifdef __BYTE_ORDER__ > +#define LITTLE_ENDIAN_CHECK (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__) > +#else > +#define LITTLE_ENDIAN_CHECK (__BYTE_ORDER == __LITTLE_ENDIAN) > +#endif > + > +#if LITTLE_ENDIAN_CHECK > #define _htons(x) ((uint16_t)((((x) & 0x00ffU) << 8) | (((x) & 0xff00U) >> 8))) > #else > #define _htons(x) (x) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] test-pmd: Fix "__BYTE_ORDER__" not defined error 2014-12-03 16:26 ` [dpdk-dev] [PATCH] test-pmd: Fix "__BYTE_ORDER__" not defined error Qiu, Michael @ 2014-12-03 19:59 ` Thomas Monjalon 2014-12-03 20:47 ` [dpdk-dev] [PATCH 0/2] fix endianness in EAL Thomas Monjalon 0 siblings, 1 reply; 32+ messages in thread From: Thomas Monjalon @ 2014-12-03 19:59 UTC (permalink / raw) To: Qiu, Michael; +Cc: dev 2014-12-03 16:26, Qiu, Michael: > > --- a/app/test-pmd/csumonly.c > > +++ b/app/test-pmd/csumonly.c > > @@ -81,7 +81,13 @@ > > > > /* we cannot use htons() from arpa/inet.h due to name conflicts, and we > > * cannot use rte_cpu_to_be_16() on a constant in a switch/case */ > > -#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ > > +#ifdef __BYTE_ORDER__ > > +#define LITTLE_ENDIAN_CHECK (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__) > > +#else > > +#define LITTLE_ENDIAN_CHECK (__BYTE_ORDER == __LITTLE_ENDIAN) > > +#endif > > + > > +#if LITTLE_ENDIAN_CHECK > > #define _htons(x) ((uint16_t)((((x) & 0x00ffU) << 8) | (((x) & 0xff00U) >> 8))) > > #else > > #define _htons(x) (x) I don't agree we should fix it in testpmd. It's a more general problem which must be solved in EAL. I'm preparing a patchset to fix it. -- Thomas ^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH 0/2] fix endianness in EAL 2014-12-03 19:59 ` Thomas Monjalon @ 2014-12-03 20:47 ` Thomas Monjalon 2014-12-03 20:47 ` [dpdk-dev] [PATCH 1/2] eal: detect endianness Thomas Monjalon ` (2 more replies) 0 siblings, 3 replies; 32+ messages in thread From: Thomas Monjalon @ 2014-12-03 20:47 UTC (permalink / raw) To: dev It can be hard to implement a reliable detection of endianness. Previous trials in testpmd failed. The IBM Power patchset introduced a config option. This patchset try to improve the situation by having a detection in EAL headers. Please test it (especially on IBM Power) with different toolchains or distributions. Thomas Monjalon (2): eal: detect endianness app/testpmd: fix endianness detection -- 2.1.3 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH 1/2] eal: detect endianness 2014-12-03 20:47 ` [dpdk-dev] [PATCH 0/2] fix endianness in EAL Thomas Monjalon @ 2014-12-03 20:47 ` Thomas Monjalon 2014-12-04 2:28 ` Qiu, Michael 2014-12-03 20:47 ` [dpdk-dev] [PATCH 2/2] app/testpmd: fix endianness detection Thomas Monjalon 2014-12-04 9:28 ` [dpdk-dev] [PATCH 0/2] fix endianness in EAL Chao Zhu 2 siblings, 1 reply; 32+ messages in thread From: Thomas Monjalon @ 2014-12-03 20:47 UTC (permalink / raw) To: dev There is no standard to check endianness. So we need to try different checks. Previous trials were done in testpmd (see commits 51f694dd40f56 and 64741f237cf29) without full success. This one is not guaranteed to work everywhere so it could evolve when exceptions are found. If endianness is not detected, there is a fallback on x86 to little endian. It could be forced before doing detection but it would add some arch-dependent code in the generic header. The option CONFIG_RTE_ARCH_BIG_ENDIAN introduced for IBM Power only (commit a982ec81d84d53) can be removed. A compile-time check is better. Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com> --- config/defconfig_ppc_64-power8-linuxapp-gcc | 1 - .../common/include/arch/ppc_64/rte_byteorder.h | 4 ++-- .../common/include/arch/x86/rte_byteorder.h | 4 ++++ .../common/include/generic/rte_byteorder.h | 28 ++++++++++++++++++++++ 4 files changed, 34 insertions(+), 3 deletions(-) diff --git a/config/defconfig_ppc_64-power8-linuxapp-gcc b/config/defconfig_ppc_64-power8-linuxapp-gcc index 48018c3..d97a885 100644 --- a/config/defconfig_ppc_64-power8-linuxapp-gcc +++ b/config/defconfig_ppc_64-power8-linuxapp-gcc @@ -34,7 +34,6 @@ CONFIG_RTE_MACHINE="power8" CONFIG_RTE_ARCH="ppc_64" CONFIG_RTE_ARCH_PPC_64=y -CONFIG_RTE_ARCH_BIG_ENDIAN=y CONFIG_RTE_ARCH_64=y CONFIG_RTE_TOOLCHAIN="gcc" diff --git a/lib/librte_eal/common/include/arch/ppc_64/rte_byteorder.h b/lib/librte_eal/common/include/arch/ppc_64/rte_byteorder.h index 1a89051..80436f2 100644 --- a/lib/librte_eal/common/include/arch/ppc_64/rte_byteorder.h +++ b/lib/librte_eal/common/include/arch/ppc_64/rte_byteorder.h @@ -105,7 +105,7 @@ static inline uint64_t rte_arch_bswap64(uint64_t _x) /* Power 8 have both little endian and big endian mode * Power 7 only support big endian */ -#ifndef RTE_ARCH_BIG_ENDIAN +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN #define rte_cpu_to_le_16(x) (x) #define rte_cpu_to_le_32(x) (x) @@ -123,7 +123,7 @@ static inline uint64_t rte_arch_bswap64(uint64_t _x) #define rte_be_to_cpu_32(x) rte_bswap32(x) #define rte_be_to_cpu_64(x) rte_bswap64(x) -#else +#else /* RTE_BIG_ENDIAN */ #define rte_cpu_to_le_16(x) rte_bswap16(x) #define rte_cpu_to_le_32(x) rte_bswap32(x) diff --git a/lib/librte_eal/common/include/arch/x86/rte_byteorder.h b/lib/librte_eal/common/include/arch/x86/rte_byteorder.h index 1aa6985..ffdb6ef 100644 --- a/lib/librte_eal/common/include/arch/x86/rte_byteorder.h +++ b/lib/librte_eal/common/include/arch/x86/rte_byteorder.h @@ -40,6 +40,10 @@ extern "C" { #include "generic/rte_byteorder.h" +#ifndef RTE_BYTE_ORDER +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN +#endif + /* * An architecture-optimized byte swap for a 16-bit value. * diff --git a/lib/librte_eal/common/include/generic/rte_byteorder.h b/lib/librte_eal/common/include/generic/rte_byteorder.h index 9358136..ea23fdf 100644 --- a/lib/librte_eal/common/include/generic/rte_byteorder.h +++ b/lib/librte_eal/common/include/generic/rte_byteorder.h @@ -44,6 +44,34 @@ */ #include <stdint.h> +#ifdef RTE_EXEC_ENV_BSDAPP +#include <sys/endian.h> +#else +#include <endian.h> +#endif + +/* + * Compile-time endianness detection + */ +#define RTE_BIG_ENDIAN 1 +#define RTE_LITTLE_ENDIAN 2 +#if defined __BYTE_ORDER +#if __BYTE_ORDER == __BIG_ENDIAN +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN +#elif __BYTE_ORDER == __LITTLE_ENDIAN +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN +#endif /* __BYTE_ORDER */ +#elif defined __BYTE_ORDER__ +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN +#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN +#endif /* __BYTE_ORDER__ */ +#elif defined __BIG_ENDIAN__ +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN +#elif defined __LITTLE_ENDIAN__ +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN +#endif /* * An internal function to swap bytes in a 16-bit value. -- 2.1.3 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH 1/2] eal: detect endianness 2014-12-03 20:47 ` [dpdk-dev] [PATCH 1/2] eal: detect endianness Thomas Monjalon @ 2014-12-04 2:28 ` Qiu, Michael 2014-12-04 9:00 ` Thomas Monjalon 0 siblings, 1 reply; 32+ messages in thread From: Qiu, Michael @ 2014-12-04 2:28 UTC (permalink / raw) To: Thomas Monjalon, dev; +Cc: Chao Zhu On 12/4/2014 5:26 AM, Thomas Monjalon wrote: > There is no standard to check endianness. > So we need to try different checks. > Previous trials were done in testpmd (see commits > 51f694dd40f56 and 64741f237cf29) without full success. > This one is not guaranteed to work everywhere so it could > evolve when exceptions are found. > > If endianness is not detected, there is a fallback on x86 > to little endian. It could be forced before doing detection > but it would add some arch-dependent code in the generic header. > > The option CONFIG_RTE_ARCH_BIG_ENDIAN introduced for IBM Power only > (commit a982ec81d84d53) can be removed. A compile-time check is better. > > Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com> > --- > config/defconfig_ppc_64-power8-linuxapp-gcc | 1 - > .../common/include/arch/ppc_64/rte_byteorder.h | 4 ++-- > .../common/include/arch/x86/rte_byteorder.h | 4 ++++ > .../common/include/generic/rte_byteorder.h | 28 ++++++++++++++++++++++ > 4 files changed, 34 insertions(+), 3 deletions(-) > > diff --git a/config/defconfig_ppc_64-power8-linuxapp-gcc b/config/defconfig_ppc_64-power8-linuxapp-gcc > index 48018c3..d97a885 100644 > --- a/config/defconfig_ppc_64-power8-linuxapp-gcc > +++ b/config/defconfig_ppc_64-power8-linuxapp-gcc > @@ -34,7 +34,6 @@ CONFIG_RTE_MACHINE="power8" > > CONFIG_RTE_ARCH="ppc_64" > CONFIG_RTE_ARCH_PPC_64=y > -CONFIG_RTE_ARCH_BIG_ENDIAN=y > CONFIG_RTE_ARCH_64=y > > CONFIG_RTE_TOOLCHAIN="gcc" > diff --git a/lib/librte_eal/common/include/arch/ppc_64/rte_byteorder.h b/lib/librte_eal/common/include/arch/ppc_64/rte_byteorder.h > index 1a89051..80436f2 100644 > --- a/lib/librte_eal/common/include/arch/ppc_64/rte_byteorder.h > +++ b/lib/librte_eal/common/include/arch/ppc_64/rte_byteorder.h > @@ -105,7 +105,7 @@ static inline uint64_t rte_arch_bswap64(uint64_t _x) > /* Power 8 have both little endian and big endian mode > * Power 7 only support big endian > */ > -#ifndef RTE_ARCH_BIG_ENDIAN > +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN > > #define rte_cpu_to_le_16(x) (x) > #define rte_cpu_to_le_32(x) (x) > @@ -123,7 +123,7 @@ static inline uint64_t rte_arch_bswap64(uint64_t _x) > #define rte_be_to_cpu_32(x) rte_bswap32(x) > #define rte_be_to_cpu_64(x) rte_bswap64(x) > > -#else > +#else /* RTE_BIG_ENDIAN */ > > #define rte_cpu_to_le_16(x) rte_bswap16(x) > #define rte_cpu_to_le_32(x) rte_bswap32(x) > diff --git a/lib/librte_eal/common/include/arch/x86/rte_byteorder.h b/lib/librte_eal/common/include/arch/x86/rte_byteorder.h > index 1aa6985..ffdb6ef 100644 > --- a/lib/librte_eal/common/include/arch/x86/rte_byteorder.h > +++ b/lib/librte_eal/common/include/arch/x86/rte_byteorder.h > @@ -40,6 +40,10 @@ extern "C" { > > #include "generic/rte_byteorder.h" > > +#ifndef RTE_BYTE_ORDER > +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN > +#endif > + > /* > * An architecture-optimized byte swap for a 16-bit value. > * > diff --git a/lib/librte_eal/common/include/generic/rte_byteorder.h b/lib/librte_eal/common/include/generic/rte_byteorder.h > index 9358136..ea23fdf 100644 > --- a/lib/librte_eal/common/include/generic/rte_byteorder.h > +++ b/lib/librte_eal/common/include/generic/rte_byteorder.h > @@ -44,6 +44,34 @@ > */ > > #include <stdint.h> > +#ifdef RTE_EXEC_ENV_BSDAPP > +#include <sys/endian.h> > +#else > +#include <endian.h> > +#endif > + > +/* > + * Compile-time endianness detection > + */ > +#define RTE_BIG_ENDIAN 1 > +#define RTE_LITTLE_ENDIAN 2 > +#if defined __BYTE_ORDER > +#if __BYTE_ORDER == __BIG_ENDIAN > +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN > +#elif __BYTE_ORDER == __LITTLE_ENDIAN > +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN > +#endif /* __BYTE_ORDER */ > +#elif defined __BYTE_ORDER__ > +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ > +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN > +#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ > +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN > +#endif /* __BYTE_ORDER__ */ > +#elif defined __BIG_ENDIAN__ > +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN > +#elif defined __LITTLE_ENDIAN__ > +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN > +#endif What do you think about : +/* + * Compile-time endianness detection + */ +#define RTE_BIG_ENDIAN 1 +#define RTE_LITTLE_ENDIAN 2 +if defined __BYTE_ORDER__ /* Prefer gcc build-in macros */ +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN +#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN +#endif /* __BYTE_ORDER__ */ +#else +#if defined RTE_EXEC_ENV_BSDAPP +#include <sys/endian.h> +#else +#include <endian.h> +#endif +#if defined __BYTE_ORDER +#if __BYTE_ORDER == __BIG_ENDIAN +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN +#elif __BYTE_ORDER == __LITTLE_ENDIAN +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN +#endif /* __BYTE_ORDER */ +#elif defined __BIG_ENDIAN__ +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN +#elif defined __LITTLE_ENDIAN__ +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN +#endif +#endif > > /* > * An internal function to swap bytes in a 16-bit value. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH 1/2] eal: detect endianness 2014-12-04 2:28 ` Qiu, Michael @ 2014-12-04 9:00 ` Thomas Monjalon 2014-12-04 10:28 ` Qiu, Michael 0 siblings, 1 reply; 32+ messages in thread From: Thomas Monjalon @ 2014-12-04 9:00 UTC (permalink / raw) To: Qiu, Michael; +Cc: dev, Chao Zhu 2014-12-04 02:28, Qiu, Michael: > On 12/4/2014 5:26 AM, Thomas Monjalon wrote: > > There is no standard to check endianness. > > So we need to try different checks. > > Previous trials were done in testpmd (see commits > > 51f694dd40f56 and 64741f237cf29) without full success. > > This one is not guaranteed to work everywhere so it could > > evolve when exceptions are found. [...] > > #include <stdint.h> > > +#ifdef RTE_EXEC_ENV_BSDAPP > > +#include <sys/endian.h> > > +#else > > +#include <endian.h> > > +#endif > > + > > +/* > > + * Compile-time endianness detection > > + */ > > +#define RTE_BIG_ENDIAN 1 > > +#define RTE_LITTLE_ENDIAN 2 > > +#if defined __BYTE_ORDER > > +#if __BYTE_ORDER == __BIG_ENDIAN > > +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN > > +#elif __BYTE_ORDER == __LITTLE_ENDIAN > > +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN > > +#endif /* __BYTE_ORDER */ > > +#elif defined __BYTE_ORDER__ > > +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ > > +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN > > +#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ > > +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN > > +#endif /* __BYTE_ORDER__ */ > > +#elif defined __BIG_ENDIAN__ > > +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN > > +#elif defined __LITTLE_ENDIAN__ > > +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN > > +#endif > > What do you think about : > > +/* > + * Compile-time endianness detection > + */ > +#define RTE_BIG_ENDIAN 1 > +#define RTE_LITTLE_ENDIAN 2 > +if defined __BYTE_ORDER__ /* Prefer gcc build-in macros */ > +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ > +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN > +#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ > +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN > +#endif /* __BYTE_ORDER__ */ > +#else > +#if defined RTE_EXEC_ENV_BSDAPP > +#include <sys/endian.h> > +#else > +#include <endian.h> > +#endif > +#if defined __BYTE_ORDER > +#if __BYTE_ORDER == __BIG_ENDIAN > +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN > +#elif __BYTE_ORDER == __LITTLE_ENDIAN > +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN > +#endif /* __BYTE_ORDER */ > +#elif defined __BIG_ENDIAN__ > +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN > +#elif defined __LITTLE_ENDIAN__ > +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN > +#endif > +#endif Please, could you give more explanations about your proposal? Why not always try to include endian.h? Why giving high priority to __BYTE_ORDER__? -- Thomas ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH 1/2] eal: detect endianness 2014-12-04 9:00 ` Thomas Monjalon @ 2014-12-04 10:28 ` Qiu, Michael 2014-12-04 12:19 ` Thomas Monjalon 0 siblings, 1 reply; 32+ messages in thread From: Qiu, Michael @ 2014-12-04 10:28 UTC (permalink / raw) To: Thomas Monjalon; +Cc: dev, Chao Zhu On 12/4/2014 5:01 PM, Thomas Monjalon wrote: > 2014-12-04 02:28, Qiu, Michael: >> On 12/4/2014 5:26 AM, Thomas Monjalon wrote: >>> There is no standard to check endianness. >>> So we need to try different checks. >>> Previous trials were done in testpmd (see commits >>> 51f694dd40f56 and 64741f237cf29) without full success. >>> This one is not guaranteed to work everywhere so it could >>> evolve when exceptions are found. > [...] >>> #include <stdint.h> >>> +#ifdef RTE_EXEC_ENV_BSDAPP >>> +#include <sys/endian.h> >>> +#else >>> +#include <endian.h> >>> +#endif >>> + >>> +/* >>> + * Compile-time endianness detection >>> + */ >>> +#define RTE_BIG_ENDIAN 1 >>> +#define RTE_LITTLE_ENDIAN 2 >>> +#if defined __BYTE_ORDER >>> +#if __BYTE_ORDER == __BIG_ENDIAN >>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN >>> +#elif __BYTE_ORDER == __LITTLE_ENDIAN >>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN >>> +#endif /* __BYTE_ORDER */ >>> +#elif defined __BYTE_ORDER__ >>> +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ >>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN >>> +#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ >>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN >>> +#endif /* __BYTE_ORDER__ */ >>> +#elif defined __BIG_ENDIAN__ >>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN >>> +#elif defined __LITTLE_ENDIAN__ >>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN >>> +#endif >> What do you think about : >> >> +/* >> + * Compile-time endianness detection >> + */ >> +#define RTE_BIG_ENDIAN 1 >> +#define RTE_LITTLE_ENDIAN 2 >> +if defined __BYTE_ORDER__ /* Prefer gcc build-in macros */ >> +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ >> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN >> +#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ >> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN >> +#endif /* __BYTE_ORDER__ */ >> +#else >> +#if defined RTE_EXEC_ENV_BSDAPP >> +#include <sys/endian.h> >> +#else >> +#include <endian.h> >> +#endif >> +#if defined __BYTE_ORDER >> +#if __BYTE_ORDER == __BIG_ENDIAN >> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN >> +#elif __BYTE_ORDER == __LITTLE_ENDIAN >> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN >> +#endif /* __BYTE_ORDER */ >> +#elif defined __BIG_ENDIAN__ >> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN >> +#elif defined __LITTLE_ENDIAN__ >> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN >> +#endif >> +#endif > Please, could you give more explanations about your proposal? > Why not always try to include endian.h? I assume that if gcc can handler why we need include that file? Also it seems that only old version could have this issue, newer versions has build in this marcos. So that's why I prefer "__BYTE_ORDER__" for high priority. Thanks, Michael > Why giving high priority to __BYTE_ORDER__? > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH 1/2] eal: detect endianness 2014-12-04 10:28 ` Qiu, Michael @ 2014-12-04 12:19 ` Thomas Monjalon 2014-12-04 12:50 ` Qiu, Michael 0 siblings, 1 reply; 32+ messages in thread From: Thomas Monjalon @ 2014-12-04 12:19 UTC (permalink / raw) To: Qiu, Michael; +Cc: dev, Chao Zhu 2014-12-04 10:28, Qiu, Michael: > On 12/4/2014 5:01 PM, Thomas Monjalon wrote: > > 2014-12-04 02:28, Qiu, Michael: > >> On 12/4/2014 5:26 AM, Thomas Monjalon wrote: > >>> There is no standard to check endianness. > >>> So we need to try different checks. > >>> Previous trials were done in testpmd (see commits > >>> 51f694dd40f56 and 64741f237cf29) without full success. > >>> This one is not guaranteed to work everywhere so it could > >>> evolve when exceptions are found. > > [...] > >>> #include <stdint.h> > >>> +#ifdef RTE_EXEC_ENV_BSDAPP > >>> +#include <sys/endian.h> > >>> +#else > >>> +#include <endian.h> > >>> +#endif > >>> + > >>> +/* > >>> + * Compile-time endianness detection > >>> + */ > >>> +#define RTE_BIG_ENDIAN 1 > >>> +#define RTE_LITTLE_ENDIAN 2 > >>> +#if defined __BYTE_ORDER > >>> +#if __BYTE_ORDER == __BIG_ENDIAN > >>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN > >>> +#elif __BYTE_ORDER == __LITTLE_ENDIAN > >>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN > >>> +#endif /* __BYTE_ORDER */ > >>> +#elif defined __BYTE_ORDER__ > >>> +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ > >>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN > >>> +#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ > >>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN > >>> +#endif /* __BYTE_ORDER__ */ > >>> +#elif defined __BIG_ENDIAN__ > >>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN > >>> +#elif defined __LITTLE_ENDIAN__ > >>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN > >>> +#endif > >> What do you think about : > >> > >> +/* > >> + * Compile-time endianness detection > >> + */ > >> +#define RTE_BIG_ENDIAN 1 > >> +#define RTE_LITTLE_ENDIAN 2 > >> +if defined __BYTE_ORDER__ /* Prefer gcc build-in macros */ > >> +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ > >> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN > >> +#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ > >> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN > >> +#endif /* __BYTE_ORDER__ */ > >> +#else > >> +#if defined RTE_EXEC_ENV_BSDAPP > >> +#include <sys/endian.h> > >> +#else > >> +#include <endian.h> > >> +#endif > >> +#if defined __BYTE_ORDER > >> +#if __BYTE_ORDER == __BIG_ENDIAN > >> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN > >> +#elif __BYTE_ORDER == __LITTLE_ENDIAN > >> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN > >> +#endif /* __BYTE_ORDER */ > >> +#elif defined __BIG_ENDIAN__ > >> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN > >> +#elif defined __LITTLE_ENDIAN__ > >> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN > >> +#endif > >> +#endif > > > > Please, could you give more explanations about your proposal? > > Why not always try to include endian.h? > > I assume that if gcc can handler why we need include that file? Separating include on top is easier to read, and I'm not sure it won't be needed for __BYTE_ORDER__ with some toolchains. > Also it seems that only old version could have this issue, newer > versions has build in this marcos. > > So that's why I prefer "__BYTE_ORDER__" for high priority. I have no problem with reversing this priority. > > Why giving high priority to __BYTE_ORDER__? Any other comment? May I apply with above change? -- Thomas ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH 1/2] eal: detect endianness 2014-12-04 12:19 ` Thomas Monjalon @ 2014-12-04 12:50 ` Qiu, Michael 0 siblings, 0 replies; 32+ messages in thread From: Qiu, Michael @ 2014-12-04 12:50 UTC (permalink / raw) To: Thomas Monjalon; +Cc: dev, Chao Zhu On 12/4/2014 8:20 PM, Thomas Monjalon wrote: > 2014-12-04 10:28, Qiu, Michael: >> On 12/4/2014 5:01 PM, Thomas Monjalon wrote: >>> 2014-12-04 02:28, Qiu, Michael: >>>> On 12/4/2014 5:26 AM, Thomas Monjalon wrote: >>>>> There is no standard to check endianness. >>>>> So we need to try different checks. >>>>> Previous trials were done in testpmd (see commits >>>>> 51f694dd40f56 and 64741f237cf29) without full success. >>>>> This one is not guaranteed to work everywhere so it could >>>>> evolve when exceptions are found. >>> [...] >>>>> #include <stdint.h> >>>>> +#ifdef RTE_EXEC_ENV_BSDAPP >>>>> +#include <sys/endian.h> >>>>> +#else >>>>> +#include <endian.h> >>>>> +#endif >>>>> + >>>>> +/* >>>>> + * Compile-time endianness detection >>>>> + */ >>>>> +#define RTE_BIG_ENDIAN 1 >>>>> +#define RTE_LITTLE_ENDIAN 2 >>>>> +#if defined __BYTE_ORDER >>>>> +#if __BYTE_ORDER == __BIG_ENDIAN >>>>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN >>>>> +#elif __BYTE_ORDER == __LITTLE_ENDIAN >>>>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN >>>>> +#endif /* __BYTE_ORDER */ >>>>> +#elif defined __BYTE_ORDER__ >>>>> +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ >>>>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN >>>>> +#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ >>>>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN >>>>> +#endif /* __BYTE_ORDER__ */ >>>>> +#elif defined __BIG_ENDIAN__ >>>>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN >>>>> +#elif defined __LITTLE_ENDIAN__ >>>>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN >>>>> +#endif >>>> What do you think about : >>>> >>>> +/* >>>> + * Compile-time endianness detection >>>> + */ >>>> +#define RTE_BIG_ENDIAN 1 >>>> +#define RTE_LITTLE_ENDIAN 2 >>>> +if defined __BYTE_ORDER__ /* Prefer gcc build-in macros */ >>>> +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ >>>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN >>>> +#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ >>>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN >>>> +#endif /* __BYTE_ORDER__ */ >>>> +#else >>>> +#if defined RTE_EXEC_ENV_BSDAPP >>>> +#include <sys/endian.h> >>>> +#else >>>> +#include <endian.h> >>>> +#endif >>>> +#if defined __BYTE_ORDER >>>> +#if __BYTE_ORDER == __BIG_ENDIAN >>>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN >>>> +#elif __BYTE_ORDER == __LITTLE_ENDIAN >>>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN >>>> +#endif /* __BYTE_ORDER */ >>>> +#elif defined __BIG_ENDIAN__ >>>> +#define RTE_BYTE_ORDER RTE_BIG_ENDIAN >>>> +#elif defined __LITTLE_ENDIAN__ >>>> +#define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN >>>> +#endif >>>> +#endif >>> Please, could you give more explanations about your proposal? >>> Why not always try to include endian.h? >> I assume that if gcc can handler why we need include that file? > Separating include on top is easier to read, and I'm not sure it won't > be needed for __BYTE_ORDER__ with some toolchains. > >> Also it seems that only old version could have this issue, newer >> versions has build in this marcos. >> >> So that's why I prefer "__BYTE_ORDER__" for high priority. > I have no problem with reversing this priority. > >>> Why giving high priority to __BYTE_ORDER__? > Any other comment? May I apply with above change? Acked-by: Michael Qiu <michael.qiu@intel.com> ^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH 2/2] app/testpmd: fix endianness detection 2014-12-03 20:47 ` [dpdk-dev] [PATCH 0/2] fix endianness in EAL Thomas Monjalon 2014-12-03 20:47 ` [dpdk-dev] [PATCH 1/2] eal: detect endianness Thomas Monjalon @ 2014-12-03 20:47 ` Thomas Monjalon 2014-12-04 9:28 ` [dpdk-dev] [PATCH 0/2] fix endianness in EAL Chao Zhu 2 siblings, 0 replies; 32+ messages in thread From: Thomas Monjalon @ 2014-12-03 20:47 UTC (permalink / raw) To: dev Use endianness detection factorized in EAL. The comment about arpa/inet.h is not valid anymore since commit d07180f211c08 ("net: fix conflict with libc"). The macro _htons could also be moved in rte_byteorder.h by providing some constant byte swapping. Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com> --- app/test-pmd/config.c | 2 +- app/test-pmd/csumonly.c | 5 ++--- app/test-pmd/testpmd.h | 2 +- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index f0b770e..69a83c2 100644 --- a/app/test-pmd/config.c +++ b/app/test-pmd/config.c @@ -628,7 +628,7 @@ ring_dma_zone_lookup(const char *ring_name, uint8_t port_id, uint16_t q_id) union igb_ring_dword { uint64_t dword; struct { -#ifdef RTE_ARCH_BIG_ENDIAN +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN uint32_t lo; uint32_t hi; #else diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c index 6f43761..da4bca4 100644 --- a/app/test-pmd/csumonly.c +++ b/app/test-pmd/csumonly.c @@ -79,9 +79,8 @@ #define IP_HDRLEN 0x05 /* default IP header length == five 32-bits words. */ #define IP_VHL_DEF (IP_VERSION | IP_HDRLEN) -/* we cannot use htons() from arpa/inet.h due to name conflicts, and we - * cannot use rte_cpu_to_be_16() on a constant in a switch/case */ -#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ +/* We cannot use rte_cpu_to_be_16() on a constant in a switch/case */ +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN #define _htons(x) ((uint16_t)((((x) & 0x00ffU) << 8) | (((x) & 0xff00U) >> 8))) #else #define _htons(x) (x) diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index c925910..f8b0740 100644 --- a/app/test-pmd/testpmd.h +++ b/app/test-pmd/testpmd.h @@ -567,7 +567,7 @@ int tx_queue_id_is_invalid(queueid_t txq_id); #define RTE_BE_TO_CPU_16(be_16_v) rte_be_to_cpu_16((be_16_v)) #define RTE_CPU_TO_BE_16(cpu_16_v) rte_cpu_to_be_16((cpu_16_v)) #else -#ifdef __big_endian__ +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN #define RTE_BE_TO_CPU_16(be_16_v) (be_16_v) #define RTE_CPU_TO_BE_16(cpu_16_v) (cpu_16_v) #else -- 2.1.3 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH 0/2] fix endianness in EAL 2014-12-03 20:47 ` [dpdk-dev] [PATCH 0/2] fix endianness in EAL Thomas Monjalon 2014-12-03 20:47 ` [dpdk-dev] [PATCH 1/2] eal: detect endianness Thomas Monjalon 2014-12-03 20:47 ` [dpdk-dev] [PATCH 2/2] app/testpmd: fix endianness detection Thomas Monjalon @ 2014-12-04 9:28 ` Chao Zhu 2014-12-05 16:01 ` Thomas Monjalon 2 siblings, 1 reply; 32+ messages in thread From: Chao Zhu @ 2014-12-04 9:28 UTC (permalink / raw) To: Thomas Monjalon, dev On 2014/12/4 4:47, Thomas Monjalon wrote: > It can be hard to implement a reliable detection of endianness. > Previous trials in testpmd failed. > The IBM Power patchset introduced a config option. > > This patchset try to improve the situation by having a detection > in EAL headers. > > Please test it (especially on IBM Power) with different toolchains > or distributions. > > Thomas Monjalon (2): > eal: detect endianness > app/testpmd: fix endianness detection > I tested it on IBM Power with GCC 4.8. It works fine. If there is no other better way to do the detection, I think this patch is good enough. Acked-by: Chao Zhu<chaozhu@linux.vnet.ibm.com> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH 0/2] fix endianness in EAL 2014-12-04 9:28 ` [dpdk-dev] [PATCH 0/2] fix endianness in EAL Chao Zhu @ 2014-12-05 16:01 ` Thomas Monjalon 0 siblings, 0 replies; 32+ messages in thread From: Thomas Monjalon @ 2014-12-05 16:01 UTC (permalink / raw) To: Chao Zhu; +Cc: dev > > It can be hard to implement a reliable detection of endianness. > > Previous trials in testpmd failed. > > The IBM Power patchset introduced a config option. > > > > This patchset try to improve the situation by having a detection > > in EAL headers. > > > > Please test it (especially on IBM Power) with different toolchains > > or distributions. > > > > Thomas Monjalon (2): > > eal: detect endianness > > app/testpmd: fix endianness detection > > > I tested it on IBM Power with GCC 4.8. It works fine. > If there is no other better way to do the detection, I think this patch > is good enough. > > Acked-by: Chao Zhu<chaozhu@linux.vnet.ibm.com> Applied -- Thomas ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2014-12-12 6:51 UTC | newest] Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <1417606044-3432-1-git-send-email-michael.qiu@intel.com> [not found] ` <1417606099-3489-1-git-send-email-michael.qiu@intel.com> 2014-12-03 11:42 ` [dpdk-dev] [PATCH] test-pmd: Fix pointer aliasing error Bruce Richardson 2014-12-03 13:59 ` Qiu, Michael 2014-12-03 14:51 ` Bruce Richardson 2014-12-03 15:19 ` Qiu, Michael 2014-12-03 15:36 ` Bruce Richardson 2014-12-04 2:38 ` Qiu, Michael 2014-12-04 3:28 ` [dpdk-dev] [PATCH v2] " Michael Qiu 2014-12-04 4:16 ` [dpdk-dev] [PATCH v3] " Michael Qiu 2014-12-05 5:34 ` Qiu, Michael 2014-12-05 9:24 ` Thomas Monjalon 2014-12-08 1:28 ` Qiu, Michael 2014-12-08 1:30 ` Qiu, Michael 2014-12-10 3:44 ` Qiu, Michael 2014-12-11 0:54 ` Thomas Monjalon 2014-12-11 17:51 ` r k 2014-12-12 6:49 ` Qiu, Michael 2014-12-04 12:54 ` [dpdk-dev] [PATCH v2] " Ananyev, Konstantin 2014-12-03 15:24 ` [dpdk-dev] [PATCH] " Olivier MATZ 2014-12-03 16:03 ` Dayu Qiu 2014-12-03 15:57 ` Dayu Qiu 2014-12-03 16:26 ` [dpdk-dev] [PATCH] test-pmd: Fix "__BYTE_ORDER__" not defined error Qiu, Michael 2014-12-03 19:59 ` Thomas Monjalon 2014-12-03 20:47 ` [dpdk-dev] [PATCH 0/2] fix endianness in EAL Thomas Monjalon 2014-12-03 20:47 ` [dpdk-dev] [PATCH 1/2] eal: detect endianness Thomas Monjalon 2014-12-04 2:28 ` Qiu, Michael 2014-12-04 9:00 ` Thomas Monjalon 2014-12-04 10:28 ` Qiu, Michael 2014-12-04 12:19 ` Thomas Monjalon 2014-12-04 12:50 ` Qiu, Michael 2014-12-03 20:47 ` [dpdk-dev] [PATCH 2/2] app/testpmd: fix endianness detection Thomas Monjalon 2014-12-04 9:28 ` [dpdk-dev] [PATCH 0/2] fix endianness in EAL Chao Zhu 2014-12-05 16:01 ` Thomas Monjalon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).