Hi Bruce, > -----Original Message----- > From: Bruce Richardson <bruce.richard...@intel.com> > Sent: Wednesday, March 11, 2020 8:08 PM > To: Morten Brørup <m...@smartsharesystems.com> > Cc: Gavin Hu <gavin...@arm.com>; Ferruh Yigit <ferruh.yi...@intel.com>; > dev@dpdk.org; 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 > > On Wed, Mar 11, 2020 at 10:04:33AM +0100, Morten Brørup wrote: > > > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Gavin Hu > > > Sent: Wednesday, March 11, 2020 8:50 AM > > > > > > Hi Morten, > > > > > > > From: Morten Brørup <m...@smartsharesystems.com> > > > > Sent: Monday, March 9, 2020 9:31 PM > > > > > > > > > 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) > > > > > > > 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?
Yes the errors persist if we have the cast in dereferencing.
