[PATCH] net: add bit fields to IPv6 header definition

[PATCH] net: add bit fields to IPv6 header definition

[Gregory Etelson]

DPDK IPv6 header definition combined the `version`, `traffic class`
and `flow label` header fields into a single 32 bits structure member
`vtc_flow`.

The patch expands IPv6 header definition with dedicated structure
members for the `version`, `traffic class` and `flow label` fields.
The patch also preserves existing `vtc_flow` structure member for
backward compatibility.

Signed-off-by: Gregory Etelson <getelson@nvidia.com>
---
 lib/net/rte_ip.h | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
index 0d103d4127..26e78a6624 100644
--- a/lib/net/rte_ip.h
+++ b/lib/net/rte_ip.h
@@ -524,7 +524,21 @@ rte_ipv4_udptcp_cksum_mbuf_verify(const struct rte_mbuf *m,
  * IPv6 Header
  */
 struct rte_ipv6_hdr {
-	rte_be32_t vtc_flow;	/**< IP version, traffic class & flow label. */
+	__extension__
+	union {
+		rte_be32_t vtc_flow;        /**< IP version, traffic class & flow label. */
+		struct {
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+			uint32_t flow_label:24; /**< flow label */
+			uint32_t tc:4;     /**< traffic class */
+			uint32_t version:4; /**< version */
+#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+			uint8_t version:4; /**< version */
+			uint8_t tc:4;     /**< traffic class */
+			uint32_t flow_label:24; /**< flow label */
+#endif
+		};
+	};
 	rte_be16_t payload_len;	/**< IP payload size, including ext. headers */
 	uint8_t  proto;		/**< Protocol, next header. */
 	uint8_t  hop_limits;	/**< Hop limits. */
-- 
2.43.0

RE: [PATCH] net: add bit fields to IPv6 header definition

[Morten Brørup]

> From: Gregory Etelson [mailto:getelson@nvidia.com]
> Sent: Tuesday, 18 June 2024 07.18
> 
> DPDK IPv6 header definition combined the `version`, `traffic class`
> and `flow label` header fields into a single 32 bits structure member
> `vtc_flow`.
> 
> The patch expands IPv6 header definition with dedicated structure
> members for the `version`, `traffic class` and `flow label` fields.
> The patch also preserves existing `vtc_flow` structure member for
> backward compatibility.

Good addition.
I had been wondering why we didn't have this already. :-)

> 
> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> ---
>  lib/net/rte_ip.h | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> index 0d103d4127..26e78a6624 100644
> --- a/lib/net/rte_ip.h
> +++ b/lib/net/rte_ip.h
> @@ -524,7 +524,21 @@ rte_ipv4_udptcp_cksum_mbuf_verify(const struct rte_mbuf
> *m,
>   * IPv6 Header
>   */
>  struct rte_ipv6_hdr {
> -	rte_be32_t vtc_flow;	/**< IP version, traffic class & flow label.
> */
> +	__extension__
> +	union {
> +		rte_be32_t vtc_flow;        /**< IP version, traffic class & flow
> label. */

The mbuf structure [1] has __extension__ here,
i.e. preceding the structure following the integer field,
instead of preceding the union.

[1]: https://git.dpdk.org/dpdk/tree/lib/mbuf/rte_mbuf_core.h#n520

> +		struct {
> +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> +			uint32_t flow_label:24; /**< flow label */

Flow Label is 20 bits, not 24.

> +			uint32_t tc:4;     /**< traffic class */

TC is 8 bits, not 4.

> +			uint32_t version:4; /**< version */
> +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> +			uint8_t version:4; /**< version */
> +			uint8_t tc:4;     /**< traffic class */

:8, not :4.

> +			uint32_t flow_label:24; /**< flow label */

:20, not :24.

> +#endif
> +		};
> +	};
>  	rte_be16_t payload_len;	/**< IP payload size, including ext. headers
> */
>  	uint8_t  proto;		/**< Protocol, next header. */
>  	uint8_t  hop_limits;	/**< Hop limits. */
> --
> 2.43.0

RE: [PATCH] net: add bit fields to IPv6 header definition

[Morten Brørup]

> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> 
> > From: Gregory Etelson [mailto:getelson@nvidia.com]
> > Sent: Tuesday, 18 June 2024 07.18
> >
> > DPDK IPv6 header definition combined the `version`, `traffic class`
> > and `flow label` header fields into a single 32 bits structure member
> > `vtc_flow`.
> >
> > The patch expands IPv6 header definition with dedicated structure
> > members for the `version`, `traffic class` and `flow label` fields.
> > The patch also preserves existing `vtc_flow` structure member for
> > backward compatibility.
> 
> Good addition.
> I had been wondering why we didn't have this already. :-)
> 
> >
> > Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> > ---
> >  lib/net/rte_ip.h | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> > index 0d103d4127..26e78a6624 100644
> > --- a/lib/net/rte_ip.h
> > +++ b/lib/net/rte_ip.h
> > @@ -524,7 +524,21 @@ rte_ipv4_udptcp_cksum_mbuf_verify(const struct rte_mbuf
> > *m,
> >   * IPv6 Header
> >   */
> >  struct rte_ipv6_hdr {
> > -	rte_be32_t vtc_flow;	/**< IP version, traffic class & flow label.
> > */
> > +	__extension__
> > +	union {
> > +		rte_be32_t vtc_flow;        /**< IP version, traffic class & flow
> > label. */
> 
> The mbuf structure [1] has __extension__ here,
> i.e. preceding the structure following the integer field,
> instead of preceding the union.
> 
> [1]: https://git.dpdk.org/dpdk/tree/lib/mbuf/rte_mbuf_core.h#n520
> 
> > +		struct {
> > +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> > +			uint32_t flow_label:24; /**< flow label */
> 
> Flow Label is 20 bits, not 24.

I forgot to mention that the flow_label field should be rte_be32_t instead of uint32_t.

> 
> > +			uint32_t tc:4;     /**< traffic class */
> 
> TC is 8 bits, not 4.

While you are at it, please also expand the "tc" field to another union with dedicated fields for "dscp:6" and "ecn:2", keeping the combined "tc" field too. The same goes for the IPv4 "type_of_service" field. This expansion could go into a separate patch, possibly in the same series.

> 
> > +			uint32_t version:4; /**< version */
> > +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> > +			uint8_t version:4; /**< version */
> > +			uint8_t tc:4;     /**< traffic class */
> 
> :8, not :4.
> 
> > +			uint32_t flow_label:24; /**< flow label */
> 
> :20, not :24.
> 
> > +#endif
> > +		};
> > +	};
> >  	rte_be16_t payload_len;	/**< IP payload size, including ext. headers
> > */
> >  	uint8_t  proto;		/**< Protocol, next header. */
> >  	uint8_t  hop_limits;	/**< Hop limits. */
> > --
> > 2.43.0

[PATCH v2] net: add bit fields to IPv6 header definition

[Gregory Etelson]

DPDK IPv6 header definition combined the `version`, `traffic class`
and `flow label` header fields into a single 32 bits structure member
`vtc_flow`.

The patch expands IPv6 header definition with dedicated structure
members for the `version`, `traffic class` and `flow label` fields.
The `traffic class` is also separated into DS and ECN fields.

The patch also preserves existing `vtc_flow` structure member for
backward compatibility.

Signed-off-by: Gregory Etelson <getelson@nvidia.com>
---
v2: 
Replace uint32_t with rte_be32_t.
Split traffic_class into DS and ECN bits.
---
 lib/net/rte_ip.h | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
index 0d103d4127..fe1d596054 100644
--- a/lib/net/rte_ip.h
+++ b/lib/net/rte_ip.h
@@ -524,7 +524,23 @@ rte_ipv4_udptcp_cksum_mbuf_verify(const struct rte_mbuf *m,
  * IPv6 Header
  */
 struct rte_ipv6_hdr {
-	rte_be32_t vtc_flow;	/**< IP version, traffic class & flow label. */
+	union {
+		rte_be32_t vtc_flow;        /**< IP version, traffic class & flow label. */
+		__extension__
+		struct {
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+			rte_be32_t flow_label:20; /**< flow label */
+			rte_be32_t ecn:2; /**< ECN */
+			rte_be32_t ds:6;     /**< differentiated services */
+			rte_be32_t version:4; /**< version */
+#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+			rte_be32_t version:4; /**< version */
+			rte_be32_t ds:6;     /**< differentiated services */
+			rte_be32_t ecn:2; /**< ECN */
+			rte_be32_t flow_label:20; /**< flow label */
+#endif
+		};
+	};
 	rte_be16_t payload_len;	/**< IP payload size, including ext. headers */
 	uint8_t  proto;		/**< Protocol, next header. */
 	uint8_t  hop_limits;	/**< Hop limits. */
--
2.43.0

RE: [PATCH] net: add bit fields to IPv6 header definition

[Etelson, Gregory]

Hello,

I've posted v2 patch with fixes.

Regards,
Gregory

RE: [PATCH v2] net: add bit fields to IPv6 header definition

[Morten Brørup]

> From: Gregory Etelson [mailto:getelson@nvidia.com]
> Sent: Tuesday, 18 June 2024 09.58
> 
> DPDK IPv6 header definition combined the `version`, `traffic class`
> and `flow label` header fields into a single 32 bits structure member
> `vtc_flow`.
> 
> The patch expands IPv6 header definition with dedicated structure
> members for the `version`, `traffic class` and `flow label` fields.
> The `traffic class` is also separated into DS and ECN fields.
> 
> The patch also preserves existing `vtc_flow` structure member for
> backward compatibility.
> 
> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> ---
> v2:
> Replace uint32_t with rte_be32_t.
> Split traffic_class into DS and ECN bits.
> ---
>  lib/net/rte_ip.h | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> index 0d103d4127..fe1d596054 100644
> --- a/lib/net/rte_ip.h
> +++ b/lib/net/rte_ip.h
> @@ -524,7 +524,23 @@ rte_ipv4_udptcp_cksum_mbuf_verify(const struct rte_mbuf
> *m,
>   * IPv6 Header
>   */
>  struct rte_ipv6_hdr {
> -	rte_be32_t vtc_flow;	/**< IP version, traffic class & flow label.
> */
> +	union {
> +		rte_be32_t vtc_flow;        /**< IP version, traffic class & flow
> label. */
> +		__extension__
> +		struct {
> +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> +			rte_be32_t flow_label:20; /**< flow label */

I don't know if sub-unions of bitfields are possible and/or require special magic for packing the bits.
If possible, expanding the 8 bit "tc" field like this would be nice:

+ union {
+ 	rte_be32_t tc:8;
+ 	__extension__
+ 	struct {

> +			rte_be32_t ecn:2; /**< ECN */
> +			rte_be32_t ds:6;     /**< differentiated services */

+ 	};
+ };

> +			rte_be32_t version:4; /**< version */
> +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> +			rte_be32_t version:4; /**< version */
> +			rte_be32_t ds:6;     /**< differentiated services */
> +			rte_be32_t ecn:2; /**< ECN */
> +			rte_be32_t flow_label:20; /**< flow label */
> +#endif
> +		};
> +	};
>  	rte_be16_t payload_len;	/**< IP payload size, including ext. headers
> */
>  	uint8_t  proto;		/**< Protocol, next header. */
>  	uint8_t  hop_limits;	/**< Hop limits. */
> --
> 2.43.0

Without the suggested addition,
Reviewed-by: Morten Brørup <mb@smartsharesystems.com>

With the suggested addition,
Acked-by: Morten Brørup <mb@smartsharesystems.com>

[PATCH v3] net: add bit fields to IPv6 header definition

[Gregory Etelson]

DPDK IPv6 header definition combined the `version`, `traffic class`
and `flow label` header fields into a single 32 bits structure member
`vtc_flow`.

The patch expands IPv6 header definition with dedicated structure
members for the `version`, `traffic class` and `flow label` fields.
The `traffic class` is also separated into DS and ECN fields.

The patch also preserves existing `vtc_flow` structure member for
backward compatibility.

Signed-off-by: Gregory Etelson <getelson@nvidia.com>
---
v2: 
Replace uint32_t with rte_be32_t.
Split traffic_class into DS and ECN bits.
v3:
Define both traffic_class and DS + ECN fields.
---
 lib/net/rte_ip.h | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
index 0d103d4127..3adfcbc431 100644
--- a/lib/net/rte_ip.h
+++ b/lib/net/rte_ip.h
@@ -524,7 +524,33 @@ rte_ipv4_udptcp_cksum_mbuf_verify(const struct rte_mbuf *m,
  * IPv6 Header
  */
 struct rte_ipv6_hdr {
-	rte_be32_t vtc_flow;	/**< IP version, traffic class & flow label. */
+	union {
+		rte_be32_t vtc_flow;        /**< IP version, traffic class & flow label. */
+		__extension__
+		struct {
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+			rte_be32_t flow_label:20; /**< flow label */
+			union {
+				rte_be32_t tc:8;
+				struct {
+					rte_be32_t ecn:2; /**< ECN */
+					rte_be32_t ds:6;  /**< differentiated services */
+				};
+			};
+			rte_be32_t version:4; /**< version */
+#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+			rte_be32_t version:4; /**< version */
+			union {
+				rte_be32_t tc:8;
+				struct {
+					rte_be32_t ds:6;  /**< differentiated services */
+					rte_be32_t ecn:2; /**< ECN */
+				};
+			};
+			rte_be32_t flow_label:20; /**< flow label */
+#endif
+		};
+	};
 	rte_be16_t payload_len;	/**< IP payload size, including ext. headers */
 	uint8_t  proto;		/**< Protocol, next header. */
 	uint8_t  hop_limits;	/**< Hop limits. */
-- 
2.43.0

RE: [PATCH v3] net: add bit fields to IPv6 header definition

[Morten Brørup]

>  struct rte_ipv6_hdr {
> -	rte_be32_t vtc_flow;	/**< IP version, traffic class & flow label.
> */
> +	union {
> +		rte_be32_t vtc_flow;        /**< IP version, traffic class & flow
> label. */
> +		__extension__
> +		struct {
> +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> +			rte_be32_t flow_label:20; /**< flow label */

I played around with this in Godbolt, and __attribute__((__packed__)) only packs bytes, not bits, so the packed structure grows from 4 to 5 bytes, because the compiler adds 4 bits of padding here, to byte align the union holding "tc".

Please revert to v2.

Sorry about the noise. ;-)

> +			union {
> +				rte_be32_t tc:8;
> +				struct {
> +					rte_be32_t ecn:2; /**< ECN */
> +					rte_be32_t ds:6;  /**< differentiated
> services */
> +				};
> +			};

[PATCH v4] net: add bit fields to IPv6 header definition

[Gregory Etelson]

DPDK IPv6 header definition combined the `version`, `traffic class`
and `flow label` header fields into a single 32 bits structure member
`vtc_flow`.

The patch expands IPv6 header definition with dedicated structure
members for the `version`, `traffic class` and `flow label` fields.
The `traffic class` is also separated into DS and ECN fields.

The patch also preserves existing `vtc_flow` structure member for
backward compatibility.

Signed-off-by: Gregory Etelson <getelson@nvidia.com>
---
v2: 
Replace uint32_t with rte_be32_t.
Split traffic_class into DS and ECN bits.
v3:
Define both traffic_class and DS + ECN fields.
v4: 
The `packed` attribute works on bytes only.
Back to v2.
---
 lib/net/rte_ip.h | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
index 0d103d4127..fe1d596054 100644
--- a/lib/net/rte_ip.h
+++ b/lib/net/rte_ip.h
@@ -524,7 +524,23 @@ rte_ipv4_udptcp_cksum_mbuf_verify(const struct rte_mbuf *m,
  * IPv6 Header
  */
 struct rte_ipv6_hdr {
-	rte_be32_t vtc_flow;	/**< IP version, traffic class & flow label. */
+	union {
+		rte_be32_t vtc_flow;        /**< IP version, traffic class & flow label. */
+		__extension__
+		struct {
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+			rte_be32_t flow_label:20; /**< flow label */
+			rte_be32_t ecn:2; /**< ECN */
+			rte_be32_t ds:6;     /**< differentiated services */
+			rte_be32_t version:4; /**< version */
+#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+			rte_be32_t version:4; /**< version */
+			rte_be32_t ds:6;     /**< differentiated services */
+			rte_be32_t ecn:2; /**< ECN */
+			rte_be32_t flow_label:20; /**< flow label */
+#endif
+		};
+	};
 	rte_be16_t payload_len;	/**< IP payload size, including ext. headers */
 	uint8_t  proto;		/**< Protocol, next header. */
 	uint8_t  hop_limits;	/**< Hop limits. */
-- 
2.43.0

RE: [PATCH v4] net: add bit fields to IPv6 header definition

[Morten Brørup]

> From: Gregory Etelson [mailto:getelson@nvidia.com]
> 
> DPDK IPv6 header definition combined the `version`, `traffic class`
> and `flow label` header fields into a single 32 bits structure member
> `vtc_flow`.
> 
> The patch expands IPv6 header definition with dedicated structure
> members for the `version`, `traffic class` and `flow label` fields.
> The `traffic class` is also separated into DS and ECN fields.
> 
> The patch also preserves existing `vtc_flow` structure member for
> backward compatibility.
> 
> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> ---

Reviewed-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH] net: add bit fields to IPv6 header definition

[Tyler Retzlaff]

On Tue, Jun 18, 2024 at 08:42:53AM +0200, Morten Brørup wrote:
> > From: Gregory Etelson [mailto:getelson@nvidia.com]
> > Sent: Tuesday, 18 June 2024 07.18
> > 
> > DPDK IPv6 header definition combined the `version`, `traffic class`
> > and `flow label` header fields into a single 32 bits structure member
> > `vtc_flow`.
> > 
> > The patch expands IPv6 header definition with dedicated structure
> > members for the `version`, `traffic class` and `flow label` fields.
> > The patch also preserves existing `vtc_flow` structure member for
> > backward compatibility.
> 
> Good addition.
> I had been wondering why we didn't have this already. :-)
> 
> > 
> > Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> > ---
> >  lib/net/rte_ip.h | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> > index 0d103d4127..26e78a6624 100644
> > --- a/lib/net/rte_ip.h
> > +++ b/lib/net/rte_ip.h
> > @@ -524,7 +524,21 @@ rte_ipv4_udptcp_cksum_mbuf_verify(const struct rte_mbuf
> > *m,
> >   * IPv6 Header
> >   */
> >  struct rte_ipv6_hdr {
> > -	rte_be32_t vtc_flow;	/**< IP version, traffic class & flow label.
> > */
> > +	__extension__
> > +	union {
> > +		rte_be32_t vtc_flow;        /**< IP version, traffic class & flow
> > label. */
> 
> The mbuf structure [1] has __extension__ here,
> i.e. preceding the structure following the integer field,
> instead of preceding the union.
> 
> [1]: https://git.dpdk.org/dpdk/tree/lib/mbuf/rte_mbuf_core.h#n520

+1 anonymous unions are standard C so __extension__ is not necessary, it
is necessary for an anonymous struct.

> 
> > +		struct {
> > +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> > +			uint32_t flow_label:24; /**< flow label */
> 
> Flow Label is 20 bits, not 24.
> 
> > +			uint32_t tc:4;     /**< traffic class */
> 
> TC is 8 bits, not 4.
> 
> > +			uint32_t version:4; /**< version */
> > +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> > +			uint8_t version:4; /**< version */
> > +			uint8_t tc:4;     /**< traffic class */
> 
> :8, not :4.
> 
> > +			uint32_t flow_label:24; /**< flow label */
> 
> :20, not :24.
> 
> > +#endif
> > +		};
> > +	};
> >  	rte_be16_t payload_len;	/**< IP payload size, including ext. headers
> > */
> >  	uint8_t  proto;		/**< Protocol, next header. */
> >  	uint8_t  hop_limits;	/**< Hop limits. */
> > --
> > 2.43.0
> 

Re: [PATCH] net: add bit fields to IPv6 header definition

[Etelson, Gregory]

Hello,

>> The mbuf structure [1] has __extension__ here,
>> i.e. preceding the structure following the integer field,
>> instead of preceding the union.
>>
>> [1]: https://git.dpdk.org/dpdk/tree/lib/mbuf/rte_mbuf_core.h#n520
>
> +1 anonymous unions are standard C so __extension__ is not necessary, it
> is necessary for an anonymous struct.
>

That was fixed in the v4: 
https://mails.dpdk.org/archives/dev/2024-June/295803.html

Regards,
Gregory