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.