Hi Kevin, > -----Original Message----- > From: Kevin Traynor <ktray...@redhat.com> > Sent: Wednesday, April 8, 2020 1:14 AM > To: Gavin Hu <gavin...@arm.com>; Bruce Richardson > <bruce.richard...@intel.com>; Morten Brørup > <m...@smartsharesystems.com> > Cc: Ferruh Yigit <ferruh.yi...@intel.com>; dev@dpdk.org; nd > <n...@arm.com>; david.march...@redhat.com; tho...@monjalon.net; > 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 13/03/2020 09:22, Gavin Hu wrote: > > 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, it gives errors elsewhere that have the cast. > > > > Hi Gavin, I lost track if v2 is still a candidate for merge. fwiw, it > compiles without giving the zero-length-bounds warning on my system. > > Kevin.
Yes, this path alone is a candidate for merge. We brainstormed other solutions but they did not work out. /Gavin
