net: introduce IPv4 ihl and version fields

From: dev [mailto:[email protected]] On Behalf Of Gregory Etelson
Sent: Thursday, 27 May 2021 17.29

RTE IPv4 header definition combines the version' and ihl' fields into a single structure member.
This patch introduces dedicated structure members for both version' and ihl' IPv4 fields. Separated header fields definitions allow to create simplified code to match on the IHL value in a flow rule. The original `version_ihl' structure member is kept for backward compatibility.

Signed-off-by Gregory Etelson <[email protected]>

---
 app/test/test_flow_classify.c |  8 ++++----
 lib/net/rte_ip.h              | 16 +++++++++++++++-
 2 files changed, 19 insertions(+), 5 deletions(-)

app/test/test_flow_classify.c +1 -1

index 951606f248..4f64be5357 100644
@@ -95,7 +95,7 @@ static struct rte_acl_field_def
 ipv4_defs[NUM_FIELDS_IPV4] = {
  *  dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"
  */
 static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
-	{ 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
+	{ { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
 	  RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)}
 };

static const struct rte_flow_item_ipv4 ipv4_mask_24 = {

app/test/test_flow_classify.c +3 -3

@@ -131,7 +131,7 @@ static struct rte_flow_item  end_item = {
 RTE_FLOW_ITEM_TYPE_END,
  *  dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end"
  */
 static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = {
-	{ 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
+	{ { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
 	  RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)}
 };

@@ -150,8 +150,8 @@ static struct rte_flow_item  tcp_item_1 = {
 RTE_FLOW_ITEM_TYPE_TCP,
  *  dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end"
  */
 static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = {
-	{ 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12, 13, 14),
-	RTE_IPV4(15, 16, 17, 18)}
+	{ { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0,
+	RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)}
 };
 

static struct rte_flow_item_sctp sctp_spec_1 = {

lib/net/rte_ip.h +15 -2

index 4b728969c1..684bb028b2 100644
@@ -38,7 +38,21 @@ extern "C" {
  * IPv4 Header
  */
 struct rte_ipv4_hdr {
-	uint8_t  version_ihl;		/**< version and header length */
+	__extension__
+	union {
+		uint8_t version_ihl;    /**< version and header length */
+		struct {
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+			uint8_t ihl:4;
+			uint8_t version:4;
+#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+			uint8_t version:4;
+			uint8_t ihl:4;
+#else
+#error "setup endian definition"
+#endif
+		};
+	};
 	uint8_t  type_of_service;	/**< type of service */
 	rte_be16_t total_length;	/**< length of packet */
 	rte_be16_t packet_id;		/**< packet ID */
--

2.31.1

Quoted 4 lines

From: dev [mailto:[email protected]] On Behalf Of Gregory Etelson
Sent: Thursday, 27 May 2021 17.29

and version fields

Quoted 77 lines

RTE IPv4 header definition combines the version' and ihl' fields into a single structure member.
This patch introduces dedicated structure members for both version' and ihl' IPv4 fields. Separated header fields definitions allow to create simplified code to match on the IHL value in a flow rule. The original `version_ihl' structure member is kept for backward compatibility.

Signed-off-by Gregory Etelson <[email protected]>

---
 app/test/test_flow_classify.c |  8 ++++----
 lib/net/rte_ip.h              | 16 +++++++++++++++-
 2 files changed, 19 insertions(+), 5 deletions(-)

app/test/test_flow_classify.c +1 -1

index 951606f248..4f64be5357 100644
@@ -95,7 +95,7 @@ static struct rte_acl_field_def
 ipv4_defs[NUM_FIELDS_IPV4] = {
  *  dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"
  */
 static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
-	{ 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
+	{ { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
 	  RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)}
 };

static const struct rte_flow_item_ipv4 ipv4_mask_24 = {

app/test/test_flow_classify.c +3 -3

@@ -131,7 +131,7 @@ static struct rte_flow_item  end_item = {
 RTE_FLOW_ITEM_TYPE_END,
  *  dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end"
  */
 static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = {
-	{ 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
+	{ { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
 	  RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)}
 };

@@ -150,8 +150,8 @@ static struct rte_flow_item  tcp_item_1 = {
 RTE_FLOW_ITEM_TYPE_TCP,
  *  dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end"
  */
 static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = {
-	{ 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12, 13, 14),
-	RTE_IPV4(15, 16, 17, 18)}
+	{ { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0,
+	RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)}
 };
 

static struct rte_flow_item_sctp sctp_spec_1 = {

lib/net/rte_ip.h +15 -1

index 4b728969c1..684bb028b2 100644
@@ -38,7 +38,21 @@ extern "C" {
  * IPv4 Header
  */
 struct rte_ipv4_hdr {
-	uint8_t  version_ihl;		/**< version and header length */
+	__extension__
+	union {
+		uint8_t version_ihl;    /**< version and header length */
+		struct {
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+			uint8_t ihl:4;
+			uint8_t version:4;
+#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+			uint8_t version:4;
+			uint8_t ihl:4;
+#else
+#error "setup endian definition"
+#endif
+		};
+	};
 	uint8_t  type_of_service;	/**< type of service */

Quoted 5 lines

lib/net/rte_ip.h +0 -1

 	rte_be16_t total_length;	/**< length of packet */
 	rte_be16_t packet_id;		/**< packet ID */
--

2.31.1

This does not break the ABI, but it could be discussed if it breaks the API due to the required structure initialization changes shown in test_flow_classify.c.

I think this patch is an improvement, and that such structure modifications should be generally accepted, so:

Acked-by Morten Brørup <[email protected]>

From: dev [mailto:[email protected]] On Behalf Of Ananyev, Konstantin
Sent: Friday, 28 May 2021 12.21

Quoted 1 line

Quoted 1 line

From: dev [mailto:[email protected]] On Behalf Of Gregory

Etelson

Quoted 6 lines

Quoted 1 line

Sent: Thursday, 27 May 2021 17.29

and version fields

Quoted 4 lines

RTE IPv4 header definition combines the version' and ihl' fields into a single structure member.
This patch introduces dedicated structure members for both

`version'

Quoted 72 lines

Quoted 72 lines

and ihl' IPv4 fields. Separated header fields definitions allow to create simplified code to match on the IHL value in a flow rule. The original version_ihl' structure member is kept for backward compatibility.

Signed-off-by Gregory Etelson <[email protected]>

---
 app/test/test_flow_classify.c |  8 ++++----
 lib/net/rte_ip.h              | 16 +++++++++++++++-
 2 files changed, 19 insertions(+), 5 deletions(-)

app/test/test_flow_classify.c +1 -1

index 951606f248..4f64be5357 100644
@@ -95,7 +95,7 @@ static struct rte_acl_field_def
 ipv4_defs[NUM_FIELDS_IPV4] = {
  *  dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"
  */
 static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
-	{ 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
+	{ { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
 	  RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)}
 };

static const struct rte_flow_item_ipv4 ipv4_mask_24 = {

app/test/test_flow_classify.c +3 -3

@@ -131,7 +131,7 @@ static struct rte_flow_item  end_item = {
 RTE_FLOW_ITEM_TYPE_END,
  *  dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end"
  */
 static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = {
-	{ 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
+	{ { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
 	  RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)}
 };

@@ -150,8 +150,8 @@ static struct rte_flow_item  tcp_item_1 = {
 RTE_FLOW_ITEM_TYPE_TCP,
  *  dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end"
  */
 static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = {
-	{ 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12, 13, 14),
-	RTE_IPV4(15, 16, 17, 18)}
+	{ { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0,
+	RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)}
 };
 

static struct rte_flow_item_sctp sctp_spec_1 = {

lib/net/rte_ip.h +15 -1

index 4b728969c1..684bb028b2 100644
@@ -38,7 +38,21 @@ extern "C" {
  * IPv4 Header
  */
 struct rte_ipv4_hdr {
-	uint8_t  version_ihl;		/**< version and header length */
+	__extension__
+	union {
+		uint8_t version_ihl;    /**< version and header length */
+		struct {
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+			uint8_t ihl:4;
+			uint8_t version:4;
+#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+			uint8_t version:4;
+			uint8_t ihl:4;
+#else
+#error "setup endian definition"
+#endif
+		};
+	};

Quoted 8 lines

Quoted 6 lines

lib/net/rte_ip.h +0 -1

 	uint8_t  type_of_service;	/**< type of service */
 	rte_be16_t total_length;	/**< length of packet */
 	rte_be16_t packet_id;		/**< packet ID */
--

2.31.1

This does not break the ABI, but it could be discussed if it breaks

the API due to the required structure initialization changes shown in

Quoted 1 line

test_flow_classify.c.

Yep, I guess it might be classified as API change.
Another thing that concerns me - it is not the only place in IPv4 header when we unite multiple bit-fields into one field:
type_of_service, fragment_offset.
If we start splitting ipv4 fields into actual bitfields, I suppose we'll end-up splitting these ones too.
But I am not sure it will pay off - as compiler not always generates optimal code for reading/updating bitfields.
Did you consider just adding extra macros to simplify access to these fields (like RTE_IPV4_HDR_(GET_SET)_*),
instead?

Quoted 1 line

I think this patch is an improvement, and that such structure

modifications should be generally accepted, so:

Quoted 2 lines

Acked-by Morten Brørup <[email protected]>

Quoted 15 lines

Quoted 3 lines

Quoted 3 lines

RTE IPv4 header definition combines the version' and ihl' fields into a single structure member.
This patch introduces dedicated structure members for both

`version'

Quoted 11 lines

Quoted 11 lines

and ihl' IPv4 fields. Separated header fields definitions allow to create simplified code to match on the IHL value in a flow rule. The original version_ihl' structure member is kept for backward compatibility.

Signed-off-by Gregory Etelson <[email protected]>

---
 app/test/test_flow_classify.c |  8 ++++----
 lib/net/rte_ip.h              | 16 +++++++++++++++-
 2 files changed, 19 insertions(+), 5 deletions(-)

Quoted 15 lines

Quoted 7 lines

Quoted 7 lines

(quoted diff) +0 -0

b/app/test/test_flow_classify.c index 951606f248..4f64be5357 100644

app/test/test_flow_classify.c +0 -0

@@ -95,7 +95,7 @@ static struct rte_acl_field_def
 ipv4_defs[NUM_FIELDS_IPV4] = {

Quoted 8 lines

Quoted 8 lines

app/test/test_flow_classify.c +1 -1

  *  dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"
  */
 static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
- { 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
+ { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
    RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)}  };  static const

struct rte_flow_item_ipv4 ipv4_mask_24 = { @@ -131,7 +131,7 @@ static struct rte_flow_item end_item = {

RTE_FLOW_ITEM_TYPE_END,

Quoted 54 lines

Quoted 43 lines

Quoted 43 lines

* dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end" */
static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = {
- { 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
+ { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,

    RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)}  };

app/test/test_flow_classify.c +2 -2

@@ -150,8 +150,8 @@ static struct rte_flow_item  tcp_item_1 = {
 RTE_FLOW_ITEM_TYPE_TCP,
  *  dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end"
  */
 static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = {
- { 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12, 13, 14),
- RTE_IPV4(15, 16, 17, 18)}
+ { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0,
+ RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)}
 };
 

static struct rte_flow_item_sctp sctp_spec_1 = { diff --git
a/lib/net/rte_ip.h b/lib/net/rte_ip.h index 4b728969c1..684bb028b2 100644

lib/net/rte_ip.h +15 -1

@@ -38,7 +38,21 @@ extern "C" {
  * IPv4 Header
  */
 struct rte_ipv4_hdr {
- uint8_t  version_ihl;           /**< version and header length */
+ __extension__
+ union {
+         uint8_t version_ihl;    /**< version and header length */
+         struct {
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+                 uint8_t ihl:4;
+                 uint8_t version:4; #elif RTE_BYTE_ORDER ==
+RTE_BIG_ENDIAN
+                 uint8_t version:4;
+                 uint8_t ihl:4;
+#else
+#error "setup endian definition"
+#endif
+         };
+ };

Quoted 8 lines

Quoted 6 lines

lib/net/rte_ip.h +0 -1

  uint8_t  type_of_service;       /**< type of service */
  rte_be16_t total_length;        /**< length of packet */
  rte_be16_t packet_id;           /**< packet ID */
--

2.31.1

This does not break the ABI, but it could be discussed if it breaks

the API due to the required structure initialization changes shown in

Quoted 1 line

test_flow_classify.c.

Quoted 11 lines

Yep, I guess it might be classified as API change.
Another thing that concerns me - it is not the only place in IPv4 header when we unite multiple bit-fields into one field:
type_of_service, fragment_offset.
If we start splitting ipv4 fields into actual bitfields, I suppose we'll end-up splitting these ones too.
But I am not sure it will pay off - as compiler not always generates optimal code for reading/updating bitfields.
Did you consider just adding extra macros to simplify access to these fields (like RTE_IPV4_HDR_(GET_SET)_*), instead?

Let's please not introduce accessor macros for bitfields. If we don't introduce bitfields like these, I would rather stick with the current _MASK, _SHIFT and _FLAG defines.

Yes, this change will lead to the introduction of more bitfields, both here and in other places. We already accepted it in the eCPRI structure (/lib/net/rte_ecpri.h), so why not just generally accept it.

Are modern compilers really worse at handling a bitfield defined like this, compared to handling a single uint8_t with hand coding? I consider your concern very important, so I'm only asking if it is still relevant, to avoid making decisions based on past experience that might be outdated. (I admit to falling into that trap myself, once in a while.)

Quoted 5 lines

Quoted 1 line

I think this patch is an improvement, and that such structure

modifications should be generally accepted, so:

Quoted 2 lines

Acked-by Morten Brørup <[email protected]>

Quoted 107 lines

Quoted 17 lines

Quoted 5 lines

Quoted 5 lines

RTE IPv4 header definition combines the version' and ihl' fields into a single structure member.
This patch introduces dedicated structure members for both

`version'

Quoted 11 lines

Quoted 11 lines

and ihl' IPv4 fields. Separated header fields definitions allow to create simplified code to match on the IHL value in a flow rule. The original version_ihl' structure member is kept for backward compatibility.

Signed-off-by Gregory Etelson <[email protected]>

---
 app/test/test_flow_classify.c |  8 ++++----
 lib/net/rte_ip.h              | 16 +++++++++++++++-
 2 files changed, 19 insertions(+), 5 deletions(-)

Quoted 15 lines

Quoted 7 lines

Quoted 7 lines

(quoted diff) +0 -0

b/app/test/test_flow_classify.c index 951606f248..4f64be5357 100644

app/test/test_flow_classify.c +0 -0

@@ -95,7 +95,7 @@ static struct rte_acl_field_def
 ipv4_defs[NUM_FIELDS_IPV4] = {

Quoted 8 lines

Quoted 8 lines

app/test/test_flow_classify.c +1 -1

  *  dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"
  */
 static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
- { 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
+ { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
    RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)}  };  static const

struct rte_flow_item_ipv4 ipv4_mask_24 = { @@ -131,7 +131,7 @@ static struct rte_flow_item end_item = {

RTE_FLOW_ITEM_TYPE_END,

Quoted 54 lines

Quoted 43 lines

Quoted 43 lines

* dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end" */
static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = {
- { 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
+ { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,

    RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)}  };

app/test/test_flow_classify.c +2 -2

@@ -150,8 +150,8 @@ static struct rte_flow_item  tcp_item_1 = {
 RTE_FLOW_ITEM_TYPE_TCP,
  *  dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end"
  */
 static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = {
- { 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12, 13, 14),
- RTE_IPV4(15, 16, 17, 18)}
+ { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0,
+ RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)}
 };
 

static struct rte_flow_item_sctp sctp_spec_1 = { diff --git
a/lib/net/rte_ip.h b/lib/net/rte_ip.h index 4b728969c1..684bb028b2 100644

lib/net/rte_ip.h +15 -1

@@ -38,7 +38,21 @@ extern "C" {
  * IPv4 Header
  */
 struct rte_ipv4_hdr {
- uint8_t  version_ihl;           /**< version and header length */
+ __extension__
+ union {
+         uint8_t version_ihl;    /**< version and header length */
+         struct {
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+                 uint8_t ihl:4;
+                 uint8_t version:4; #elif RTE_BYTE_ORDER ==
+RTE_BIG_ENDIAN
+                 uint8_t version:4;
+                 uint8_t ihl:4;
+#else
+#error "setup endian definition"
+#endif
+         };
+ };

Quoted 8 lines

Quoted 6 lines

lib/net/rte_ip.h +0 -1

  uint8_t  type_of_service;       /**< type of service */
  rte_be16_t total_length;        /**< length of packet */
  rte_be16_t packet_id;           /**< packet ID */
--

2.31.1

This does not break the ABI, but it could be discussed if it breaks

the API due to the required structure initialization changes shown in

Quoted 1 line

test_flow_classify.c.

Quoted 11 lines

Yep, I guess it might be classified as API change.
Another thing that concerns me - it is not the only place in IPv4 header when we unite multiple bit-fields into one field:
type_of_service, fragment_offset.
If we start splitting ipv4 fields into actual bitfields, I suppose we'll end-up splitting these ones too.
But I am not sure it will pay off - as compiler not always generates optimal code for reading/updating bitfields.
Did you consider just adding extra macros to simplify access to these fields (like RTE_IPV4_HDR_(GET_SET)_*), instead?

Let's please not introduce accessor macros for bitfields. If we don't introduce bitfields like these, I would rather stick with the current _MASK, _SHIFT and _FLAG defines.

Yes, this change will lead to the introduction of more bitfields, both here and in other places. We already accepted it in the eCPRI structure (/lib/net/rte_ecpri.h), so why not just generally accept it.

Quoted 6 lines

Are modern compilers really worse at handling a bitfield defined like this, compared to handling a single uint8_t with hand coding? I consider your concern very important, so I'm only asking if it is still relevant, to avoid making decisions based on past experience that might be outdated. (I admit to falling into that trap myself, once in a while.)

I compared x86 code generated with gcc-9, gcc-10 and clang-10 for these 2 functions:
void test_ipv4_hdr_byte(struct rte_ipv4_hdr *h, uint8_t version, uint8_t ihl)

{
	h->version_ihl = ((version & 0x0f) << 4) | (ihl & 0x0f);
}
void test_ipv4_hdr_bits(struct rte_ipv4_hdr *h, uint8_t version, uint8_t ihl)
{
	h->version = version & 0x0f;
	h->ihl = ihl & 0x0f;
}

meson configuration flags: --default-library=static --buildtype=release Each compiler produced identical code for both functions.

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

I think this patch is an improvement, and that such structure

modifications should be generally accepted, so:

Quoted 2 lines

Acked-by Morten Brørup <[email protected]>

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

Quoted 3 lines

RTE IPv4 header definition combines the version' and ihl' fields into a single structure member.
This patch introduces dedicated structure members for both

`version'

Quoted 2 lines

Quoted 2 lines

and `ihl' IPv4 fields. Separated header fields definitions allow to create simplified code to match on the IHL value in a flow

rule.

Quoted 110 lines

Quoted 102 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

The original `version_ihl' structure member is kept for backward compatibility.

Signed-off-by Gregory Etelson <[email protected]>

---
 app/test/test_flow_classify.c |  8 ++++----
 lib/net/rte_ip.h              | 16 +++++++++++++++-
 2 files changed, 19 insertions(+), 5 deletions(-)

Quoted 15 lines

Quoted 7 lines

Quoted 7 lines

(quoted diff) +0 -0

b/app/test/test_flow_classify.c index 951606f248..4f64be5357 100644

app/test/test_flow_classify.c +0 -0

@@ -95,7 +95,7 @@ static struct rte_acl_field_def
 ipv4_defs[NUM_FIELDS_IPV4] = {

Quoted 8 lines

Quoted 8 lines

app/test/test_flow_classify.c +1 -1

  *  dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"
  */
 static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
- { 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
+ { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
    RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)}  };  static

const struct rte_flow_item_ipv4 ipv4_mask_24 = { @@ -131,7 +131,7 @@ static struct rte_flow_item end_item = {

RTE_FLOW_ITEM_TYPE_END,

Quoted 57 lines

Quoted 45 lines

Quoted 45 lines

* dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end" */
static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = {
- { 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
+ { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,

    RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)}  };

app/test/test_flow_classify.c +2 -2

@@ -150,8 +150,8 @@ static struct rte_flow_item  tcp_item_1 =
 { RTE_FLOW_ITEM_TYPE_TCP,
  *  dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end"
  */
 static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = {
- { 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12, 13,
 14),
- RTE_IPV4(15, 16, 17, 18)}
+ { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0,
+ RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)}
 };

static struct rte_flow_item_sctp sctp_spec_1 = { diff --git
a/lib/net/rte_ip.h b/lib/net/rte_ip.h index 4b728969c1..684bb028b2
100644

lib/net/rte_ip.h +14 -1

@@ -38,7 +38,21 @@ extern "C" {
  * IPv4 Header
  */
 struct rte_ipv4_hdr {
- uint8_t  version_ihl;           /**< version and header length */
+ __extension__
+ union {
+         uint8_t version_ihl;    /**< version and header length */
+         struct {
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+                 uint8_t ihl:4;
+                 uint8_t version:4; #elif RTE_BYTE_ORDER ==
+RTE_BIG_ENDIAN
+                 uint8_t version:4;
+                 uint8_t ihl:4; #else #error "setup endian
+definition"
+#endif
+         };
+ };
  uint8_t  type_of_service;       /**< type of service */

Quoted 8 lines

Quoted 5 lines

lib/net/rte_ip.h +0 -1

  rte_be16_t total_length;        /**< length of packet */
  rte_be16_t packet_id;           /**< packet ID */
--

2.31.1

This does not break the ABI, but it could be discussed if it breaks

the API due to the required structure initialization changes shown in

Quoted 1 line

test_flow_classify.c.

Quoted 11 lines

Yep, I guess it might be classified as API change. Another thing that concerns me - it is not the only place in IPv4 header when we unite multiple bit-fields into one field:
type_of_service, fragment_offset.
If we start splitting ipv4 fields into actual bitfields, I suppose we'll end-up splitting these ones too.
But I am not sure it will pay off - as compiler not always generates optimal code for reading/updating bitfields. Did you consider just adding extra macros to simplify access to these fields (like RTE_IPV4_HDR_(GET_SET)_*), instead?

Let's please not introduce accessor macros for bitfields. If we don't introduce bitfields like these, I would rather stick with the current _MASK, _SHIFT and _FLAG defines.

Yes, this change will lead to the introduction of more bitfields, both here and in other places. We already accepted it in the eCPRI structure (/lib/net/rte_ecpri.h), so why not just generally accept it.

Quoted 7 lines

Are modern compilers really worse at handling a bitfield defined like this, compared to handling a single uint8_t with hand coding? I consider your concern very important, so I'm only asking if it is still relevant, to avoid making decisions based on past experience that might be outdated. (I admit to falling into that trap myself, once in a while.)

Quoted 1 line

I compared x86 code generated with gcc-9, gcc-10 and clang-10 for these

2 functions:

Quoted 10 lines

void test_ipv4_hdr_byte(struct rte_ipv4_hdr *h, uint8_t version,

uint8_t ihl) {
      h->version_ihl = ((version & 0x0f) << 4) | (ihl & 0x0f); } void
test_ipv4_hdr_bits(struct rte_ipv4_hdr *h, uint8_t version, uint8_t
ihl) {
      h->version = version & 0x0f;
      h->ihl = ihl & 0x0f;
}

meson configuration flags: --default-library=static --buildtype=release Each compiler produced identical code for both

functions.

For that particular case (2 bit-fields packed tightly into one byte) compilers usually perform quite well. At least I never saw issues for such case. Bit-fields that do cross byte boundaries - that might be a trouble.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

I think this patch is an improvement, and that such structure

modifications should be generally accepted, so:

Quoted 2 lines

Acked-by Morten Brørup <[email protected]>

-----Original Message-----
From: Gregory Etelson
Sent: Monday, May 31, 2021 14:10
To: Ananyev, Konstantin <[email protected]>; Morten Brørup <[email protected]>; [email protected]
Cc: Matan Azrad <[email protected]>; Ori Kam <[email protected]>; Raslan Darawsheh <[email protected]>; Iremonger, Bernard <[email protected]>; Olivier Matz <[email protected]>
Subject: RE: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields

Quoted 132 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

RTE IPv4 header definition combines the version' and ihl' fields into a single structure member.
This patch introduces dedicated structure members for both

`version'

Quoted 3 lines

Quoted 3 lines

and `ihl' IPv4 fields. Separated header fields definitions allow to create simplified code to match on the IHL value in a flow

rule.

Quoted 110 lines

Quoted 102 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

The original `version_ihl' structure member is kept for backward compatibility.

Signed-off-by Gregory Etelson <[email protected]>

---
 app/test/test_flow_classify.c |  8 ++++----
 lib/net/rte_ip.h              | 16 +++++++++++++++-
 2 files changed, 19 insertions(+), 5 deletions(-)

Quoted 15 lines

Quoted 7 lines

Quoted 7 lines

(quoted diff) +0 -0

b/app/test/test_flow_classify.c index 951606f248..4f64be5357 100644

app/test/test_flow_classify.c +0 -0

@@ -95,7 +95,7 @@ static struct rte_acl_field_def
 ipv4_defs[NUM_FIELDS_IPV4] = {

Quoted 8 lines

Quoted 8 lines

app/test/test_flow_classify.c +1 -1

  *  dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"
  */
 static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
- { 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
+ { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
    RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)}  };  static

const struct rte_flow_item_ipv4 ipv4_mask_24 = { @@ -131,7 +131,7 @@ static struct rte_flow_item end_item = {

RTE_FLOW_ITEM_TYPE_END,

Quoted 57 lines

Quoted 45 lines

Quoted 45 lines

* dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end" */
static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = {
- { 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
+ { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,

    RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)}  };

app/test/test_flow_classify.c +2 -2

@@ -150,8 +150,8 @@ static struct rte_flow_item  tcp_item_1
 = { RTE_FLOW_ITEM_TYPE_TCP,
  *  dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end"
  */
 static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = {
- { 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12, 13,
 14),
- RTE_IPV4(15, 16, 17, 18)}
+ { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0,
+ RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)}
 };

static struct rte_flow_item_sctp sctp_spec_1 = { diff --git
a/lib/net/rte_ip.h b/lib/net/rte_ip.h index 4b728969c1..684bb028b2
100644

lib/net/rte_ip.h +14 -1

@@ -38,7 +38,21 @@ extern "C" {
  * IPv4 Header
  */
 struct rte_ipv4_hdr {
- uint8_t  version_ihl;           /**< version and header length */
+ __extension__
+ union {
+         uint8_t version_ihl;    /**< version and header length */
+         struct {
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+                 uint8_t ihl:4;
+                 uint8_t version:4; #elif RTE_BYTE_ORDER ==
+RTE_BIG_ENDIAN
+                 uint8_t version:4;
+                 uint8_t ihl:4; #else #error "setup endian
+definition"
+#endif
+         };
+ };
  uint8_t  type_of_service;       /**< type of service */

Quoted 8 lines

Quoted 5 lines

lib/net/rte_ip.h +0 -1

  rte_be16_t total_length;        /**< length of packet */
  rte_be16_t packet_id;           /**< packet ID */
--

2.31.1

This does not break the ABI, but it could be discussed if it breaks

the API due to the required structure initialization changes shown in

Quoted 1 line

test_flow_classify.c.

Quoted 11 lines

Yep, I guess it might be classified as API change. Another thing that concerns me - it is not the only place in IPv4 header when we unite multiple bit-fields into one field:
type_of_service, fragment_offset.
If we start splitting ipv4 fields into actual bitfields, I suppose we'll end-up splitting these ones too.
But I am not sure it will pay off - as compiler not always generates optimal code for reading/updating bitfields. Did you consider just adding extra macros to simplify access to these fields (like RTE_IPV4_HDR_(GET_SET)_*), instead?

Let's please not introduce accessor macros for bitfields. If we don't introduce bitfields like these, I would rather stick with the current _MASK, _SHIFT and _FLAG defines.

Yes, this change will lead to the introduction of more bitfields, both here and in other places. We already accepted it in the eCPRI structure (/lib/net/rte_ecpri.h), so why not just generally accept it.

Quoted 7 lines

Are modern compilers really worse at handling a bitfield defined like this, compared to handling a single uint8_t with hand coding? I consider your concern very important, so I'm only asking if it is still relevant, to avoid making decisions based on past experience that might be outdated. (I admit to falling into that trap myself, once in a while.)

Quoted 2 lines

I compared x86 code generated with gcc-9, gcc-10 and clang-10 for these

2 functions:

Quoted 11 lines

void test_ipv4_hdr_byte(struct rte_ipv4_hdr *h, uint8_t version,

uint8_t ihl) {
      h->version_ihl = ((version & 0x0f) << 4) | (ihl & 0x0f); }
void test_ipv4_hdr_bits(struct rte_ipv4_hdr *h, uint8_t version,
uint8_t
ihl) {
      h->version = version & 0x0f;
      h->ihl = ihl & 0x0f;
}

meson configuration flags: --default-library=static --buildtype=release Each compiler produced identical code for both

Quoted 4 lines

functions.

For that particular case (2 bit-fields packed tightly into one byte) compilers usually perform quite well. At least I never saw issues for such

case.

Quoted 2 lines

Bit-fields that do cross byte boundaries - that might be a trouble.

Can we keep both implementations, the combined byte and the bit-field, grouped into a union ? In that case application or PMD can select access method that fits.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

I think this patch is an improvement, and that such structure

modifications should be generally accepted, so:

Quoted 2 lines

Acked-by Morten Brørup <[email protected]>

-----Original Message-----
From: Gregory Etelson
Sent: Wednesday, June 2, 2021 12:52
To: Morten Brørup <[email protected]>; Iremonger, Bernard <[email protected]>; [email protected]
Cc: Matan Azrad <[email protected]>; Ori Kam <[email protected]>; Raslan Darawsheh <[email protected]>; Olivier Matz <[email protected]>; Thomas Monjalon <[email protected]>
Subject: RE: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields

Hello,

Is there another concern about that patch ?
Please comment.

Regards,
Gregory

Quoted 4 lines

-----Original Message-----
From: Gregory Etelson
Sent: Monday, May 31, 2021 14:10
To: Ananyev, Konstantin <[email protected]>; Morten

Brørup

Quoted 2 lines

<[email protected]>; [email protected]
Cc: Matan Azrad <[email protected]>; Ori Kam <[email protected]>;

Raslan

Quoted 2 lines

Darawsheh <[email protected]>; Iremonger, Bernard <[email protected]>; Olivier Matz

<[email protected]>

Quoted 114 lines

Subject: RE: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields

Quoted 111 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

RTE IPv4 header definition combines the version' and ihl' fields into a single structure member.
This patch introduces dedicated structure members for both

`version'

Quoted 3 lines

Quoted 3 lines

and `ihl' IPv4 fields. Separated header fields definitions allow to create simplified code to match on the IHL value in a flow

rule.

Quoted 103 lines

Quoted 103 lines

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

The original `version_ihl' structure member is kept for backward compatibility.

Signed-off-by Gregory Etelson <[email protected]>

---
 app/test/test_flow_classify.c |  8 ++++----
 lib/net/rte_ip.h              | 16 +++++++++++++++-
 2 files changed, 19 insertions(+), 5 deletions(-)

(quoted diff) +0 -0

b/app/test/test_flow_classify.c index
951606f248..4f64be5357
100644

app/test/test_flow_classify.c +0 -0

@@ -95,7 +95,7 @@ static struct rte_acl_field_def
 ipv4_defs[NUM_FIELDS_IPV4] = {
  *  dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

app/test/test_flow_classify.c +1 -1

  */
 static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
- { 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
+ { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
    RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)}  };

static const struct rte_flow_item_ipv4 ipv4_mask_24 = { @@ -131,7
+131,7 @@ static struct rte_flow_item end_item = {

RTE_FLOW_ITEM_TYPE_END,

Quoted 57 lines

Quoted 45 lines

Quoted 45 lines

* dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end" */
static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = {
- { 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
+ { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,

    RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)}  };

app/test/test_flow_classify.c +2 -2

@@ -150,8 +150,8 @@ static struct rte_flow_item
 tcp_item_1 = { RTE_FLOW_ITEM_TYPE_TCP,
  *  dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end"
  */
 static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = {
- { 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12,
 13, 14),
- RTE_IPV4(15, 16, 17, 18)}
+ { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0,
+ RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)}
 };

static struct rte_flow_item_sctp sctp_spec_1 = { diff
--git a/lib/net/rte_ip.h b/lib/net/rte_ip.h index 4b728969c1..684bb028b2
100644

lib/net/rte_ip.h +14 -1

@@ -38,7 +38,21 @@ extern "C" {
  * IPv4 Header
  */
 struct rte_ipv4_hdr {
- uint8_t  version_ihl;           /**< version and header length */
+ __extension__
+ union {
+         uint8_t version_ihl;    /**< version and header length */
+         struct {
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+                 uint8_t ihl:4;
+                 uint8_t version:4; #elif RTE_BYTE_ORDER
+== RTE_BIG_ENDIAN
+                 uint8_t version:4;
+                 uint8_t ihl:4; #else #error "setup
+endian definition"
+#endif
+         };
+ };
  uint8_t  type_of_service;       /**< type of service */

Quoted 8 lines

Quoted 5 lines

lib/net/rte_ip.h +0 -1

  rte_be16_t total_length;        /**< length of packet */
  rte_be16_t packet_id;           /**< packet ID */
--

2.31.1

This does not break the ABI, but it could be discussed if it breaks

the API due to the required structure initialization changes shown in

Quoted 1 line

test_flow_classify.c.

Quoted 11 lines

Yep, I guess it might be classified as API change. Another thing that concerns me - it is not the only place in IPv4 header when we unite multiple bit-fields into one field:
type_of_service, fragment_offset.
If we start splitting ipv4 fields into actual bitfields, I suppose we'll end-up splitting these ones too.
But I am not sure it will pay off - as compiler not always generates optimal code for reading/updating bitfields. Did you consider just adding extra macros to simplify access to these fields (like RTE_IPV4_HDR_(GET_SET)_*), instead?

Let's please not introduce accessor macros for bitfields. If we don't introduce bitfields like these, I would rather stick with the current _MASK, _SHIFT and _FLAG defines.

Yes, this change will lead to the introduction of more bitfields, both here and in other places. We already accepted it in the eCPRI structure (/lib/net/rte_ecpri.h), so why not just

generally accept it.

Quoted 44 lines

Quoted 23 lines

Quoted 11 lines

Quoted 8 lines

Are modern compilers really worse at handling a bitfield defined like this, compared to handling a single uint8_t with hand coding? I consider your concern very important, so I'm only asking if it is still relevant, to avoid making decisions based on past experience that might be outdated. (I admit to falling into that trap myself, once in a while.)

I compared x86 code generated with gcc-9, gcc-10 and clang-10 for these

2 functions:

Quoted 11 lines

void test_ipv4_hdr_byte(struct rte_ipv4_hdr *h, uint8_t version,

uint8_t ihl) {
      h->version_ihl = ((version & 0x0f) << 4) | (ihl & 0x0f); }
void test_ipv4_hdr_bits(struct rte_ipv4_hdr *h, uint8_t version,
uint8_t
ihl) {
      h->version = version & 0x0f;
      h->ihl = ihl & 0x0f;
}

meson configuration flags: --default-library=static --buildtype=release Each compiler produced identical code for both

Quoted 5 lines

functions.

For that particular case (2 bit-fields packed tightly into one byte) compilers usually perform quite well. At least I never saw issues for such

case.

Quoted 2 lines

Bit-fields that do cross byte boundaries - that might be a trouble.

Can we keep both implementations, the combined byte and the bit-field, grouped into a union ? In that case application or PMD can select access method that fits.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

I think this patch is an improvement, and that such structure

modifications should be generally accepted, so:

Quoted 2 lines

Acked-by Morten Brørup <[email protected]>

Hello,

There was no activity that patch for a long time.
The patch is marked as failed, but we verified failed tests and concluded that the failures can be ignored.
https://patchwork.dpdk.org/project/dpdk/patch/[email protected]/
How should I proceed with this case ?
Please advise.

Hi Gregory,

Quoted 8 lines · Gregory Etelson

Hello,

There was no activity that patch for a long time.
The patch is marked as failed, but we verified failed tests and concluded that the failures can be ignored.
https://patchwork.dpdk.org/project/dpdk/patch/[email protected]/
How should I proceed with this case ?
Please advise.

I like the idea of this patch: to me it is more convenient to access to these fields with a bitfield. I don't see a problem about using bitfields here, glibc or FreeBSD netinet/ip.h are doing the same.

However, as stated previously, this patch breaks the initialization API.

The DPDK ABI/API policy is described here:
http://doc.dpdk.org/guides/contributing/abi_policy.html#the-dpdk-abi-policy

Quoted 1 line

From this document:

The API should only be changed for significant reasons, such as performance enhancements. API breakages due to changes such as reorganizing public structure fields for aesthetic or readability purposes should be avoided.

So to follow the project policy, I think we should reject this path.

Regards,
Olivier

Quoted 18 lines · Olivier Matz

Hi Gregory,

Quoted 10 lines · Gregory Etelson

Hello,

There was no activity that patch for a long time.
The patch is marked as failed, but we verified failed tests and concluded that the failures can be ignored.
https://patchwork.dpdk.org/project/dpdk/patch/[email protected]/

How should I proceed with this case ?
Please advise.

I like the idea of this patch: to me it is more convenient to access to these fields with a bitfield. I don't see a problem about using bitfields here, glibc or FreeBSD netinet/ip.h are doing the same.

Quoted 1 line

However, as stated previously, this patch breaks the initialization API.

Very good point. I guess we overlooked it in a number of patches with fix RTE flow API items to start from corresponding network headers. We used unions there to avoid ABI breakage, but it looks like we have broken initialization API anyway.

We should decide if initialization ABI breakage is a show-stopper for RTE flow API items switching to use network protocol headers.

Quoted 15 lines

The DPDK ABI/API policy is described here:
http://doc.dpdk.org/guides/contributing/abi_policy.html#the-dpdk-abi-policy

Quoted 1 line

From this document:

   The API should only be changed for significant reasons, such as performance enhancements. API breakages due to changes such as reorganizing public structure fields for aesthetic or readability purposes should be avoided.

So to follow the project policy, I think we should reject this path.

Regards,
Olivier

Quoted 2 lines

From: dev [mailto:[email protected]] On Behalf Of Gregory Etelson
Sent: Thursday, 27 May 2021 17.29

and version fields

Quoted 80 lines

RTE IPv4 header definition combines the version' and ihl' fields into a single structure member.
This patch introduces dedicated structure members for both version' and ihl' IPv4 fields. Separated header fields definitions allow to create simplified code to match on the IHL value in a flow rule. The original `version_ihl' structure member is kept for backward compatibility.

Signed-off-by Gregory Etelson <[email protected]>

---
  app/test/test_flow_classify.c |  8 ++++----
  lib/net/rte_ip.h              | 16 +++++++++++++++-
  2 files changed, 19 insertions(+), 5 deletions(-)

app/test/test_flow_classify.c +1 -1

index 951606f248..4f64be5357 100644
@@ -95,7 +95,7 @@ static struct rte_acl_field_def
 ipv4_defs[NUM_FIELDS_IPV4] = {
   *  dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"
   */
  static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
-	{ 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
+	{ { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
  	  RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)}
  };

  static const struct rte_flow_item_ipv4 ipv4_mask_24 = {

app/test/test_flow_classify.c +3 -3

@@ -131,7 +131,7 @@ static struct rte_flow_item  end_item = {
 RTE_FLOW_ITEM_TYPE_END,
   *  dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end"
   */
  static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = {
-	{ 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
+	{ { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
  	  RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)}
  };

@@ -150,8 +150,8 @@ static struct rte_flow_item  tcp_item_1 = {
 RTE_FLOW_ITEM_TYPE_TCP,
   *  dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end"
   */
  static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = {
-	{ 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12, 13, 14),
-	RTE_IPV4(15, 16, 17, 18)}
+	{ { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0,
+	RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)}
  };
 

  static struct rte_flow_item_sctp sctp_spec_1 = {

lib/net/rte_ip.h +15 -2

index 4b728969c1..684bb028b2 100644
@@ -38,7 +38,21 @@ extern "C" {
   * IPv4 Header
   */
  struct rte_ipv4_hdr {
-	uint8_t  version_ihl;		/**< version and header length */
+	__extension__
+	union {
+		uint8_t version_ihl;    /**< version and header length */
+		struct {
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+			uint8_t ihl:4;
+			uint8_t version:4;
+#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+			uint8_t version:4;
+			uint8_t ihl:4;
+#else
+#error "setup endian definition"
+#endif
+		};
+	};
  	uint8_t  type_of_service;	/**< type of service */
  	rte_be16_t total_length;	/**< length of packet */
  	rte_be16_t packet_id;		/**< packet ID */
--

Quoted 2 lines

2.31.1

This does not break the ABI, but it could be discussed if it breaks the API due to the required structure initialization changes shown in test_flow_classify.c. I think this patch is an improvement, and that such structure modifications should be generally accepted, so:

Acked-by Morten Brørup <[email protected]>

.

Hi, Morten and all,
I have a questions which has bothering me for a long time. What's the difference between API and ABI？
Why does this patch does not breake ABI， but break API(maybe)?

Hope for your reply, thanks.

Quoted 5 lines · On Thu, 3 Jun 2021 08:58:42 +0800 "Min Hu (Connor)" <[email protected]> wrote:

Hi, Morten and all,
I have a questions which has bothering me for a long time. What's the difference between API and ABI？
Why does this patch does not breake ABI， but break API(maybe)?

Quoted 1 line

Hope for your reply, thanks.

The API being fixed, that a user can in confidence recompile their source code and it will compile without any new errors.

The ABI guarantee means, that an application dynamically linked to DPDK shared libraries will work without problem if the DPDK libraries are updated.

lib/net/rte_ip.h +14 -0

+	union {
+		uint8_t version_ihl;    /**< version and header length */
+		struct {
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+			uint8_t ihl:4;
+			uint8_t version:4;
+#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+			uint8_t version:4;
+			uint8_t ihl:4;
+#else
+#error "setup endian definition"
+#endif
+		};
+	};
 	uint8_t  type_of_service;	/**< type of service */
 	rte_be16_t total_length;	/**< length of packet */
 	rte_be16_t packet_id;		/**< packet ID */

Signature · 1 line

2.31.1

From: Gregory Etelson <[email protected]>

RTE IPv4 header definition combines the version' and ihl' fields into a single structure member.
This patch introduces dedicated structure members for both version' and ihl' IPv4 fields. Separated header fields definitions allow to create simplified code to match on the IHL value in a flow rule. The original `version_ihl' structure member is kept for backward compatibility.

Signed-off-by Gregory Etelson <[email protected]>

Depends-on: f7383e7c7ec1 ("net: announce changes in IPv4 header access")

lib/net/rte_ip.h +0 -1

@@ -38,7 +38,21 @@ extern "C" {
  * IPv4 Header
  */
 struct rte_ipv4_hdr {
-	uint8_t  version_ihl;		/**< version and header length */

nit: although it's obvious, we may want to add
/**< IP version */ and
/**< header length */ for these new fields, for
consistency with the
rest of the structure.

app/test/test_flow_classify.c static struct rte_acl_field_def ipv4_defs[NUM_FIELDS_IPV4] = { · line 100

Quoted 12 lines · On Mon, 4 Oct 2021 15:13:22 +0300 Gregory Etelson <[email protected]> wrote:

app/test/test_flow_classify.c +1 -1

index 951606f248..4f64be5357 100644
@@ -95,7 +95,7 @@ static struct rte_acl_field_def ipv4_defs[NUM_FIELDS_IPV4] = {
  *  dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"
  */
 static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
-	{ 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
+	{ { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
 	  RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)}
 };

This ends up being an API change which was not announced.

It was announced (commit f7383e7c7ec1). But that's a good point:
- the removal of the deprecation is missing in the patch

- the message notice also talks about union for
fragment_offset

RTE IPv4 header definition combines the version' and ihl' fields into a single structure member.
This patch introduces dedicated structure members for both version' and ihl' IPv4 fields. Separated header fields definitions allow to create simplified code to match on the IHL value in a flow rule. The original `version_ihl' structure member is kept for backward compatibility.
The patch implements one of 2 announced changes to the IPv4 header.
IPv4 header encodes fragment information into 16 bits field. 3 bits hold flags and remaining 13 bits are for fragment offset. 13 bits bit-field cannot be defined both for big and little endian systems.

Signed-off-by Gregory Etelson <[email protected]>

Acked-by Olivier Matz <[email protected]>

RTE IPv4 header definition combines the version' and ihl' fields into a single structure member.
This patch introduces dedicated structure members for both version' and ihl' IPv4 fields. Separated header fields definitions allow to create simplified code to match on the IHL value in a flow rule. The original `version_ihl' structure member is kept for backward compatibility.
The patch implements one of 2 announced changes to the IPv4 header.
IPv4 header encodes fragment information into 16 bits field. 3 bits hold flags and remaining 13 bits are for fragment offset. 13 bits bit-field cannot be defined both for big and little endian systems.

Signed-off-by Gregory Etelson <[email protected]>

Acked-by Olivier Matz <[email protected]>

The deprecation notice for fragment_offset seems not implemented but removed.

What do you think to remove the relevant part that is implemented in this patch, remove the residue in another patch explaining why it is not implemented?

IPv4 header encodes fragment information into 16 bits field. 3 bits hold flags and remaining 13 bits are for fragment offset. 13 bits bit-field cannot be defined both for big and little endian systems.

The patch removes IPv4 fragments union announce.

Fixes f7383e7c7ec1 ("net: announce changes in IPv4 header access")

Signed-off-by Gregory Etelson <[email protected]>

13/10/2021 19:13, Gregory Etelson:

Quoted 7 lines

IPv4 header encodes fragment information into 16 bits field. 3 bits hold flags and remaining 13 bits are for fragment offset. 13 bits bit-field cannot be defined both for big and little endian systems.

The patch removes IPv4 fragments union announce.

Quoted 3 lines

Fixes f7383e7c7ec1 ("net: announce changes in IPv4 header access")

Signed-off-by Gregory Etelson <[email protected]>

OK to drop this announce.
There is no implementation anyway,
it will be back in one year if there is a solution.

Acked-by Thomas Monjalon <[email protected]>

Quoted 2 lines · Thomas Monjalon

13/10/2021 19:13, Gregory Etelson:

Quoted 1 line

IPv4 header encodes fragment information

into 16 bits field.

Quoted 1 line

Quoted 1 line

3 bits hold flags and remaining 13 bits are for

fragment offset.

Quoted 1 line

Quoted 1 line

13 bits bit-field cannot be defined both for big

and little endian

Quoted 3 lines

Quoted 3 lines

systems.

The patch removes IPv4 fragments union

announce.

Quoted 2 lines

Quoted 2 lines

Fixes f7383e7c7ec1 ("net: announce changes

in IPv4 header access")

Quoted 2 lines

Quoted 2 lines

Signed-off-by Gregory Etelson

<[email protected]>

Quoted 1 line

Quoted 4 lines

OK to drop this announce.
There is no implementation anyway,
it will be back in one year if there is a solution.

If there is an option to have it back, why not
keep it in the deprecation
notice, this ensures we won't forgot it.

Quoted 1 line

Acked-by Thomas Monjalon

<[email protected]>

Quoted 2 lines

Hello Ferruh,

Quoted 20 lines

Quoted 2 lines · Thomas Monjalon

13/10/2021 19:13, Gregory Etelson:

Quoted 1 line

IPv4 header encodes fragment information

into 16 bits field.

Quoted 1 line

Quoted 1 line

3 bits hold flags and remaining 13 bits are for

fragment offset.

Quoted 1 line

Quoted 1 line

13 bits bit-field cannot be defined both for big

and little endian

Quoted 3 lines

Quoted 3 lines

systems.

The patch removes IPv4 fragments union

announce.

Quoted 2 lines

Quoted 2 lines

Fixes f7383e7c7ec1 ("net: announce changes

in IPv4 header access")

Quoted 2 lines

Quoted 2 lines

Signed-off-by Gregory Etelson

<[email protected]>

Quoted 1 line

Quoted 9 lines

Quoted 4 lines

OK to drop this announce.
There is no implementation anyway,
it will be back in one year if there is a solution.

If there is an option to have it back, why not
keep it in the deprecation
notice, this ensures we won't forgot it.

It doesn't look that 3/13 bits split can be implemented as

#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
	/* LE bit-fields */
# elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
	/* BE bit-fields */
#endif

The minimal bits ratio for that option is 4/12.

Quoted 4 lines

Quoted 1 line

Acked-by Thomas Monjalon

<[email protected]>

Quoted 2 lines

-----Original Message-----
From: dev <[email protected]> On Behalf Of Ferruh Yigit
Sent: Thursday, October 14, 2021 3:33 PM
Subject: Re: [dpdk-dev] [PATCH v6 1/2] net: fix IPv4 change announce

Quoted 39 lines · Gregory Etelson

Hello Ferruh,

Quoted 20 lines

Quoted 2 lines · Thomas Monjalon

13/10/2021 19:13, Gregory Etelson:

Quoted 1 line

IPv4 header encodes fragment information

into 16 bits field.

Quoted 1 line

Quoted 1 line

3 bits hold flags and remaining 13 bits are for

fragment offset.

Quoted 1 line

Quoted 1 line

13 bits bit-field cannot be defined both for big

and little endian

Quoted 3 lines

Quoted 3 lines

systems.

The patch removes IPv4 fragments union

announce.

Quoted 2 lines

Quoted 2 lines

Fixes f7383e7c7ec1 ("net: announce changes

in IPv4 header access")

Quoted 2 lines

Quoted 2 lines

Signed-off-by Gregory Etelson

<[email protected]>

Quoted 1 line

Quoted 8 lines

Quoted 4 lines

OK to drop this announce.
There is no implementation anyway,
it will be back in one year if there is a solution.

If there is an option to have it back, why not keep it in the deprecation notice, this ensures we won't forgot it.

It doesn't look that 3/13 bits split can be implemented as

#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
	/* LE bit-fields */
# elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
	/* BE bit-fields */
#endif

Quoted 2 lines

The minimal bits ratio for that option is 4/12.

I got your point, no objection from me there.

So we want to completely abandon the change, and I expect people who acked the deprecation notice at first place would like to ack dismissing it.

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

Acked-by Thomas Monjalon

<[email protected]>

Quoted 2 lines

13/10/2021 19:13, Gregory Etelson:

Quoted 9 lines

IPv4 header encodes fragment information into 16 bits field. 3 bits hold flags and remaining 13 bits are for fragment offset. 13 bits bit-field cannot be defined both for big and little endian systems.

The patch removes IPv4 fragments union announce.

Fixes f7383e7c7ec1 ("net: announce changes in IPv4 header access")

Quoted 1 line

Signed-off-by Gregory Etelson <[email protected]>

OK to drop this announce.
There is no implementation anyway,
it will be back in one year if there is a solution.

Acked-by Thomas Monjalon <[email protected]>

  struct rte_ipv4_hdr {

lib/net/rte_ip.h +3 -1

-	uint8_t  version_ihl;		/**< version and header length */
+	__extension__
+	union {
+		uint8_t version_ihl;    /**< version and header length */

Quoted 11 lines

  struct rte_ipv4_hdr {

lib/net/rte_ip.h +9 -1

-	uint8_t  version_ihl;		/**< version and header length */
+	__extension__
+	union {
+		uint8_t version_ihl;    /**< version and header length */
+		struct {
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+			uint8_t ihl:4;     /**< header length */
+			uint8_t version:4; /**< version */
+#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+			uint8_t version:4; /**< version */

lib/net/rte_ip.h extern "C" { · line 54

Quoted 4 lines

lib/net/rte_ip.h +4 -0

+			uint8_t ihl:4;     /**< header length */
+#else
+#error "setup endian definition"
+#endif

Do we need the last 'else' part?
Although it is harmless to have it, other protocol headers for endianness check doesn't have this part, so I think better to be consistent.

Quoted 2 lines

Quoted 2 lines

struct rte_ipv4_hdr {
- uint8_t version_ihl; /**< version

and header length */

Quoted 3 lines

lib/net/rte_ip.h extern "C" {

Quoted 3 lines

lib/net/rte_ip.h +3 -0

+   __extension__
+   union {
+           uint8_t version_ihl;    /**< version

and header length */

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+           struct {
+#if RTE_BYTE_ORDER ==

RTE_LITTLE_ENDIAN

Quoted 1 line

Quoted 1 line

+ uint8_t ihl:4; /**< header

length */

Quoted 1 line

Quoted 1 line

+ uint8_t version:4; /**< version

*/

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+                   uint8_t version:4; /**< version

*/

Quoted 1 line

Quoted 1 line

+ uint8_t ihl:4; /**< header

length */

Quoted 6 lines

lib/net/rte_ip.h extern "C" { · line 54

Quoted 3 lines

lib/net/rte_ip.h +3 -0

+#else
+#error "setup endian definition"
+#endif

Do we need the last 'else' part?
Although it is harmless to have it, other

protocol headers for endianness

Quoted 1 line

check doesn't have this part, so I think better

to be consistent.

In lib/eal/include/generic/rte_byteorder.h we
already have
#if !defined(RTE_BYTE_ORDER)
#error Unknown endianness.
#endif

So indeed we don't need this last else part.

Gregory Etelson (2):
net: fix IPv4 change announce
net: introduce IPv4 ihl and version fields

Quoted 4 lines

Gregory Etelson (2):
net: fix IPv4 change announce
net: introduce IPv4 ihl and version fields

Hi Gregory,

Can you please change the order of the first and second patch?

This way I can get the first one, since it is already acked, before -rc1, and continue reviews for second one, it will be OK since it is a doc patch.

14/10/2021 10:21, Ferruh Yigit:

Quoted 8 lines · Gregory Etelson

Quoted 4 lines

Gregory Etelson (2):
net: fix IPv4 change announce
net: introduce IPv4 ihl and version fields

Hi Gregory,

Quoted 5 lines

Can you please change the order of the first and second patch?

This way I can get the first one, since it is already acked, before -rc1, and continue reviews for second one, it will be OK since it is a doc patch.

It makes more sense in this order I think.
The first patch is just dropping a deprecation note, I can ack.

Quoted 4 lines

Gregory Etelson (2):
net: fix IPv4 change announce
net: introduce IPv4 ihl and version fields

Hi Gregory,

Can you please change the order of the first and
second patch?

This way I can get the first one, since it is already acked, before -rc1,
and continue reviews for second one, it will be OK since it is a
doc patch.

Thanks,
ferruh

Hello Ferruh,

Quoted 6 lines · Gregory Etelson

Quoted 4 lines

Gregory Etelson (2):
net: fix IPv4 change announce
net: introduce IPv4 ihl and version fields

Quoted 5 lines

Hi Gregory,

Can you please change the order of the first and
second patch?

In the existing order, the code patch reflects announced changes.

Quoted 8 lines

This way I can get the first one, since it is already acked, before -rc1,
and continue reviews for second one, it will be OK since it is a
doc patch.

Thanks,
ferruh

-----Original Message-----
From: Gregory Etelson <[email protected]>
Sent: Thursday, October 14, 2021 20:41
To: [email protected]; Gregory Etelson <[email protected]>
Cc: Matan Azrad <[email protected]>; Raslan Darawsheh <[email protected]>; [email protected]; NBU-Contact-Thomas Monjalon <[email protected]>; [email protected]; Akhil Goyal <[email protected]>; Ori Kam <[email protected]>; Ray Kinsella <[email protected]>; Slava Ovsiienko <[email protected]>; Andrew Rybchenko <[email protected]>; Ajit Khaparde <[email protected]>
Subject: [PATCH v7 1/2] net: fix IPv4 change announce

IPv4 header encodes fragment information into 16 bits field.
3 bits hold flags and remaining 13 bits are for fragment offset. 13 bits bit-field cannot be defined both for big and little endian systems.

The patch removes IPv4 fragments union announce.

Fixes f7383e7c7ec1 ("net: announce changes in IPv4 header access")

Signed-off-by Gregory Etelson <[email protected]>

Acked-by Thomas Monjalon <[email protected]>

Acked-by Akhil Goyal <[email protected]>

Acked-by Ori Kam <[email protected]>

Quoted 20 lines

-----Original Message-----
From: Gregory Etelson <[email protected]>
Sent: Thursday, October 14, 2021 20:41
To: [email protected]; Gregory Etelson <[email protected]>
Cc: Matan Azrad <[email protected]>; Raslan Darawsheh <[email protected]>; [email protected]; NBU-Contact-Thomas Monjalon <[email protected]>; [email protected]; Akhil Goyal <[email protected]>; Ori Kam <[email protected]>; Ray Kinsella <[email protected]>; Slava Ovsiienko <[email protected]>; Andrew Rybchenko <[email protected]>; Ajit Khaparde <[email protected]>
Subject: [PATCH v7 1/2] net: fix IPv4 change announce

IPv4 header encodes fragment information into 16 bits field.
3 bits hold flags and remaining 13 bits are for fragment offset. 13 bits bit-field cannot be defined both for big and little endian systems.

The patch removes IPv4 fragments union announce.

Quoted 6 lines

Fixes f7383e7c7ec1 ("net: announce changes in IPv4 header access")

Signed-off-by Gregory Etelson <[email protected]>

Acked-by Thomas Monjalon <[email protected]>

Acked-by Akhil Goyal <[email protected]>

Acked-by Ori Kam <[email protected]>

Acked-by Viacheslav Ovsiienko <[email protected]>

RTE IPv4 header definition combines the version' and ihl' fields into a single structure member.
This patch introduces dedicated structure members for both version' and ihl' IPv4 fields. Separated header fields definitions allow to create simplified code to match on the IHL value in a flow rule. The original `version_ihl' structure member is kept for backward compatibility.

Signed-off-by Gregory Etelson <[email protected]>

Acked-by Olivier Matz <[email protected]>

Acked-by Ray Kinsella <[email protected]>

---
v2: Add dependency.
v3: Add comments.
v4: Update release notes.
v5: Remove deprecation notice.
Update the patch comment.
v6: split the patch into 2 parts:
1: update the announce.
2: implement the IPv4 header changes.
v7: Remove #else.

---
 app/test/test_flow_classify.c          |  8 ++++----
 doc/guides/rel_notes/deprecation.rst   |  3 ---
 doc/guides/rel_notes/release_21_11.rst |  3 +++
 lib/net/rte_ip.h                       | 14 +++++++++++++-
 4 files changed, 20 insertions(+), 8 deletions(-)

app/test/test_flow_classify.c +4 -4

index 951606f248..4f64be5357 100644
@@ -95,7 +95,7 @@ static struct rte_acl_field_def ipv4_defs[NUM_FIELDS_IPV4] = {
  *  dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"
  */
 static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
-       { 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
+       { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
          RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)}
 };
 static const struct rte_flow_item_ipv4 ipv4_mask_24 = {

@@ -131,7 +131,7 @@ static struct rte_flow_item  end_item = { RTE_FLOW_ITEM_TYPE_END,
  *  dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end"
  */
 static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = {
-       { 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
+       { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
          RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)}
 };
 

@@ -150,8 +150,8 @@ static struct rte_flow_item  tcp_item_1 = { RTE_FLOW_ITEM_TYPE_TCP,
  *  dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end"
  */
 static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = {
-       { 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12, 13, 14),
-       RTE_IPV4(15, 16, 17, 18)}
+       { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0,
+       RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)}
 };
 
 static struct rte_flow_item_sctp sctp_spec_1 = {

doc/guides/rel_notes/deprecation.rst +0 -3

index 841653fe30..d1f3faac39 100644
@@ -158,9 +158,6 @@ Deprecation Notices
   consistent with existing outer header checksum status flag naming, which
   should help in reducing confusion about its usage.
 
-* net: The structure ``rte_ipv4_hdr`` will have a union for
-  existing ``version_ihl`` byte and new bitfield for ``version`` and ``ihl``.
-
 * vhost: ``rte_vdpa_register_device``, ``rte_vdpa_unregister_device``,
   ``rte_vhost_host_notifier_ctrl`` and ``rte_vdpa_relay_vring_used`` vDPA
   driver interface will be marked as internal in DPDK v21.11.

doc/guides/rel_notes/release_21_11.rst +3 -0

index e1726774ba..fa60fd9a85 100644
@@ -216,6 +216,9 @@ API Changes
   the crypto/security operation. This field will be used to communicate
   events such as soft expiry with IPsec in lookaside mode.
 
+* net: Add ``version`` and ``ihl`` bit-fields to ``struct rte_ipv4_hdr``.
+  Existing ``version_ihl`` field was kept for backward compatibility.
+
 
 ABI Changes
 -----------

lib/net/rte_ip.h +13 -2

index b3d45e85db..0ef2430607 100644
@@ -39,7 +39,19 @@ extern "C" {
  * IPv4 Header
  */
 struct rte_ipv4_hdr {
-       uint8_t  version_ihl;           /**< version and header length */
+       __extension__
+       union {
+               uint8_t version_ihl;    /**< version and header length */
+               struct {
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+                       uint8_t ihl:4;     /**< header length */
+                       uint8_t version:4; /**< version */
+#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+                       uint8_t version:4; /**< version */
+                       uint8_t ihl:4;     /**< header length */
+#endif
+               };
+       };
        uint8_t  type_of_service;       /**< type of service */
        rte_be16_t total_length;        /**< length of packet */
        rte_be16_t packet_id;           /**< packet ID */
--

2.33.0

Gregory Etelson (2):
net: fix IPv4 change announce
net: introduce IPv4 ihl and version fields