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 770F9A0524; Thu, 7 Jan 2021 16:50:26 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3937B140FF0; Thu, 7 Jan 2021 16:50:26 +0100 (CET) Received: from mail-pf1-f172.google.com (mail-pf1-f172.google.com [209.85.210.172]) by mails.dpdk.org (Postfix) with ESMTP id 595C2140FEE for ; Thu, 7 Jan 2021 16:50:24 +0100 (CET) Received: by mail-pf1-f172.google.com with SMTP id c79so4123646pfc.2 for ; Thu, 07 Jan 2021 07:50:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=bkfNczfdLFJhDqe5KUYtY2uhOpyrM7wyrv5GnO/UdDo=; b=ZKlR6u8WGJ1/ONi4Z2YFesaya/tekL62eDKBiT7QtjOmpGDLpAHW12zdVLbP7RC8nu Vq6SEep2bub61hY+NRySxYjoyDpLmB4RGd2VN1mY/UHPAFpb/HYjQxPp6UrwOitREIgp FO+y+IANo+PDTReN/tzZiDyXNns3QMwaDrCKQdZB5aJYoFzCHLBaaSy8SazhBEVt93dN vWCKBKksxIQ7hhlgwx+fLEeEuDHjN4dO49ThPtee4AwfdOs9u4aXi6hbDG0/R5UBvhSl xGPAL6YMAml/2XAE4fzGsXxF2g7UUMPM7dAYFRMn9RHjFnluCJqiIcuLFXREPthi1pWv 2ofw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=bkfNczfdLFJhDqe5KUYtY2uhOpyrM7wyrv5GnO/UdDo=; b=Ph7MjfxmdTYMnuPBtxWe19WPB9iY2Obw/SeM4fGNoAaR5OhntCDUcbCHLjx23AlY6U u58r31QDrJdTJCThv9SQuuE8uenqVOXFIiuRmX0e/pu3BQ0rFcTkuDOBl5x/DkNdgv/m 6KjVzzQTrBFDQjcLH7NMTUnGnQR3LJagYsgvcKp/RmsC1JWMr0T+W0FEM8oUroPOr2Ls C/vifWW/UyWEpZUVRqL2Lfnga5sBvgkGqYwoq0fB9dWFvkpzUgdJnCbfC6iMesjRCZcF oM5XFRLpfubsyn2nDm8PK6/e2g9yJjdxtM7yEscIZ/477idkfXKBkaPUhTxkaRFNwWAb Jl6A== X-Gm-Message-State: AOAM533rkYzh0/P/kplXMcvL7zC9fcRlz0yUbojVWLcQO8XpjFeXZ3JP 2HsbzWLJFz73P5RhfMArsv07ug== X-Google-Smtp-Source: ABdhPJypHSTYyNd+MPWW6T6mhSKLui6DLIqN1Raq0g0CGzRNsBLluEdIxaWx7ane1nLzCpvynSa9uA== X-Received: by 2002:a65:4906:: with SMTP id p6mr2396137pgs.173.1610034623300; Thu, 07 Jan 2021 07:50:23 -0800 (PST) Received: from hermes.local (204-195-22-127.wavecable.com. [204.195.22.127]) by smtp.gmail.com with ESMTPSA id 72sm6415001pfw.177.2021.01.07.07.50.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 07 Jan 2021 07:50:23 -0800 (PST) Date: Thu, 7 Jan 2021 07:50:08 -0800 From: Stephen Hemminger To: George Prekas Cc: Ferruh Yigit , Wenzhuo Lu , Beilei Xing , Bernard Iremonger , Message-ID: <20210107075008.77704541@hermes.local> In-Reply-To: <6539f363-97b1-82b2-1b09-036aa75c9dc9@amazon.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 Wed, 6 Jan 2021 23:39:39 -0600 George Prekas wrote: > On 1/6/2021 12:02 PM, Ferruh Yigit wrote: > > On 12/5/2020 5:42 AM, George Prekas wrote: =20 > >> 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. > >> --- > >> =C2=A0 app/test-pmd/meson.build | 1 + > >> =C2=A0 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 @@ > >> =C2=A0 # override default name to drop the hyphen > >> =C2=A0 name =3D 'testpmd' > >> =C2=A0 cflags +=3D '-Wno-deprecated-declarations' > >> +cflags +=3D '-fno-strict-aliasing' > >> =C2=A0 sources =3D files('5tswap.c', > >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 'cmdline.c', > >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 'cmdline_flow.c', > >> =20 > >=20 > > Hi George, > >=20 > > I am trying to understand this, the relevant code is as below: > > ip_hdr->hdr_checksum =3D ip_sum((unaligned_uint16_t *)ip_hdr, sizeof(*i= p_hdr)); > >=20 > > 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 calculation= s using > > data pointed by 'hdr' pointer, since the 'hdr' pointer is not used to a= lter the > > data and compiler may think data is not changed at all. > >=20 > > 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 dat= a 'hdr' > > pointing is changed. > >=20 > > 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? > >=20 > >=20 > > And if the issue is strict aliasing rule violation as you said, compile= r flag is > > an option but not sure how much it reduces the compiler optimization be= nefit, I > > guess other options also not so good, memcpy brings too much work on ru= ntime and > > union requires bigger change and makes code complex. > > I wonder if making 'ip_sum()' a non inline function can help, can you p= lease > > give a try since you can reproduce it? =20 >=20 > Hi Ferruh, >=20 > Thanks for looking into it. >=20 > I am copy-pasting at the end of this email a minimal reproduction. It cal= culates 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. I= f 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 dif= ferent versions behave. >=20 > My understanding is that the code violates the C standard (https://stacko= verflow.com/a/99010). >=20 > --- cut here ---=20 >=20 > #include > #include > #include > #include >=20 > 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; > }; >=20 > static inline uint16_t ip_sum(const uint16_t *hdr, int hdr_len) > { > uint32_t sum =3D 0; >=20 > while (hdr_len > 1) > { > sum +=3D *hdr++; > if (sum & 0x80000000) > sum =3D (sum & 0xFFFF) + (sum >> 16); > hdr_len -=3D 2; > } >=20 > while (sum >> 16) > sum =3D (sum & 0xFFFF) + (sum >> 16); >=20 > return ~sum; > } >=20 > static void pkt_burst_flow_gen(void) > { > struct rte_ipv4_hdr *ip_hdr =3D (struct rte_ipv4_hdr *) malloc(4096); > memset(ip_hdr, 0, sizeof(*ip_hdr)); > ip_hdr->version_ihl =3D 1; > ip_hdr->type_of_service =3D 2; > ip_hdr->fragment_offset =3D 3; > ip_hdr->time_to_live =3D 4; > ip_hdr->next_proto_id =3D 5; > ip_hdr->packet_id =3D 6; > ip_hdr->src_addr =3D 7; > ip_hdr->dst_addr =3D 8; > ip_hdr->total_length =3D 9; > ip_hdr->hdr_checksum =3D ip_sum((uint16_t *)ip_hdr, sizeof(*ip_hdr)); > printf("%x\n", ip_hdr->hdr_checksum); > } >=20 > int main(void) > { > pkt_burst_flow_gen(); > return 0; > } If I change your code like this to use union, Gcc 10 is still broken. It is a compiler bug. It maybe because optimizer is not smart enough to know that memset has cleared the header. #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 =3D 0; while (hdr_len > 1) { sum +=3D *hdr++; if (sum & 0x80000000) sum =3D (sum & 0xFFFF) + (sum >> 16); hdr_len -=3D 2; } while (sum >> 16) sum =3D (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 =3D malloc(sizeof(*hdr)); memset(hdr, 0, sizeof(*hdr)); hdr->ip.version_ihl =3D 1; hdr->ip.type_of_service =3D 2; hdr->ip.fragment_offset =3D 3; hdr->ip.time_to_live =3D 4; hdr->ip.next_proto_id =3D 5; hdr->ip.packet_id =3D 6; hdr->ip.src_addr =3D 7; hdr->ip.dst_addr =3D 8; hdr->ip.total_length =3D 9; hdr->ip.hdr_checksum =3D ip_sum(hdr->data, sizeof(*hdr)); printf("%x\n", hdr->ip.hdr_checksum); } int main(void) { pkt_burst_flow_gen(); return 0; }