Re: [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with unnamed union

["Morten Brørup" <mb@smartsharesystems.com>]

> From: dev [mailto:dev-bounces@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.yigit@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: stable@dpdk.org
> >>>
> >>> Signed-off-by: Gavin Hu <gavin.hu@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)


Med venlig hilsen / kind regards
- Morten Brørup