DPDK patches and discussions
 help / color / mirror / Atom feed
From: George Prekas <prekageo@amazon.com>
To: Ferruh Yigit <ferruh.yigit@intel.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>,
	"George Prekas (prekageo)" <prekageo@amazon.com>
Subject: Re: [dpdk-dev] [PATCH v2] app/testpmd: fix IP checksum calculation
Date: Thu, 7 Jan 2021 08:20:29 -0600
Message-ID: <f2062f71-0625-ebe7-4469-0a5513368153@amazon.com> (raw)
In-Reply-To: <6a84a356-191a-fca6-607f-3caf88eb3da6@intel.com>

On 1/7/2021 5: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.

According to https://gcc.gnu.org/bugs/: "if compiling with -fno-strict-aliasing -fwrapv
-fno-aggressive-loop-optimizations makes a difference ... then your code is probably not
correct"

> 
> 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.

I just checked gcc 8.3 and gcc 9.3 on godbolt and I got f8e8 (which is wrong; the correct
is f8d9). 

> 
> 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.

I've tried with __attribute__ ((noinline)) and it fixes the problem. But keep in mind
that we are dealing with broken C code. This attribute just prevents the optimization that
reveals the problem. It does not guarantee that the problem will not reappear in a future
compiler version.

I've also tried to use a union as suggested by Stephen Hemminger and it works correctly but
it requires significant code changes: you have to copy paste the IP header structure inside
a union and access it only through the union.

As a side note, here is a piece of opinion from Linus Torvalds regarding strict aliasing:
https://lkml.org/lkml/2018/6/5/769

DPDK already uses -fno-strict-aliasing for librte_node and librte_vhost.

> 
>> --- 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;
>> }
>>
> 

  parent reply	other threads:[~2021-01-07 14:20 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 [this message]
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=f2062f71-0625-ebe7-4469-0a5513368153@amazon.com \
    --to=prekageo@amazon.com \
    --cc=beilei.xing@intel.com \
    --cc=bernard.iremonger@intel.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.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