patches for DPDK stable branches
 help / color / mirror / Atom feed
From: Morten Brørup <mb@smartsharesystems.com>
To: "Gavin Hu" <Gavin.Hu@arm.com>,
	"Ferruh Yigit" <ferruh.yigit@intel.com>, <dev@dpdk.org>
Cc: "nd" <nd@arm.com>, <david.marchand@redhat.com>,
	<thomas@monjalon.net>, <ktraynor@redhat.com>,
	<jerinj@marvell.com>,
	"Honnappa Nagarahalli" <Honnappa.Nagarahalli@arm.com>,
	"Ruifeng Wang" <Ruifeng.Wang@arm.com>,
	"Phil Yang" <Phil.Yang@arm.com>,
	"Joyce Kong" <Joyce.Kong@arm.com>, <stable@dpdk.org>,
	"Olivier MATZ" <olivier.matz@6wind.com>,
	"Konstantin Ananyev" <konstantin.ananyev@intel.com>,
	"Andrew Rybchenko" <arybchenko@solarflare.com>, "nd" <nd@arm.com>
Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with unnamed union
Date: Wed, 11 Mar 2020 10:04:33 +0100
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35C60EAC@smartserver.smartshare.dk> (raw)
In-Reply-To: <VI1PR08MB5376761A8FD748CA357472C78FFC0@VI1PR08MB5376.eurprd08.prod.outlook.com>

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Gavin Hu
> Sent: Wednesday, March 11, 2020 8:50 AM
> 
> Hi Morten,
> 
> > From: Morten Brørup <mb@smartsharesystems.com>
> > Sent: Monday, March 9, 2020 9:31 PM
> >
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> > > Sent: Monday, March 9, 2020 12:30 PM
> > >
> > > On 3/9/2020 9:45 AM, Gavin Hu wrote:
> > > > Hi Ferruh,
> > > >
> > > >> -----Original Message-----
> > > >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> > > >> Sent: Monday, March 9, 2020 4:55 PM
> > > >>
> > > >> On 3/7/2020 3:56 PM, Gavin Hu wrote:
> > > >>> Declaring zero-length arrays in other contexts, including as
> > > interior
> > > >>> members of structure objects or as non-member objects, is
> > > discouraged.
> > > >>> Accessing elements of zero-length arrays declared in such
> contexts
> > > is
> > > >>> undefined and may be diagnosed.[1]
> > > >>>
> > > >>> Fix by using unnamed union and struct.
> > > >>>
> > > >>> https://bugs.dpdk.org/show_bug.cgi?id=396
> > > >>>
> > > >>> Bugzilla ID: 396
> > > >>>
> > > >>> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> > > >>>
> > > >>> Fixes: 3e6181b07038 ("mbuf: use structure marker from EAL")
> > > >>> Cc: stable@dpdk.org
> > > >>>
> > > >>> Signed-off-by: Gavin Hu <gavin.hu@arm.com>
> > > >>> ---
> > > >>> v2:
> > > >>> * change 'uint64_t rearm_data' to 'uint_64_t rearm_data[1]' to
> fix
> > > >>>   the SFC PMD compiling error on x86. <Kevin Traynor>
> > > >>> ---
> > > >>>  lib/librte_mbuf/rte_mbuf_core.h | 54 +++++++++++++++++++------
> ----
> > > ----
> > > >>>  1 file changed, 32 insertions(+), 22 deletions(-)
> > > >>>
> > > >>> diff --git a/lib/librte_mbuf/rte_mbuf_core.h
> > > >> b/lib/librte_mbuf/rte_mbuf_core.h
> > > >>> index b9a59c879..34cb152e2 100644
> > > >>> --- a/lib/librte_mbuf/rte_mbuf_core.h
> > > >>> +++ b/lib/librte_mbuf/rte_mbuf_core.h
> > > >>> @@ -480,31 +480,41 @@ struct rte_mbuf {
> > > >>>  		rte_iova_t buf_physaddr; /**< deprecated */
> > > >>>  	} __rte_aligned(sizeof(rte_iova_t));
> > > >>>
> > > >>> -	/* next 8 bytes are initialised on RX descriptor rearm */
> > > >>> -	RTE_MARKER64 rearm_data;
> > > >>> -	uint16_t data_off;
> > > >>> -
> > > >>> -	/**
> > > >>> -	 * Reference counter. Its size should at least equal to the
> size
> > > >>> -	 * of port field (16 bits), to support zero-copy broadcast.
> > > >>> -	 * It should only be accessed using the following
> functions:
> > > >>> -	 * rte_mbuf_refcnt_update(), rte_mbuf_refcnt_read(), and
> > > >>> -	 * rte_mbuf_refcnt_set(). The functionality of these
> functions
> > > (atomic,
> > > >>> -	 * or non-atomic) is controlled by the
> > > >> CONFIG_RTE_MBUF_REFCNT_ATOMIC
> > > >>> -	 * config option.
> > > >>> -	 */
> > > >>>  	RTE_STD_C11
> > > >>>  	union {
> > > >>> -		rte_atomic16_t refcnt_atomic; /**< Atomically
> accessed
> > > >> refcnt */
> > > >>> -		/** Non-atomically accessed refcnt */
> > > >>> -		uint16_t refcnt;
> > > >>> -	};
> > > >>> -	uint16_t nb_segs;         /**< Number of segments. */
> > > >>> +		/* next 8 bytes are initialised on RX descriptor
> rearm */
> > > >>> +		uint64_t rearm_data[1];
> > > >> We are using zero length array as markers only and know what we
> are
> > > doing
> > > >> with them,
> > > >> what would you think disabling the warning instead of increasing
> the
> > > >> complexity
> > > >> in mbuf struct?
> > > > Okay, I will add -Wno-zero-length-bounds to the compiler
> toolchain
> > > flags.
> > >
> > > This would be my preference but I would like to get more input, can
> you
> > > please
> > > for more comments before changing the implementation in case there
> are
> > > some
> > > strong opinion on it?
> > >
> >
> > I have some input to this discussion.
> >
> > Let me repeat what Gavin's GCC reference states: Declaring zero-
> length
> > arrays [...] as interior members of structure objects [...] is
> discouraged.
> >
> > Why would we do something that the compiler documentation says is
> > discouraged? I think the problem (i.e. using discouraged techniques)
> should
> > be fixed, not the symptom (i.e. getting warnings about using
> discouraged
> > techniques).
> >
> > Compiler warnings are here to help, and in my experience they are
> actually
> > very helpful, although avoiding them often requires somewhat more
> > verbose source code. Disabling this warning not only affects this
> file, but
> > disables warnings about potential bugs in other source code too.
> >
> > Generally, disabling compiler warnings is a slippery slope. It would
> be
> > optimal if DPDK could be compiled with -Wall, and it would probably
> reduce
> > the number of released bugs too.
> >
> > With that said, sometimes the optimal solution has to give way for
> the
> > practical solution. And this is a core file, so we should thread
> lightly.
> >
> >
> > As for an alternative solution, perhaps we can get rid of the MARKERs
> in the
> > struct and #define them instead. Not as elegant as Gavin's suggested
> union
> > based solution, but it might bring inspiration...
> >
> > struct rte_mbuf {
> >     ...
> >     } __rte_aligned(sizeof(rte_iova_t));
> >
> >     uint16_t data_off;
> >     ...
> > }
> >
> > #define rte_mbuf_rearm_data(m) ((uint64_t *)m->data_off)
> 
> This does not work out, it generates new errors:
> /root/dpdk/build/include/rte_mbuf_core.h:485:33: error: dereferencing
> type-punned pointer will break strict-aliasing rules [-Werror=strict-
> aliasing]
>   485 | #define rte_mbuf_rearm_data(m) ((uint64_t *)&m->data_off)
> 

OK. Then Bruce's suggestion probably won't work either.

I found this article about strict aliasing: https://gist.github.com/shafik/848ae25ee209f698763cffee272a58f8

The article basically says that the union based method (i.e. your original suggestion) is valid C (but not C++) and is the common solution.

Alternatives have now been discussed and tested, so we should all support your original suggestion, which seems to be the only correct and viable solution.

Please go ahead with that, and then someone should update the SFC PMD accordingly.

Furthermore, I think that Stephen's suggestion about getting rid of the markers all together is good thinking, but it would require updating a lot of PMDs accordingly. So please also consider removing other markers that can be removed without affecting a whole bunch of other files.

-Morten


  reply	other threads:[~2020-03-11  9:04 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-03 16:27 [dpdk-stable] [PATCH v1] " Gavin Hu
2020-03-04 12:32 ` [dpdk-stable] [dpdk-dev] " Kevin Traynor
2020-03-07 14:52   ` Gavin Hu
2020-03-07 15:56 ` [dpdk-stable] [PATCH v2] " Gavin Hu
2020-03-09  8:55   ` [dpdk-stable] [dpdk-dev] " Ferruh Yigit
2020-03-09  9:45     ` Gavin Hu
2020-03-09 11:29       ` Ferruh Yigit
2020-03-09 13:30         ` Morten Brørup
2020-03-09 14:16           ` Richardson, Bruce
2020-03-09 14:50             ` [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length markerwith " Morten Brørup
2020-03-11  7:50           ` [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with " Gavin Hu
2020-03-11  9:04             ` Morten Brørup [this message]
2020-03-11 12:07               ` Bruce Richardson
2020-03-13  7:36                 ` Gavin Hu
2020-03-13  9:22                 ` Gavin Hu
2020-04-07 17:13                   ` Kevin Traynor
2020-04-08 15:04                     ` Gavin Hu
2020-04-08 15:22                       ` David Marchand
2020-04-09  9:48                         ` Gavin Hu
2020-04-09 10:49                           ` Thomas Monjalon
2020-04-09 16:09                             ` Ray Kinsella
2020-04-11  2:50                             ` Gavin Hu
2020-05-14 13:24                         ` Kevin Traynor
2020-03-09 15:47     ` Stephen Hemminger

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=98CBD80474FA8B44BF855DF32C47DC35C60EAC@smartserver.smartshare.dk \
    --to=mb@smartsharesystems.com \
    --cc=Gavin.Hu@arm.com \
    --cc=Honnappa.Nagarahalli@arm.com \
    --cc=Joyce.Kong@arm.com \
    --cc=Phil.Yang@arm.com \
    --cc=Ruifeng.Wang@arm.com \
    --cc=arybchenko@solarflare.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=jerinj@marvell.com \
    --cc=konstantin.ananyev@intel.com \
    --cc=ktraynor@redhat.com \
    --cc=nd@arm.com \
    --cc=olivier.matz@6wind.com \
    --cc=stable@dpdk.org \
    --cc=thomas@monjalon.net \
    /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

patches for DPDK stable branches

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/stable/0 stable/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 stable stable/ https://inbox.dpdk.org/stable \
		stable@dpdk.org
	public-inbox-index stable

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.stable


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git