Hi Morten, > -----Original Message----- > From: Morten Brørup <m...@smartsharesystems.com> > Sent: Monday, March 9, 2020 9:31 PM > To: Ferruh Yigit <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>; Konstantin Ananyev > <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)
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) > > > Med venlig hilsen / kind regards > - Morten Brørup
