From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Stephen Hemminger <stephen@networkplumber.org>,
George Prekas <prekageo@amazon.com>
Cc: Wenzhuo Lu <wenzhuo.lu@intel.com>,
Beilei Xing <beilei.xing@intel.com>,
Bernard Iremonger <bernard.iremonger@intel.com>,
dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v2] app/testpmd: fix IP checksum calculation
Date: Thu, 7 Jan 2021 15:59:59 +0000 [thread overview]
Message-ID: <6157960e-0ed9-5bd0-a150-f1c59c6f95a4@intel.com> (raw)
In-Reply-To: <20210107075008.77704541@hermes.local>
On 1/7/2021 3:50 PM, Stephen Hemminger wrote:
> On Wed, 6 Jan 2021 23:39:39 -0600
> George Prekas <prekageo@amazon.com> 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).
>>
>> --- 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;
>> }
>
> If I change your code like this to use union, Gcc 10 is still broken.
This worked fine for me: https://godbolt.org/z/vdsxh9
> It is a compiler bug. It maybe because optimizer is not smart enough
> to know that memset has cleared the header.
>
>
> #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)
> {
> union {
> struct rte_ipv4_hdr ip;
> uint16_t data[10];
> } *hdr;
>
> hdr = malloc(sizeof(*hdr));
>
> memset(hdr, 0, sizeof(*hdr));
> hdr->ip.version_ihl = 1;
> hdr->ip.type_of_service = 2;
> hdr->ip.fragment_offset = 3;
> hdr->ip.time_to_live = 4;
> hdr->ip.next_proto_id = 5;
> hdr->ip.packet_id = 6;
> hdr->ip.src_addr = 7;
> hdr->ip.dst_addr = 8;
> hdr->ip.total_length = 9;
> hdr->ip.hdr_checksum = ip_sum(hdr->data, sizeof(*hdr));
> printf("%x\n", hdr->ip.hdr_checksum);
> }
>
> int main(void)
> {
> pkt_burst_flow_gen();
> return 0;
> }
>
next prev parent reply other threads:[~2021-01-07 16:00 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
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 [this message]
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=6157960e-0ed9-5bd0-a150-f1c59c6f95a4@intel.com \
--to=ferruh.yigit@intel.com \
--cc=beilei.xing@intel.com \
--cc=bernard.iremonger@intel.com \
--cc=dev@dpdk.org \
--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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).