From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 91F0FA0569; Wed, 11 Mar 2020 13:07:57 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 662901BFF5; Wed, 11 Mar 2020 13:07:56 +0100 (CET) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 63B1B2BAA; Wed, 11 Mar 2020 13:07:54 +0100 (CET) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Mar 2020 05:07:46 -0700 X-IronPort-AV: E=Sophos;i="5.70,540,1574150400"; d="scan'208";a="236418993" Received: from bricha3-mobl.ger.corp.intel.com ([10.237.221.49]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-SHA; 11 Mar 2020 05:07:42 -0700 Date: Wed, 11 Mar 2020 12:07:39 +0000 From: Bruce Richardson To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: Gavin Hu , Ferruh Yigit , dev@dpdk.org, nd , david.marchand@redhat.com, thomas@monjalon.net, ktraynor@redhat.com, jerinj@marvell.com, Honnappa Nagarahalli , Ruifeng Wang , Phil Yang , Joyce Kong , stable@dpdk.org, Olivier MATZ , Konstantin Ananyev , Andrew Rybchenko Message-ID: <20200311120739.GA1922@bricha3-MOBL.ger.corp.intel.com> References: <20200303162728.93744-1-gavin.hu@arm.com> <20200307155629.45021-1-gavin.hu@arm.com> <4135ab73-75d3-421a-264d-2951fc096133@intel.com> <98CBD80474FA8B44BF855DF32C47DC35C60EA3@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35C60EAC@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35C60EAC@smartserver.smartshare.dk> Subject: Re: [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with unnamed union X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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, Mar 11, 2020 at 10:04:33AM +0100, Morten Brørup wrote: > > 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 > > > 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 > > > > >> 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 > > > > >>> --- > > > > >>> v2: > > > > >>> * change 'uint64_t rearm_data' to 'uint_64_t rearm_data[1]' to > > fix > > > > >>> the SFC PMD compiling error on x86. > > > > >>> --- > > > > >>> 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. > Does it still give errors if we don't have the cast in the macro?