patches for DPDK stable branches
 help / color / Atom feed
* [dpdk-stable] [PATCH v1] mbuf: replace zero-length marker with unnamed union
@ 2020-03-03 16:27 Gavin Hu
  2020-03-04 12:32 ` [dpdk-stable] [dpdk-dev] " Kevin Traynor
  2020-03-07 15:56 ` [dpdk-stable] [PATCH v2] " Gavin Hu
  0 siblings, 2 replies; 16+ messages in thread
From: Gavin Hu @ 2020-03-03 16:27 UTC (permalink / raw)
  To: dev
  Cc: nd, thomas, david.marchand, jerinj, Honnappa.Nagarahalli,
	ruifeng.wang, phil.yang, stable

gcc 10.0.1 reports: error: array subscript 0 is outside the bounds of an
interior zero-length array 'RTE_MARKER64' {aka 'long unsigned int[0]'}
[-Werror=zero-length-bounds] 310 |  *(uint64_t *)(&mbuf->rearm_data) =
val;

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>
---
 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..5390ddcfa 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;
+		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. */
 
-- 
2.25.0


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v1] mbuf: replace zero-length marker with unnamed union
  2020-03-03 16:27 [dpdk-stable] [PATCH v1] mbuf: replace zero-length marker with unnamed union Gavin Hu
@ 2020-03-04 12:32 ` " Kevin Traynor
  2020-03-07 14:52   ` Gavin Hu
  2020-03-07 15:56 ` [dpdk-stable] [PATCH v2] " Gavin Hu
  1 sibling, 1 reply; 16+ messages in thread
From: Kevin Traynor @ 2020-03-04 12:32 UTC (permalink / raw)
  To: Gavin Hu, dev
  Cc: nd, thomas, david.marchand, jerinj, Honnappa.Nagarahalli,
	ruifeng.wang, phil.yang, stable

On 03/03/2020 16:27, Gavin Hu wrote:
> gcc 10.0.1 reports: error: array subscript 0 is outside the bounds of an
> interior zero-length array 'RTE_MARKER64' {aka 'long unsigned int[0]'}
> [-Werror=zero-length-bounds] 310 |  *(uint64_t *)(&mbuf->rearm_data) =
> val;
> 
> 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>
> ---
>  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..5390ddcfa 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;
> +		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. */
>  
> 


Hi Gavin, this causes some errors on x86:

# gcc --version | head -1
gcc (GCC) 10.0.1 20200216 (Red Hat 10.0.1-0.8)


In file included from
../lib/librte_eal/common/include/arch/x86/rte_byteorder.h:13,
                 from ../drivers/net/sfc/sfc_ef10_rx.c:14:
../drivers/net/sfc/sfc_ef10_rx.c: In function ‘sfc_ef10_rx_process_event’:
../drivers/net/sfc/sfc_ef10_rx.c:309:39: error: subscripted value is
neither array nor pointer nor vector
  309 |  RTE_BUILD_BUG_ON(sizeof(m->rearm_data[0]) !=
sizeof(rxq->rearm_data));
      |                                       ^
../lib/librte_eal/common/include/rte_common.h:292:65: note: in
definition of macro ‘RTE_BUILD_BUG_ON’
  292 | #define RTE_BUILD_BUG_ON(condition) ((void)sizeof(char[1 -
2*!!(condition)]))
      |
^~~~~~~~~
../drivers/net/sfc/sfc_ef10_rx.c:310:15: error: subscripted value is
neither array nor pointer nor vector
  310 |  m->rearm_data[0] = rxq->rearm_data;
      |


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v1] mbuf: replace zero-length marker with unnamed union
  2020-03-04 12:32 ` [dpdk-stable] [dpdk-dev] " Kevin Traynor
@ 2020-03-07 14:52   ` Gavin Hu
  0 siblings, 0 replies; 16+ messages in thread
From: Gavin Hu @ 2020-03-07 14:52 UTC (permalink / raw)
  To: Kevin Traynor, dev
  Cc: nd, thomas, david.marchand, jerinj, Honnappa Nagarahalli,
	Ruifeng Wang, Phil Yang, stable, nd

Hi Kevin,

> -----Original Message-----
> From: Kevin Traynor <ktraynor@redhat.com>
> Sent: Wednesday, March 4, 2020 8:33 PM
> To: Gavin Hu <Gavin.Hu@arm.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>; thomas@monjalon.net;
> david.marchand@redhat.com; jerinj@marvell.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; Phil Yang <Phil.Yang@arm.com>;
> stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v1] mbuf: replace zero-length marker with
> unnamed union
> 
> On 03/03/2020 16:27, Gavin Hu wrote:
> > gcc 10.0.1 reports: error: array subscript 0 is outside the bounds of an
> > interior zero-length array 'RTE_MARKER64' {aka 'long unsigned int[0]'}
> > [-Werror=zero-length-bounds] 310 |  *(uint64_t *)(&mbuf->rearm_data) =
> > val;
> >
> > 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>
> > ---
> >  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..5390ddcfa 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;

To address this historical issue, how about changing this line to 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. */
> >
> >
> 
> 
> Hi Gavin, this causes some errors on x86:
> 
> # gcc --version | head -1
> gcc (GCC) 10.0.1 20200216 (Red Hat 10.0.1-0.8)
> 
> 
> In file included from
> ../lib/librte_eal/common/include/arch/x86/rte_byteorder.h:13,
>                  from ../drivers/net/sfc/sfc_ef10_rx.c:14:
> ../drivers/net/sfc/sfc_ef10_rx.c: In function 'sfc_ef10_rx_process_event':
> ../drivers/net/sfc/sfc_ef10_rx.c:309:39: error: subscripted value is
> neither array nor pointer nor vector
>   309 |  RTE_BUILD_BUG_ON(sizeof(m->rearm_data[0]) !=
> sizeof(rxq->rearm_data));
>       |                                       ^
> ../lib/librte_eal/common/include/rte_common.h:292:65: note: in
> definition of macro 'RTE_BUILD_BUG_ON'
>   292 | #define RTE_BUILD_BUG_ON(condition) ((void)sizeof(char[1 -
> 2*!!(condition)]))
>       |
> ^~~~~~~~~
> ../drivers/net/sfc/sfc_ef10_rx.c:310:15: error: subscripted value is
> neither array nor pointer nor vector
>   310 |  m->rearm_data[0] = rxq->rearm_data;
>       |


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [dpdk-stable] [PATCH v2] mbuf: replace zero-length marker with unnamed union
  2020-03-03 16:27 [dpdk-stable] [PATCH v1] mbuf: replace zero-length marker with unnamed union Gavin Hu
  2020-03-04 12:32 ` [dpdk-stable] [dpdk-dev] " Kevin Traynor
@ 2020-03-07 15:56 ` " Gavin Hu
  2020-03-09  8:55   ` [dpdk-stable] [dpdk-dev] " Ferruh Yigit
  1 sibling, 1 reply; 16+ messages in thread
From: Gavin Hu @ 2020-03-07 15:56 UTC (permalink / raw)
  To: dev
  Cc: nd, david.marchand, thomas, ktraynor, jerinj,
	honnappa.nagarahalli, ruifeng.wang, phil.yang, joyce.kong,
	stable

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];
+		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. */
 
-- 
2.25.0


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with unnamed union
  2020-03-07 15:56 ` [dpdk-stable] [PATCH v2] " Gavin Hu
@ 2020-03-09  8:55   ` " Ferruh Yigit
  2020-03-09  9:45     ` Gavin Hu
  2020-03-09 15:47     ` Stephen Hemminger
  0 siblings, 2 replies; 16+ messages in thread
From: Ferruh Yigit @ 2020-03-09  8:55 UTC (permalink / raw)
  To: Gavin Hu, dev
  Cc: nd, david.marchand, thomas, ktraynor, jerinj,
	honnappa.nagarahalli, ruifeng.wang, phil.yang, joyce.kong,
	stable, Olivier MATZ, Konstantin Ananyev, Andrew Rybchenko

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?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with unnamed union
  2020-03-09  8:55   ` [dpdk-stable] [dpdk-dev] " Ferruh Yigit
@ 2020-03-09  9:45     ` Gavin Hu
  2020-03-09 11:29       ` Ferruh Yigit
  2020-03-09 15:47     ` Stephen Hemminger
  1 sibling, 1 reply; 16+ messages in thread
From: Gavin Hu @ 2020-03-09  9:45 UTC (permalink / raw)
  To: Ferruh Yigit, dev
  Cc: nd, david.marchand, thomas, ktraynor, jerinj,
	Honnappa Nagarahalli, Ruifeng Wang, Phil Yang, Joyce Kong,
	stable, Olivier MATZ, Konstantin Ananyev, Andrew Rybchenko, nd

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with unnamed union
  2020-03-09  9:45     ` Gavin Hu
@ 2020-03-09 11:29       ` Ferruh Yigit
  2020-03-09 13:30         ` Morten Brørup
  0 siblings, 1 reply; 16+ messages in thread
From: Ferruh Yigit @ 2020-03-09 11:29 UTC (permalink / raw)
  To: Gavin Hu, dev
  Cc: nd, david.marchand, thomas, ktraynor, jerinj,
	Honnappa Nagarahalli, Ruifeng Wang, Phil Yang, Joyce Kong,
	stable, Olivier MATZ, Konstantin Ananyev, Andrew Rybchenko

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?




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with unnamed union
  2020-03-09 11:29       ` Ferruh Yigit
@ 2020-03-09 13:30         ` Morten Brørup
  2020-03-09 14:16           ` Richardson, Bruce
  2020-03-11  7:50           ` [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with " Gavin Hu
  0 siblings, 2 replies; 16+ messages in thread
From: Morten Brørup @ 2020-03-09 13:30 UTC (permalink / raw)
  To: Ferruh Yigit, Gavin Hu, dev
  Cc: nd, david.marchand, thomas, ktraynor, jerinj,
	Honnappa Nagarahalli, Ruifeng Wang, Phil Yang, Joyce Kong,
	stable, Olivier MATZ, Konstantin Ananyev, Andrew Rybchenko

> 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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with unnamed union
  2020-03-09 13:30         ` Morten Brørup
@ 2020-03-09 14:16           ` Richardson, Bruce
  2020-03-09 14:50             ` [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length markerwith " Morten Brørup
  2020-03-11  7:50           ` [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with " Gavin Hu
  1 sibling, 1 reply; 16+ messages in thread
From: Richardson, Bruce @ 2020-03-09 14:16 UTC (permalink / raw)
  To: Morten Brørup, Yigit, Ferruh, Gavin Hu, dev
  Cc: nd, david.marchand, thomas, ktraynor, jerinj,
	Honnappa Nagarahalli, Ruifeng Wang, Phil Yang, Joyce Kong,
	stable, Olivier MATZ, Ananyev, Konstantin, Andrew Rybchenko



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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length markerwith unnamed union
  2020-03-09 14:16           ` Richardson, Bruce
@ 2020-03-09 14:50             ` " Morten Brørup
  0 siblings, 0 replies; 16+ messages in thread
From: Morten Brørup @ 2020-03-09 14:50 UTC (permalink / raw)
  To: Richardson, Bruce, Yigit, Ferruh, Gavin Hu, dev
  Cc: nd, david.marchand, thomas, ktraynor, jerinj,
	Honnappa Nagarahalli, Ruifeng Wang, Phil Yang, Joyce Kong,
	stable, Olivier MATZ, Ananyev, Konstantin, Andrew Rybchenko

> 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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with unnamed union
  2020-03-09  8:55   ` [dpdk-stable] [dpdk-dev] " Ferruh Yigit
  2020-03-09  9:45     ` Gavin Hu
@ 2020-03-09 15:47     ` Stephen Hemminger
  1 sibling, 0 replies; 16+ messages in thread
From: Stephen Hemminger @ 2020-03-09 15:47 UTC (permalink / raw)
  To: Ferruh Yigit
  Cc: Gavin Hu, dev, nd, david.marchand, thomas, ktraynor, jerinj,
	honnappa.nagarahalli, ruifeng.wang, phil.yang, joyce.kong,
	stable, Olivier MATZ, Konstantin Ananyev, Andrew Rybchenko

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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with unnamed union
  2020-03-09 13:30         ` Morten Brørup
  2020-03-09 14:16           ` Richardson, Bruce
@ 2020-03-11  7:50           ` " Gavin Hu
  2020-03-11  9:04             ` Morten Brørup
  1 sibling, 1 reply; 16+ messages in thread
From: Gavin Hu @ 2020-03-11  7:50 UTC (permalink / raw)
  To: Morten Brørup, Ferruh Yigit, dev
  Cc: nd, david.marchand, thomas, ktraynor, jerinj,
	Honnappa Nagarahalli, Ruifeng Wang, Phil Yang, Joyce Kong,
	stable, Olivier MATZ, Konstantin Ananyev, Andrew Rybchenko, nd

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with unnamed union
  2020-03-11  7:50           ` [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with " Gavin Hu
@ 2020-03-11  9:04             ` Morten Brørup
  2020-03-11 12:07               ` Bruce Richardson
  0 siblings, 1 reply; 16+ messages in thread
From: Morten Brørup @ 2020-03-11  9:04 UTC (permalink / raw)
  To: Gavin Hu, Ferruh Yigit, dev
  Cc: nd, david.marchand, thomas, ktraynor, jerinj,
	Honnappa Nagarahalli, Ruifeng Wang, Phil Yang, Joyce Kong,
	stable, Olivier MATZ, Konstantin Ananyev, Andrew Rybchenko, nd

> 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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with unnamed union
  2020-03-11  9:04             ` Morten Brørup
@ 2020-03-11 12:07               ` Bruce Richardson
  2020-03-13  7:36                 ` Gavin Hu
  2020-03-13  9:22                 ` Gavin Hu
  0 siblings, 2 replies; 16+ messages in thread
From: Bruce Richardson @ 2020-03-11 12:07 UTC (permalink / raw)
  To: Morten Brørup
  Cc: Gavin Hu, Ferruh Yigit, dev, nd, david.marchand, thomas,
	ktraynor, jerinj, Honnappa Nagarahalli, Ruifeng Wang, Phil Yang,
	Joyce Kong, stable, Olivier MATZ, Konstantin Ananyev,
	Andrew Rybchenko

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?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with unnamed union
  2020-03-11 12:07               ` Bruce Richardson
@ 2020-03-13  7:36                 ` Gavin Hu
  2020-03-13  9:22                 ` Gavin Hu
  1 sibling, 0 replies; 16+ messages in thread
From: Gavin Hu @ 2020-03-13  7:36 UTC (permalink / raw)
  To: Bruce Richardson, Morten Brørup
  Cc: Ferruh Yigit, dev, nd, david.marchand, thomas, ktraynor, jerinj,
	Honnappa Nagarahalli, Ruifeng Wang, Phil Yang, Joyce Kong,
	stable, Olivier MATZ, Konstantin Ananyev, Andrew Rybchenko, nd

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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with unnamed union
  2020-03-11 12:07               ` Bruce Richardson
  2020-03-13  7:36                 ` Gavin Hu
@ 2020-03-13  9:22                 ` Gavin Hu
  1 sibling, 0 replies; 16+ messages in thread
From: Gavin Hu @ 2020-03-13  9:22 UTC (permalink / raw)
  To: Bruce Richardson, Morten Brørup
  Cc: Ferruh Yigit, dev, nd, david.marchand, thomas, ktraynor, jerinj,
	Honnappa Nagarahalli, Ruifeng Wang, Phil Yang, Joyce Kong,
	stable, Olivier MATZ, Konstantin Ananyev, Andrew Rybchenko, nd

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. 

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, back to index

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-03 16:27 [dpdk-stable] [PATCH v1] mbuf: replace zero-length marker with unnamed union Gavin Hu
2020-03-04 12:32 ` [dpdk-stable] [dpdk-dev] " Kevin Traynor
2020-03-07 14:52   ` Gavin Hu
2020-03-07 15:56 ` [dpdk-stable] [PATCH v2] " Gavin Hu
2020-03-09  8:55   ` [dpdk-stable] [dpdk-dev] " Ferruh Yigit
2020-03-09  9:45     ` Gavin Hu
2020-03-09 11:29       ` Ferruh Yigit
2020-03-09 13:30         ` Morten Brørup
2020-03-09 14:16           ` Richardson, Bruce
2020-03-09 14:50             ` [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length markerwith " Morten Brørup
2020-03-11  7:50           ` [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: replace zero-length marker with " Gavin Hu
2020-03-11  9:04             ` Morten Brørup
2020-03-11 12:07               ` Bruce Richardson
2020-03-13  7:36                 ` Gavin Hu
2020-03-13  9:22                 ` Gavin Hu
2020-03-09 15:47     ` Stephen Hemminger

patches for DPDK stable branches

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ http://inbox.dpdk.org/stable \
		stable@dpdk.org
	public-inbox-index stable


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.stable


AGPL code for this site: git clone https://public-inbox.org/ public-inbox