> -----Original Message----- > From: dev <dev-boun...@dpdk.org> On Behalf Of Morten Brørup > Sent: Monday, March 9, 2020 1:31 PM > To: Yigit, Ferruh <ferruh.yi...@intel.com>; Gavin Hu <gavin...@arm.com>; > dev@dpdk.org > Cc: nd <n...@arm.com>; david.march...@redhat.com; tho...@monjalon.net; > ktray...@redhat.com; jer...@marvell.com; Honnappa Nagarahalli > <honnappa.nagaraha...@arm.com>; Ruifeng Wang <ruifeng.w...@arm.com>; Phil > Yang <phil.y...@arm.com>; Joyce Kong <joyce.k...@arm.com>; > sta...@dpdk.org; Olivier MATZ <olivier.m...@6wind.com>; Ananyev, > Konstantin <konstantin.anan...@intel.com>; Andrew Rybchenko > <arybche...@solarflare.com> > Subject: Re: [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with > unnamed union > > > From: dev [mailto:dev-boun...@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.yi...@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: sta...@dpdk.org > > >>> > > >>> Signed-off-by: Gavin Hu <gavin...@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) > > +1 for this, it's very similar to what I was going to suggest, which was:
uint16_t data_off; #define _REARM_DATA data_off but your suggestion is probably cleaner than mine. /Bruce