From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (xvm-189-124.dc0.ghst.net [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 90B9FA0524; Thu, 7 Jan 2021 12:32:31 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 44051140F43; Thu, 7 Jan 2021 12:32:31 +0100 (CET) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by mails.dpdk.org (Postfix) with ESMTP id E7704140F41 for ; Thu, 7 Jan 2021 12:32:28 +0100 (CET) IronPort-SDR: 1XtBr7gmdzvdfCG/4pSMaKBOLmO0z202jPXnkc2fXrilkGzD8/eaDTBHS7ybvxRd3NRIMZTJ3w UwHrp11FQANQ== X-IronPort-AV: E=McAfee;i="6000,8403,9856"; a="177564009" X-IronPort-AV: E=Sophos;i="5.79,329,1602572400"; d="scan'208";a="177564009" Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Jan 2021 03:32:27 -0800 IronPort-SDR: f2BcJ/egqD9BUH08u3ZurlFRj5B334FSFEW1pdl+ZSQI+VzrR6kc5QO/PKCEq7eHzjdbDeppcn ZoWCLXizmLwQ== X-IronPort-AV: E=Sophos;i="5.79,329,1602572400"; d="scan'208";a="361938579" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.252.29.165]) ([10.252.29.165]) by orsmga002-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Jan 2021 03:32:26 -0800 To: George Prekas , Wenzhuo Lu , Beilei Xing , Bernard Iremonger Cc: dev@dpdk.org, Stephen Hemminger References: <20201203135954.1127-1-prekageo@amazon.com> <20201205054238.12469-1-prekageo@amazon.com> <8c1fd6ad-302d-f45e-f48a-e81fd5f8ba85@intel.com> <6539f363-97b1-82b2-1b09-036aa75c9dc9@amazon.com> From: Ferruh Yigit Message-ID: <6a84a356-191a-fca6-607f-3caf88eb3da6@intel.com> Date: Thu, 7 Jan 2021 11:32:22 +0000 MIME-Version: 1.0 In-Reply-To: <6539f363-97b1-82b2-1b09-036aa75c9dc9@amazon.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2] app/testpmd: fix IP checksum calculation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 >>> --- >>> 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. > --- cut here --- > > #include > #include > #include > #include > > 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; > } >