DPDK patches and discussions
 help / color / mirror / Atom feed
From: Stephen Hemminger <stephen@networkplumber.org>
To: Ferruh Yigit <ferruh.yigit@intel.com>
Cc: George Prekas <prekageo@amazon.com>,
	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: Wed, 6 Jan 2021 21:25:04 -0800
Message-ID: <20210106212504.24448d9d@hermes.local> (raw)
In-Reply-To: <8c1fd6ad-302d-f45e-f48a-e81fd5f8ba85@intel.com>

On Wed, 6 Jan 2021 18:02:49 +0000
Ferruh Yigit <ferruh.yigit@intel.com> 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?

If it is an aliasing problem, it should be fixed with a union instead of a compiler flag.

  reply	other threads:[~2021-01-07  5:25 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 [this message]
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
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=20210106212504.24448d9d@hermes.local \
    --to=stephen@networkplumber.org \
    --cc=beilei.xing@intel.com \
    --cc=bernard.iremonger@intel.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=prekageo@amazon.com \
    --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