From: Ferruh Yigit <ferruh.yigit@intel.com> To: George Prekas <prekageo@amazon.com>, Wenzhuo Lu <wenzhuo.lu@intel.com>, Beilei Xing <beilei.xing@intel.com>, Bernard Iremonger <bernard.iremonger@intel.com> Cc: dev@dpdk.org, Stephen Hemminger <stephen@networkplumber.org>, Harry van Haaren <harry.van.haaren@intel.com> Subject: Re: [dpdk-dev] [PATCH v2] app/testpmd: fix IP checksum calculation Date: Thu, 7 Jan 2021 13:06:49 +0000 Message-ID: <b1737860-1095-efcc-1126-4439a8d38bbe@intel.com> (raw) In-Reply-To: <6a84a356-191a-fca6-607f-3caf88eb3da6@intel.com> On 1/7/2021 11:32 AM, Ferruh Yigit wrote: > On 1/7/2021 5:39 AM, George Prekas wrote: >> >> >> On 1/6/2021 12:02 PM, Ferruh Yigit wrote: >>> On 12/5/2020 5:42 AM, George Prekas wrote: >>>> Strict-aliasing rules are violated by cast to uint16_t* in flowgen.c >>>> and the calculated IP checksum is wrong on GCC 9 and GCC 10. >>>> >>>> Signed-off-by: George Prekas <prekageo@amazon.com> >>>> --- >>>> v2: >>>> * Instead of a compiler barrier, use a compiler flag. >>>> --- >>>> app/test-pmd/meson.build | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/app/test-pmd/meson.build b/app/test-pmd/meson.build >>>> index 7e9c7bdd6..5d24e807f 100644 >>>> --- a/app/test-pmd/meson.build >>>> +++ b/app/test-pmd/meson.build >>>> @@ -4,6 +4,7 @@ >>>> # override default name to drop the hyphen >>>> name = 'testpmd' >>>> cflags += '-Wno-deprecated-declarations' >>>> +cflags += '-fno-strict-aliasing' >>>> sources = files('5tswap.c', >>>> 'cmdline.c', >>>> 'cmdline_flow.c', >>>> >>> >>> Hi George, >>> >>> I am trying to understand this, the relevant code is as below: >>> ip_hdr->hdr_checksum = ip_sum((unaligned_uint16_t *)ip_hdr, sizeof(*ip_hdr)); >>> >>> You are suspicious of strict aliasing rule violation, with more details: >>> The concern is the "struct rte_ipv4_hdr *ip_hdr;" aliased to "const >>> unaligned_uint16_t *hdr", and compiler can optimize out the calculations using >>> data pointed by 'hdr' pointer, since the 'hdr' pointer is not used to alter the >>> data and compiler may think data is not changed at all. >>> >>> 1) But the pointer "hdr" is assigned in the loop, from another pointer whose >>> content is changing, why this is not helping to figure out that the data 'hdr' >>> pointing is changed. >>> >>> 2) I tried to debug this, but I am not able to reproduce the issue, 'ip_sum()' >>> called each time and checksum calculated correctly. Using gcc 10.2.1-9. Can you >>> able to confirm the case with debug, or from the assembly/object file? >>> >>> >>> And if the issue is strict aliasing rule violation as you said, compiler flag is >>> an option but not sure how much it reduces the compiler optimization benefit, I >>> guess other options also not so good, memcpy brings too much work on runtime and >>> union requires bigger change and makes code complex. >>> I wonder if making 'ip_sum()' a non inline function can help, can you please >>> give a try since you can reproduce it? >> >> Hi Ferruh, >> >> Thanks for looking into it. >> >> I am copy-pasting at the end of this email a minimal reproduction. It >> calculates a checksum and prints it. The correct value is f8d9. If you compile >> it with -O0 or -O3 -fno-strict-aliasing, you will get the correct value. If >> you compile it with gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0 and -O3, you will >> get f8e8. You can also try it on https://godbolt.org/ and see how different >> versions behave. >> >> My understanding is that the code violates the C standard >> (https://stackoverflow.com/a/99010). >> > > Thanks for the sample code below, I copied to the godbolt: > https://godbolt.org/z/6fMK19 > > In gcc 10, the checksum calculation is done during compilation (when > optimization is enabled) and the value is returned directly: > mov $0xffed,%esi > > Since a calculation is happening I assume the compiler knows about the aliasing > and OK with it. > > But that optimized calculation seems wrong, when it is disabled [1] the checksum > is correct again. > > [1] all following seems helping to disable compile time calculation > - disabling optimization > - putting a compiler barrier > - putting a 'printf' inside 'ip_sum()' > - fno-strict-aliasing > > gcc 8 & 9 is not doing this compile time calculation, hence they are not affected. > > This feels like an optimization issue in gcc10, but not sure exactly on the root > cause, and how to disable it properly in our case. > As checked with the Harry, latest finding is gcc 10 left out any _non_ uint16_t type variable in sturct during its compile time calculation. Not sure if it is because of broken aliasing or gcc defect, I will report the issue. Meanwhile for short time solution, can you please try force uninline the 'ip_sum()' and try? >> --- cut here --- >> >> #include <stdint.h> >> #include <stdio.h> >> #include <stdlib.h> >> #include <string.h> >> >> struct rte_ipv4_hdr { >> uint8_t version_ihl; >> uint8_t type_of_service; >> uint16_t total_length; >> uint16_t packet_id; >> uint16_t fragment_offset; >> uint8_t time_to_live; >> uint8_t next_proto_id; >> uint16_t hdr_checksum; >> uint32_t src_addr; >> uint32_t dst_addr; >> }; >> >> static inline uint16_t ip_sum(const uint16_t *hdr, int hdr_len) >> { >> uint32_t sum = 0; >> >> while (hdr_len > 1) >> { >> sum += *hdr++; >> if (sum & 0x80000000) >> sum = (sum & 0xFFFF) + (sum >> 16); >> hdr_len -= 2; >> } >> >> while (sum >> 16) >> sum = (sum & 0xFFFF) + (sum >> 16); >> >> return ~sum; >> } >> >> static void pkt_burst_flow_gen(void) >> { >> struct rte_ipv4_hdr *ip_hdr = (struct rte_ipv4_hdr *) malloc(4096); >> memset(ip_hdr, 0, sizeof(*ip_hdr)); >> ip_hdr->version_ihl = 1; >> ip_hdr->type_of_service = 2; >> ip_hdr->fragment_offset = 3; >> ip_hdr->time_to_live = 4; >> ip_hdr->next_proto_id = 5; >> ip_hdr->packet_id = 6; >> ip_hdr->src_addr = 7; >> ip_hdr->dst_addr = 8; >> ip_hdr->total_length = 9; >> ip_hdr->hdr_checksum = ip_sum((uint16_t *)ip_hdr, sizeof(*ip_hdr)); >> printf("%x\n", ip_hdr->hdr_checksum); >> } >> >> int main(void) >> { >> pkt_burst_flow_gen(); >> return 0; >> } >> >
next prev parent reply other threads:[~2021-01-07 13:06 UTC|newest] Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-12-03 13:59 [dpdk-dev] [PATCH] " George Prekas 2020-12-03 16:08 ` Stephen Hemminger 2020-12-03 16:35 ` George Prekas 2020-12-03 18:33 ` Stephen Hemminger 2020-12-04 8:59 ` Ferruh Yigit 2020-12-05 5:47 ` George Prekas 2020-12-05 5:42 ` [dpdk-dev] [PATCH v2] " George Prekas 2021-01-05 16:26 ` George Prekas 2021-01-06 18:02 ` Ferruh Yigit 2021-01-07 5:25 ` Stephen Hemminger 2021-01-07 5:39 ` George Prekas 2021-01-07 11:32 ` Ferruh Yigit 2021-01-07 13:06 ` Ferruh Yigit [this message] 2021-01-07 14:20 ` George Prekas 2021-01-07 15:22 ` Ferruh Yigit 2021-01-07 20:45 ` George Prekas 2021-01-07 15:50 ` Stephen Hemminger 2021-01-07 15:59 ` Ferruh Yigit 2021-01-07 16:29 ` Stephen Hemminger 2021-01-07 20:42 ` [dpdk-dev] [PATCH v3] " George Prekas 2021-01-18 15:20 ` Ferruh Yigit
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=b1737860-1095-efcc-1126-4439a8d38bbe@intel.com \ --to=ferruh.yigit@intel.com \ --cc=beilei.xing@intel.com \ --cc=bernard.iremonger@intel.com \ --cc=dev@dpdk.org \ --cc=harry.van.haaren@intel.com \ --cc=prekageo@amazon.com \ --cc=stephen@networkplumber.org \ --cc=wenzhuo.lu@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
DPDK patches and discussions This inbox may be cloned and mirrored by anyone: git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \ dev@dpdk.org public-inbox-index dev Example config snippet for mirrors. Newsgroup available over NNTP: nntp://inbox.dpdk.org/inbox.dpdk.dev AGPL code for this site: git clone https://public-inbox.org/public-inbox.git