diff mbox series

[v2] mbuf: replace zero-length marker with unnamed union

Message ID 20200307155629.45021-1-gavin.hu@arm.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v2] mbuf: replace zero-length marker with unnamed union |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/travis-robot warning Travis build: failed
ci/Intel-compilation success Compilation OK

Commit Message

Gavin Hu March 7, 2020, 3:56 p.m. UTC 
  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(-)

Comments

Ferruh Yigit March 9, 2020, 8:55 a.m. UTC | #1 
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?
Gavin Hu March 9, 2020, 9:45 a.m. UTC | #2 
Hi Ferruh,

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Monday, March 9, 2020 4:55 PM
> To: Gavin Hu <Gavin.Hu@arm.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>; david.marchand@redhat.com; thomas@monjalon.net;
> ktraynor@redhat.com; jerinj@marvell.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; Phil Yang <Phil.Yang@arm.com>; Joyce Kong
> <Joyce.Kong@arm.com>; stable@dpdk.org; Olivier MATZ
> <olivier.matz@6wind.com>; Konstantin Ananyev
> <konstantin.ananyev@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>
> Subject: Re: [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with
> unnamed union
> 
> 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. 
/Gavin
Ferruh Yigit March 9, 2020, 11:29 a.m. UTC | #3 
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
>> To: Gavin Hu <Gavin.Hu@arm.com>; dev@dpdk.org
>> Cc: nd <nd@arm.com>; david.marchand@redhat.com; thomas@monjalon.net;
>> ktraynor@redhat.com; jerinj@marvell.com; Honnappa Nagarahalli
>> <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
>> <Ruifeng.Wang@arm.com>; Phil Yang <Phil.Yang@arm.com>; Joyce Kong
>> <Joyce.Kong@arm.com>; stable@dpdk.org; Olivier MATZ
>> <olivier.matz@6wind.com>; Konstantin Ananyev
>> <konstantin.ananyev@intel.com>; Andrew Rybchenko
>> <arybchenko@solarflare.com>
>> Subject: Re: [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with
>> unnamed union
>>
>> 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?
Morten Brørup March 9, 2020, 1:30 p.m. UTC | #4 
> 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
Bruce Richardson March 9, 2020, 2:16 p.m. UTC | #5 
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Morten Brørup
> Sent: Monday, March 9, 2020 1:31 PM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Gavin Hu <Gavin.Hu@arm.com>;
> dev@dpdk.org
> Cc: nd <nd@arm.com>; david.marchand@redhat.com; thomas@monjalon.net;
> ktraynor@redhat.com; jerinj@marvell.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang <Ruifeng.Wang@arm.com>; Phil
> Yang <Phil.Yang@arm.com>; Joyce Kong <Joyce.Kong@arm.com>;
> stable@dpdk.org; Olivier MATZ <olivier.matz@6wind.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>
> Subject: Re: [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with
> unnamed union
> 
> > 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)
> 
> 
+1 for this, it's very similar to what I was going to suggest, which was:

uint16_t data_off;
#define _REARM_DATA data_off

but your suggestion is probably cleaner than mine.

/Bruce
Morten Brørup March 9, 2020, 2:50 p.m. UTC | #6 
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Richardson, Bruce
> Sent: Monday, March 9, 2020 3:16 PM
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Morten Brørup
> > Sent: Monday, March 9, 2020 1:31 PM
> > > 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)
> >
> >
> +1 for this, it's very similar to what I was going to suggest, which
> was:
> 
> uint16_t data_off;
> #define _REARM_DATA data_off
> 
> but your suggestion is probably cleaner than mine.
> 
> /Bruce

I prefer Gavin's academically correct solution, i.e. using unions.

But check out the code using rearm_data! It's all drivers, and they all type cast it, except the SFC PMD.

So Bruce's suggestion is probably more workable than mine, although the #define'd name is missing an RTE prefix. (And mine is missing an &.)


And a completely alternative route is open: Type cast rearm_data in the SFC PMD, and postpone the MARKER discussion to later.

-Morten
Stephen Hemminger March 9, 2020, 3:47 p.m. UTC | #7 
On Mon, 9 Mar 2020 08:55:05 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> 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?

No to disabling warnings.

Or get rid of the markers all together, the usage is awkward already.
Gavin Hu March 11, 2020, 7:50 a.m. UTC | #8 
Hi Morten,

> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Monday, March 9, 2020 9:31 PM
> To: Ferruh Yigit <ferruh.yigit@intel.com>; Gavin Hu <Gavin.Hu@arm.com>;
> dev@dpdk.org
> Cc: nd <nd@arm.com>; david.marchand@redhat.com;
> thomas@monjalon.net; ktraynor@redhat.com; jerinj@marvell.com;
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; Phil Yang <Phil.Yang@arm.com>; Joyce Kong
> <Joyce.Kong@arm.com>; stable@dpdk.org; Olivier MATZ
> <olivier.matz@6wind.com>; Konstantin Ananyev
> <konstantin.ananyev@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>
> Subject: RE: [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with
> unnamed union
> 
> > 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)

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
Morten Brørup March 11, 2020, 9:04 a.m. UTC | #9 
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Gavin Hu
> Sent: Wednesday, March 11, 2020 8:50 AM
> 
> Hi Morten,
> 
> > From: Morten Brørup <mb@smartsharesystems.com>
> > Sent: Monday, March 9, 2020 9:31 PM
> >
> > > 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)
> 
> 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.

-Morten
Bruce Richardson March 11, 2020, 12:07 p.m. UTC | #10 
On Wed, Mar 11, 2020 at 10:04:33AM +0100, Morten Brørup wrote:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Gavin Hu
> > Sent: Wednesday, March 11, 2020 8:50 AM
> > 
> > Hi Morten,
> > 
> > > From: Morten Brørup <mb@smartsharesystems.com>
> > > Sent: Monday, March 9, 2020 9:31 PM
> > >
> > > > 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)
> > 
> > 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?
Gavin Hu March 13, 2020, 7:36 a.m. UTC | #11 
Hi Bruce,

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Wednesday, March 11, 2020 8:08 PM
> To: Morten Brørup <mb@smartsharesystems.com>
> Cc: Gavin Hu <Gavin.Hu@arm.com>; Ferruh Yigit <ferruh.yigit@intel.com>;
> dev@dpdk.org; nd <nd@arm.com>; david.marchand@redhat.com;
> thomas@monjalon.net; ktraynor@redhat.com; jerinj@marvell.com;
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; Phil Yang <Phil.Yang@arm.com>; Joyce Kong
> <Joyce.Kong@arm.com>; stable@dpdk.org; Olivier MATZ
> <olivier.matz@6wind.com>; Konstantin Ananyev
> <konstantin.ananyev@intel.com>; Andrew Rybchenko
> <arybchenko@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-bounces@dpdk.org] On Behalf Of Gavin Hu
> > > Sent: Wednesday, March 11, 2020 8:50 AM
> > >
> > > Hi Morten,
> > >
> > > > From: Morten Brørup <mb@smartsharesystems.com>
> > > > Sent: Monday, March 9, 2020 9:31 PM
> > > >
> > > > > 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)
> > >
> > > 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.
Gavin Hu March 13, 2020, 9:22 a.m. UTC | #12 
Hi Bruce,

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Wednesday, March 11, 2020 8:08 PM
> To: Morten Brørup <mb@smartsharesystems.com>
> Cc: Gavin Hu <Gavin.Hu@arm.com>; Ferruh Yigit <ferruh.yigit@intel.com>;
> dev@dpdk.org; nd <nd@arm.com>; david.marchand@redhat.com;
> thomas@monjalon.net; ktraynor@redhat.com; jerinj@marvell.com;
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; Phil Yang <Phil.Yang@arm.com>; Joyce Kong
> <Joyce.Kong@arm.com>; stable@dpdk.org; Olivier MATZ
> <olivier.matz@6wind.com>; Konstantin Ananyev
> <konstantin.ananyev@intel.com>; Andrew Rybchenko
> <arybchenko@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-bounces@dpdk.org] On Behalf Of Gavin Hu
> > > Sent: Wednesday, March 11, 2020 8:50 AM
> > >
> > > Hi Morten,
> > >
> > > > From: Morten Brørup <mb@smartsharesystems.com>
> > > > Sent: Monday, March 9, 2020 9:31 PM
> > > >
> > > > > 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)
> > >
> > > 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.
Kevin Traynor April 7, 2020, 5:13 p.m. UTC | #13 
On 13/03/2020 09:22, Gavin Hu wrote:
> Hi Bruce,
> 
>> -----Original Message-----
>> From: Bruce Richardson <bruce.richardson@intel.com>
>> Sent: Wednesday, March 11, 2020 8:08 PM
>> To: Morten Brørup <mb@smartsharesystems.com>
>> Cc: Gavin Hu <Gavin.Hu@arm.com>; Ferruh Yigit <ferruh.yigit@intel.com>;
>> dev@dpdk.org; nd <nd@arm.com>; david.marchand@redhat.com;
>> thomas@monjalon.net; ktraynor@redhat.com; jerinj@marvell.com;
>> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
>> <Ruifeng.Wang@arm.com>; Phil Yang <Phil.Yang@arm.com>; Joyce Kong
>> <Joyce.Kong@arm.com>; stable@dpdk.org; Olivier MATZ
>> <olivier.matz@6wind.com>; Konstantin Ananyev
>> <konstantin.ananyev@intel.com>; Andrew Rybchenko
>> <arybchenko@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-bounces@dpdk.org] On Behalf Of Gavin Hu
>>>> Sent: Wednesday, March 11, 2020 8:50 AM
>>>>
>>>> Hi Morten,
>>>>
>>>>> From: Morten Brørup <mb@smartsharesystems.com>
>>>>> Sent: Monday, March 9, 2020 9:31 PM
>>>>>
>>>>>> 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)
>>>>
>>>> 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.
Gavin Hu April 8, 2020, 3:04 p.m. UTC | #14 
Hi Kevin, 

> -----Original Message-----
> From: Kevin Traynor <ktraynor@redhat.com>
> Sent: Wednesday, April 8, 2020 1:14 AM
> To: Gavin Hu <Gavin.Hu@arm.com>; Bruce Richardson
> <bruce.richardson@intel.com>; Morten Brørup
> <mb@smartsharesystems.com>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; nd
> <nd@arm.com>; david.marchand@redhat.com; thomas@monjalon.net;
> jerinj@marvell.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; Phil Yang <Phil.Yang@arm.com>; Joyce Kong
> <Joyce.Kong@arm.com>; stable@dpdk.org; Olivier MATZ
> <olivier.matz@6wind.com>; Konstantin Ananyev
> <konstantin.ananyev@intel.com>; Andrew Rybchenko
> <arybchenko@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.richardson@intel.com>
> >> Sent: Wednesday, March 11, 2020 8:08 PM
> >> To: Morten Brørup <mb@smartsharesystems.com>
> >> Cc: Gavin Hu <Gavin.Hu@arm.com>; Ferruh Yigit
> <ferruh.yigit@intel.com>;
> >> dev@dpdk.org; nd <nd@arm.com>; david.marchand@redhat.com;
> >> thomas@monjalon.net; ktraynor@redhat.com; jerinj@marvell.com;
> >> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ruifeng
> Wang
> >> <Ruifeng.Wang@arm.com>; Phil Yang <Phil.Yang@arm.com>; Joyce Kong
> >> <Joyce.Kong@arm.com>; stable@dpdk.org; Olivier MATZ
> >> <olivier.matz@6wind.com>; Konstantin Ananyev
> >> <konstantin.ananyev@intel.com>; Andrew Rybchenko
> >> <arybchenko@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-bounces@dpdk.org] On Behalf Of Gavin Hu
> >>>> Sent: Wednesday, March 11, 2020 8:50 AM
> >>>>
> >>>> Hi Morten,
> >>>>
> >>>>> From: Morten Brørup <mb@smartsharesystems.com>
> >>>>> Sent: Monday, March 9, 2020 9:31 PM
> >>>>>
> >>>>>> 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)
> >>>>
> >>>> 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
David Marchand April 8, 2020, 3:22 p.m. UTC | #15 
On Wed, Apr 8, 2020 at 5:05 PM Gavin Hu <Gavin.Hu@arm.com> wrote:
> > -----Original Message-----
> > From: Kevin Traynor <ktraynor@redhat.com>
> > 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.

This patch is not mergeable, it would trigger failures in the ABI checks.

You can see in patchwork that the robot reported a warning in Travis.
http://mails.dpdk.org/archives/test-report/2020-March/119919.html
https://travis-ci.com/github/ovsrobot/dpdk/jobs/295652710#L4476


I opened a bz to libabigail.
https://sourceware.org/bugzilla/show_bug.cgi?id=25661


Either a different solution is found, or your patch will have to deal
with this issue (libabigail fix won't be ready soon afaik) and waive
this.
Gavin Hu April 9, 2020, 9:48 a.m. UTC | #16 
Hi David,

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, April 8, 2020 11:22 PM
> To: Gavin Hu <Gavin.Hu@arm.com>
> Cc: Kevin Traynor <ktraynor@redhat.com>; Bruce Richardson
> <bruce.richardson@intel.com>; Morten Brørup
> <mb@smartsharesystems.com>; Ferruh Yigit <ferruh.yigit@intel.com>;
> dev@dpdk.org; nd <nd@arm.com>; thomas@monjalon.net;
> jerinj@marvell.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; Phil Yang <Phil.Yang@arm.com>; Joyce Kong
> <Joyce.Kong@arm.com>; stable@dpdk.org; Olivier MATZ
> <olivier.matz@6wind.com>; Konstantin Ananyev
> <konstantin.ananyev@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>
> Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length
> marker with unnamed union
> 
> On Wed, Apr 8, 2020 at 5:05 PM Gavin Hu <Gavin.Hu@arm.com> wrote:
> > > -----Original Message-----
> > > From: Kevin Traynor <ktraynor@redhat.com>
> > > 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.
> 
> This patch is not mergeable, it would trigger failures in the ABI checks.
Isn't it a false failure? If yes, is it ignorable? 
> 
> You can see in patchwork that the robot reported a warning in Travis.
> http://mails.dpdk.org/archives/test-report/2020-March/119919.html
> https://travis-ci.com/github/ovsrobot/dpdk/jobs/295652710#L4476
> 
> 
> I opened a bz to libabigail.
> https://sourceware.org/bugzilla/show_bug.cgi?id=25661
> 
> 
> Either a different solution is found, or your patch will have to deal
> with this issue (libabigail fix won't be ready soon afaik) and waive
> this.
Maybe we come back to 'disable the warning', before the libabigail fix ready?  or alternatively ignore this ABI false failure, if it is. 
I do not have ideas of what otherwise the options are. 
> 
> --
> David Marchand
Thomas Monjalon April 9, 2020, 10:49 a.m. UTC | #17 
09/04/2020 11:48, Gavin Hu:
> From: David Marchand <david.marchand@redhat.com>
> > On Wed, Apr 8, 2020 at 5:05 PM Gavin Hu <Gavin.Hu@arm.com> wrote:
> > > From: Kevin Traynor <ktraynor@redhat.com>
> > > > 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.
> > 
> > This patch is not mergeable, it would trigger failures in the ABI checks.
> 
> Isn't it a false failure? If yes, is it ignorable?
> 
> > You can see in patchwork that the robot reported a warning in Travis.
> > http://mails.dpdk.org/archives/test-report/2020-March/119919.html
> > https://travis-ci.com/github/ovsrobot/dpdk/jobs/295652710#L4476
> > 
> > 
> > I opened a bz to libabigail.
> > https://sourceware.org/bugzilla/show_bug.cgi?id=25661
> > 
> > 
> > Either a different solution is found, or your patch will have to deal
> > with this issue (libabigail fix won't be ready soon afaik) and waive
> > this.
> 
> Maybe we come back to 'disable the warning', before the libabigail fix ready?  or alternatively ignore this ABI false failure, if it is. 
> I do not have ideas of what otherwise the options are. 

Gavin,
I did not check this case.
But in general, we do not skip checks, except some checkpatch ones.
The policy with ABI checks is "NEVER SKIP".
We prefer postponing patches, waiting for someone to fix tooling.

There is a lack of motivation currently for general concerns.
We need to avoid being "write-only" contributors.
So two things need to be done:
	1/ improve tooling where it needs
	2/ review patches from others
I published a review list recently:
	http://mails.dpdk.org/archives/announce/2020-April/000315.html
Ray Kinsella April 9, 2020, 4:09 p.m. UTC | #18 
On 09/04/2020 11:49, Thomas Monjalon wrote:
> 09/04/2020 11:48, Gavin Hu:
>> From: David Marchand <david.marchand@redhat.com>
>>> On Wed, Apr 8, 2020 at 5:05 PM Gavin Hu <Gavin.Hu@arm.com> wrote:
>>>> From: Kevin Traynor <ktraynor@redhat.com>
>>>>> 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.
>>>
>>> This patch is not mergeable, it would trigger failures in the ABI checks.
>>
>> Isn't it a false failure? If yes, is it ignorable?
>>
>>> You can see in patchwork that the robot reported a warning in Travis.
>>> http://mails.dpdk.org/archives/test-report/2020-March/119919.html
>>> https://travis-ci.com/github/ovsrobot/dpdk/jobs/295652710#L4476
>>>
>>>
>>> I opened a bz to libabigail.
>>> https://sourceware.org/bugzilla/show_bug.cgi?id=25661
>>>
>>>
>>> Either a different solution is found, or your patch will have to deal
>>> with this issue (libabigail fix won't be ready soon afaik) and waive
>>> this.
>>
>> Maybe we come back to 'disable the warning', before the libabigail fix ready?  or alternatively ignore this ABI false failure, if it is. 
>> I do not have ideas of what otherwise the options are. 
> 
> Gavin,
> I did not check this case.
> But in general, we do not skip checks, except some checkpatch ones.
> The policy with ABI checks is "NEVER SKIP".
> We prefer postponing patches, waiting for someone to fix tooling.

In this case Dave Marchand has more than adequately shown the issue is because of a libabigail failure.
and has raised an appropriate BZ in the right place, much kudos. 

My read of this, is that libabigail fix, will be complicated. 

> There is a lack of motivation currently for general concerns.
> We need to avoid being "write-only" contributors.
> So two things need to be done:
> 	1/ improve tooling where it needs
> 	2/ review patches from others
> I published a review list recently:
> 	http://mails.dpdk.org/archives/announce/2020-April/000315.html
Gavin Hu April 11, 2020, 2:50 a.m. UTC | #19 
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, April 9, 2020 6:50 PM
> To: Gavin Hu <Gavin.Hu@arm.com>
> Cc: David Marchand <david.marchand@redhat.com>; Kevin Traynor
> <ktraynor@redhat.com>; Bruce Richardson <bruce.richardson@intel.com>;
> Morten Brørup <mb@smartsharesystems.com>; Ferruh Yigit
> <ferruh.yigit@intel.com>; dev@dpdk.org; nd <nd@arm.com>;
> jerinj@marvell.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; Phil Yang <Phil.Yang@arm.com>; Joyce Kong
> <Joyce.Kong@arm.com>; stable@dpdk.org; Olivier MATZ
> <olivier.matz@6wind.com>; Konstantin Ananyev
> <konstantin.ananyev@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>; nd <nd@arm.com>; mdr@ashroe.eu
> Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length
> marker with unnamed union
> 
> 09/04/2020 11:48, Gavin Hu:
> > From: David Marchand <david.marchand@redhat.com>
> > > On Wed, Apr 8, 2020 at 5:05 PM Gavin Hu <Gavin.Hu@arm.com> wrote:
> > > > From: Kevin Traynor <ktraynor@redhat.com>
> > > > > 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.
> > >
> > > This patch is not mergeable, it would trigger failures in the ABI checks.
> >
> > Isn't it a false failure? If yes, is it ignorable?
> >
> > > You can see in patchwork that the robot reported a warning in Travis.
> > > http://mails.dpdk.org/archives/test-report/2020-March/119919.html
> > > https://travis-ci.com/github/ovsrobot/dpdk/jobs/295652710#L4476
> > >
> > >
> > > I opened a bz to libabigail.
> > > https://sourceware.org/bugzilla/show_bug.cgi?id=25661
> > >
> > >
> > > Either a different solution is found, or your patch will have to deal
> > > with this issue (libabigail fix won't be ready soon afaik) and waive
> > > this.
> >
> > Maybe we come back to 'disable the warning', before the libabigail fix
> ready?  or alternatively ignore this ABI false failure, if it is.
> > I do not have ideas of what otherwise the options are.
> 
> Gavin,
> I did not check this case.
> But in general, we do not skip checks, except some checkpatch ones.
> The policy with ABI checks is "NEVER SKIP".
> We prefer postponing patches, waiting for someone to fix tooling.
Ok, I am fine with this. 
> There is a lack of motivation currently for general concerns.
> We need to avoid being "write-only" contributors.
> So two things need to be done:
> 	1/ improve tooling where it needs
> 	2/ review patches from others
> I published a review list recently:
> 	http://mails.dpdk.org/archives/announce/2020-April/000315.html
Thanks!
>
Kevin Traynor May 14, 2020, 1:24 p.m. UTC | #20 
On 08/04/2020 16:22, David Marchand wrote:
> On Wed, Apr 8, 2020 at 5:05 PM Gavin Hu <Gavin.Hu@arm.com> wrote:
>>> -----Original Message-----
>>> From: Kevin Traynor <ktraynor@redhat.com>
>>> 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.
> 
> This patch is not mergeable, it would trigger failures in the ABI checks.
> 

Sent
http://inbox.dpdk.org/dev/20200514131857.11966-1-ktraynor@redhat.com as
this patch was not reworked/merged.

Kevin.

> You can see in patchwork that the robot reported a warning in Travis.
> http://mails.dpdk.org/archives/test-report/2020-March/119919.html
> https://travis-ci.com/github/ovsrobot/dpdk/jobs/295652710#L4476
> 
> 
> I opened a bz to libabigail.
> https://sourceware.org/bugzilla/show_bug.cgi?id=25661
> 
> 
> Either a different solution is found, or your patch will have to deal
> with this issue (libabigail fix won't be ready soon afaik) and waive
> this.
> 
>
diff mbox series

Patch

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];
+		RTE_STD_C11
+		struct {
+			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 {
+					/**< Atomically accessed refcnt */
+					rte_atomic16_t refcnt_atomic;
+					/** Non-atomically accessed refcnt */
+					uint16_t refcnt;
+				};
+			uint16_t nb_segs;         /**< Number of segments. */
 
-	/** Input port (16 bits to support more than 256 virtual ports).
-	 * The event eth Tx adapter uses this field to specify the output port.
-	 */
-	uint16_t port;
+			/** Input port (16 bits to support more than 256
+			 * virtual ports).  The event eth Tx adapter uses this
+			 * field to specify the output port.
+			 */
+			uint16_t port;
+		};
+	};
 
 	uint64_t ol_flags;        /**< Offload features. */