DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
@ 2021-05-27 15:28 Gregory Etelson
  2021-05-27 15:56 ` Morten Brørup
                   ` (7 more replies)
  0 siblings, 8 replies; 52+ messages in thread
From: Gregory Etelson @ 2021-05-27 15:28 UTC (permalink / raw)
  To: dev; +Cc: getelson, matan, orika, rasland, Bernard Iremonger, Olivier Matz

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 <getelson@nvidia.com>
---
 app/test/test_flow_classify.c |  8 ++++----
 lib/net/rte_ip.h              | 16 +++++++++++++++-
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/app/test/test_flow_classify.c b/app/test/test_flow_classify.c
index 951606f248..4f64be5357 100644
--- a/app/test/test_flow_classify.c
+++ b/app/test/test_flow_classify.c
@@ -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 = {
diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
index 4b728969c1..684bb028b2 100644
--- a/lib/net/rte_ip.h
+++ b/lib/net/rte_ip.h
@@ -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


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

* Re: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
  2021-05-27 15:28 [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields Gregory Etelson
@ 2021-05-27 15:56 ` Morten Brørup
  2021-05-28 10:20   ` Ananyev, Konstantin
  2021-06-03  0:58   ` Min Hu (Connor)
  2021-06-17 15:02 ` Tyler Retzlaff
                   ` (6 subsequent siblings)
  7 siblings, 2 replies; 52+ messages in thread
From: Morten Brørup @ 2021-05-27 15:56 UTC (permalink / raw)
  To: Gregory Etelson, dev
  Cc: matan, orika, rasland, Bernard Iremonger, Olivier Matz

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Gregory Etelson
> Sent: Thursday, 27 May 2021 17.29
and version fields
> 
> 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 <getelson@nvidia.com>
> ---
>  app/test/test_flow_classify.c |  8 ++++----
>  lib/net/rte_ip.h              | 16 +++++++++++++++-
>  2 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/app/test/test_flow_classify.c
> b/app/test/test_flow_classify.c
> index 951606f248..4f64be5357 100644
> --- a/app/test/test_flow_classify.c
> +++ b/app/test/test_flow_classify.c
> @@ -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 = {
> diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> index 4b728969c1..684bb028b2 100644
> --- a/lib/net/rte_ip.h
> +++ b/lib/net/rte_ip.h
> @@ -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
> 

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 <mb@smartsharesystems.com>


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

* Re: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
  2021-05-27 15:56 ` Morten Brørup
@ 2021-05-28 10:20   ` Ananyev, Konstantin
  2021-05-28 10:52     ` Morten Brørup
  2021-06-03  0:58   ` Min Hu (Connor)
  1 sibling, 1 reply; 52+ messages in thread
From: Ananyev, Konstantin @ 2021-05-28 10:20 UTC (permalink / raw)
  To: Morten Brørup, Gregory Etelson, dev
  Cc: matan, orika, rasland, Iremonger, Bernard, Olivier Matz


 
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Gregory Etelson
> > Sent: Thursday, 27 May 2021 17.29
> and version fields
> >
> > 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 <getelson@nvidia.com>
> > ---
> >  app/test/test_flow_classify.c |  8 ++++----
> >  lib/net/rte_ip.h              | 16 +++++++++++++++-
> >  2 files changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/app/test/test_flow_classify.c
> > b/app/test/test_flow_classify.c
> > index 951606f248..4f64be5357 100644
> > --- a/app/test/test_flow_classify.c
> > +++ b/app/test/test_flow_classify.c
> > @@ -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 = {
> > diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> > index 4b728969c1..684bb028b2 100644
> > --- a/lib/net/rte_ip.h
> > +++ b/lib/net/rte_ip.h
> > @@ -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
> >
> 
> 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.

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?  

> I think this patch is an improvement, and that such structure modifications should be generally accepted, so:
> 
> Acked-by: Morten Brørup <mb@smartsharesystems.com>


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

* Re: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
  2021-05-28 10:20   ` Ananyev, Konstantin
@ 2021-05-28 10:52     ` Morten Brørup
  2021-05-28 14:18       ` Gregory Etelson
  0 siblings, 1 reply; 52+ messages in thread
From: Morten Brørup @ 2021-05-28 10:52 UTC (permalink / raw)
  To: Ananyev, Konstantin, Gregory Etelson, dev
  Cc: matan, orika, rasland, Iremonger, Bernard, Olivier Matz

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> Konstantin
> Sent: Friday, 28 May 2021 12.21
> 
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Gregory
> Etelson
> > > Sent: Thursday, 27 May 2021 17.29
> > and version fields
> > >
> > > 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 <getelson@nvidia.com>
> > > ---
> > >  app/test/test_flow_classify.c |  8 ++++----
> > >  lib/net/rte_ip.h              | 16 +++++++++++++++-
> > >  2 files changed, 19 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/app/test/test_flow_classify.c
> > > b/app/test/test_flow_classify.c
> > > index 951606f248..4f64be5357 100644
> > > --- a/app/test/test_flow_classify.c
> > > +++ b/app/test/test_flow_classify.c
> > > @@ -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 = {
> > > diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> > > index 4b728969c1..684bb028b2 100644
> > > --- a/lib/net/rte_ip.h
> > > +++ b/lib/net/rte_ip.h
> > > @@ -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
> > >
> >
> > 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.
> 
> 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.)


> > I think this patch is an improvement, and that such structure
> modifications should be generally accepted, so:
> >
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> 


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

* Re: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
  2021-05-28 10:52     ` Morten Brørup
@ 2021-05-28 14:18       ` Gregory Etelson
  2021-05-31  9:58         ` Ananyev, Konstantin
  0 siblings, 1 reply; 52+ messages in thread
From: Gregory Etelson @ 2021-05-28 14:18 UTC (permalink / raw)
  To: Morten Brørup, Ananyev, Konstantin, dev
  Cc: Matan Azrad, Ori Kam, Raslan Darawsheh, Iremonger, Bernard, Olivier Matz

> > > > 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 <getelson@nvidia.com>
> > > > ---
> > > >  app/test/test_flow_classify.c |  8 ++++----
> > > >  lib/net/rte_ip.h              | 16 +++++++++++++++-
> > > >  2 files changed, 19 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/app/test/test_flow_classify.c
> > > > b/app/test/test_flow_classify.c index 951606f248..4f64be5357
> > > > 100644
> > > > --- a/app/test/test_flow_classify.c
> > > > +++ b/app/test/test_flow_classify.c
> > > > @@ -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 = { diff --git
> > > > a/lib/net/rte_ip.h b/lib/net/rte_ip.h index 4b728969c1..684bb028b2
> > > > 100644
> > > > --- a/lib/net/rte_ip.h
> > > > +++ b/lib/net/rte_ip.h
> > > > @@ -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
> > > >
> > >
> > > 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.
> >
> > 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.)
> 

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. 
 

> > > I think this patch is an improvement, and that such structure
> > modifications should be generally accepted, so:
> > >
> > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> >


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

* Re: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
  2021-05-28 14:18       ` Gregory Etelson
@ 2021-05-31  9:58         ` Ananyev, Konstantin
  2021-05-31 11:10           ` Gregory Etelson
  0 siblings, 1 reply; 52+ messages in thread
From: Ananyev, Konstantin @ 2021-05-31  9:58 UTC (permalink / raw)
  To: Gregory Etelson, Morten Brørup, dev
  Cc: Matan Azrad, Ori Kam, Raslan Darawsheh, Iremonger, Bernard, Olivier Matz



> > > > > 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 <getelson@nvidia.com>
> > > > > ---
> > > > >  app/test/test_flow_classify.c |  8 ++++----
> > > > >  lib/net/rte_ip.h              | 16 +++++++++++++++-
> > > > >  2 files changed, 19 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/app/test/test_flow_classify.c
> > > > > b/app/test/test_flow_classify.c index 951606f248..4f64be5357
> > > > > 100644
> > > > > --- a/app/test/test_flow_classify.c
> > > > > +++ b/app/test/test_flow_classify.c
> > > > > @@ -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 = { diff --git
> > > > > a/lib/net/rte_ip.h b/lib/net/rte_ip.h index 4b728969c1..684bb028b2
> > > > > 100644
> > > > > --- a/lib/net/rte_ip.h
> > > > > +++ b/lib/net/rte_ip.h
> > > > > @@ -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
> > > > >
> > > >
> > > > 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.
> > >
> > > 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.)
> >
> 
> 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.

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.  

> 
> 
> > > > I think this patch is an improvement, and that such structure
> > > modifications should be generally accepted, so:
> > > >
> > > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > >


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

* Re: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
  2021-05-31  9:58         ` Ananyev, Konstantin
@ 2021-05-31 11:10           ` Gregory Etelson
  2021-06-02  9:51             ` Gregory Etelson
  0 siblings, 1 reply; 52+ messages in thread
From: Gregory Etelson @ 2021-05-31 11:10 UTC (permalink / raw)
  To: Ananyev, Konstantin, Morten Brørup, dev
  Cc: Matan Azrad, Ori Kam, Raslan Darawsheh, Iremonger, Bernard, Olivier Matz

> > > > > > 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 <getelson@nvidia.com>
> > > > > > ---
> > > > > >  app/test/test_flow_classify.c |  8 ++++----
> > > > > >  lib/net/rte_ip.h              | 16 +++++++++++++++-
> > > > > >  2 files changed, 19 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/app/test/test_flow_classify.c
> > > > > > b/app/test/test_flow_classify.c index 951606f248..4f64be5357
> > > > > > 100644
> > > > > > --- a/app/test/test_flow_classify.c
> > > > > > +++ b/app/test/test_flow_classify.c
> > > > > > @@ -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 = { diff --git
> > > > > > a/lib/net/rte_ip.h b/lib/net/rte_ip.h index
> > > > > > 4b728969c1..684bb028b2
> > > > > > 100644
> > > > > > --- a/lib/net/rte_ip.h
> > > > > > +++ b/lib/net/rte_ip.h
> > > > > > @@ -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
> > > > > >
> > > > >
> > > > > 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.
> > > >
> > > > 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.)
> > >
> >
> > 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.
> 
> 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.
> 

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.
 
> >
> >
> > > > > I think this patch is an improvement, and that such structure
> > > > modifications should be generally accepted, so:
> > > > >
> > > > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > > >


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

* Re: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
  2021-05-31 11:10           ` Gregory Etelson
@ 2021-06-02  9:51             ` Gregory Etelson
  2021-06-10  4:10               ` Gregory Etelson
  0 siblings, 1 reply; 52+ messages in thread
From: Gregory Etelson @ 2021-06-02  9:51 UTC (permalink / raw)
  To: Morten Brørup, Iremonger, Bernard, dev
  Cc: Matan Azrad, Ori Kam, Raslan Darawsheh, Olivier Matz, Thomas Monjalon

Hello,

Is there another concern about that patch ?
Please comment.

Regards,
Gregory

> -----Original Message-----
> From: Gregory Etelson
> Sent: Monday, May 31, 2021 14:10
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Morten Brørup
> <mb@smartsharesystems.com>; dev@dpdk.org
> Cc: Matan Azrad <matan@nvidia.com>; Ori Kam <orika@nvidia.com>;
> Raslan Darawsheh <rasland@nvidia.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>; Olivier Matz <olivier.matz@6wind.com>
> Subject: RE: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
> 
> > > > > > > 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 <getelson@nvidia.com>
> > > > > > > ---
> > > > > > >  app/test/test_flow_classify.c |  8 ++++----
> > > > > > >  lib/net/rte_ip.h              | 16 +++++++++++++++-
> > > > > > >  2 files changed, 19 insertions(+), 5 deletions(-)
> > > > > > >
> > > > > > > diff --git a/app/test/test_flow_classify.c
> > > > > > > b/app/test/test_flow_classify.c index 951606f248..4f64be5357
> > > > > > > 100644
> > > > > > > --- a/app/test/test_flow_classify.c
> > > > > > > +++ b/app/test/test_flow_classify.c
> > > > > > > @@ -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 = { diff --git
> > > > > > > a/lib/net/rte_ip.h b/lib/net/rte_ip.h index
> > > > > > > 4b728969c1..684bb028b2
> > > > > > > 100644
> > > > > > > --- a/lib/net/rte_ip.h
> > > > > > > +++ b/lib/net/rte_ip.h
> > > > > > > @@ -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
> > > > > > >
> > > > > >
> > > > > > 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.
> > > > >
> > > > > 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.)
> > > >
> > >
> > > 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.
> >
> > 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.
> >
> 
> 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.
> 
> > >
> > >
> > > > > > I think this patch is an improvement, and that such structure
> > > > > modifications should be generally accepted, so:
> > > > > >
> > > > > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > > > >


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

* Re: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
  2021-05-27 15:56 ` Morten Brørup
  2021-05-28 10:20   ` Ananyev, Konstantin
@ 2021-06-03  0:58   ` Min Hu (Connor)
  2021-06-03  2:03     ` Stephen Hemminger
  1 sibling, 1 reply; 52+ messages in thread
From: Min Hu (Connor) @ 2021-06-03  0:58 UTC (permalink / raw)
  To: Morten Brørup, Gregory Etelson, dev
  Cc: matan, orika, rasland, Bernard Iremonger, Olivier Matz

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.

在 2021/5/27 23:56, Morten Brørup 写道:
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Gregory Etelson
>> Sent: Thursday, 27 May 2021 17.29
> and version fields
>>
>> 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 <getelson@nvidia.com>
>> ---
>>   app/test/test_flow_classify.c |  8 ++++----
>>   lib/net/rte_ip.h              | 16 +++++++++++++++-
>>   2 files changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/app/test/test_flow_classify.c
>> b/app/test/test_flow_classify.c
>> index 951606f248..4f64be5357 100644
>> --- a/app/test/test_flow_classify.c
>> +++ b/app/test/test_flow_classify.c
>> @@ -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 = {
>> diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
>> index 4b728969c1..684bb028b2 100644
>> --- a/lib/net/rte_ip.h
>> +++ b/lib/net/rte_ip.h
>> @@ -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
>>
> 
> 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 <mb@smartsharesystems.com>
> 
> .
> 

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

* Re: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
  2021-06-03  0:58   ` Min Hu (Connor)
@ 2021-06-03  2:03     ` Stephen Hemminger
  2021-06-03  4:59       ` Gregory Etelson
  0 siblings, 1 reply; 52+ messages in thread
From: Stephen Hemminger @ 2021-06-03  2:03 UTC (permalink / raw)
  To: Min Hu (Connor)
  Cc: Morten Brørup, Gregory Etelson, dev, matan, orika, rasland,
	Bernard Iremonger, Olivier Matz

On Thu, 3 Jun 2021 08:58:42 +0800
"Min Hu (Connor)" <humin29@huawei.com> 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)?
> 	
> 	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.

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

* Re: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
  2021-06-03  2:03     ` Stephen Hemminger
@ 2021-06-03  4:59       ` Gregory Etelson
  0 siblings, 0 replies; 52+ messages in thread
From: Gregory Etelson @ 2021-06-03  4:59 UTC (permalink / raw)
  To: Stephen Hemminger, Min Hu (Connor)
  Cc: Morten Brørup, dev, Matan Azrad, Ori Kam, Raslan Darawsheh,
	Bernard Iremonger, Olivier Matz

> On Thu, 3 Jun 2021 08:58:42 +0800
> "Min Hu (Connor)" <humin29@huawei.com> 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)?
> >
> >       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.

Hello Stephen,

Thank you for the clarification.

According to the above statements, the patch introduces alternative
access method to IPv4 version & ihl fields without breaking existing API.

Regards,
Gregory

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

* Re: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
  2021-06-02  9:51             ` Gregory Etelson
@ 2021-06-10  4:10               ` Gregory Etelson
  2021-06-10  9:22                 ` Olivier Matz
  0 siblings, 1 reply; 52+ messages in thread
From: Gregory Etelson @ 2021-06-10  4:10 UTC (permalink / raw)
  To: Iremonger, Bernard, Olivier Matz, Morten Brørup, dev
  Cc: Matan Azrad, Ori Kam, Raslan Darawsheh, Asaf Penso

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/20210527152858.13312-1-getelson@nvidia.com/
How should I proceed with this case ?
Please advise.

Thank you.

Regards,
Gregory

> -----Original Message-----
> From: Gregory Etelson
> Sent: Wednesday, June 2, 2021 12:52
> To: Morten Brørup <mb@smartsharesystems.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>; dev@dpdk.org
> Cc: Matan Azrad <matan@nvidia.com>; Ori Kam <orika@nvidia.com>;
> Raslan Darawsheh <rasland@nvidia.com>; Olivier Matz
> <olivier.matz@6wind.com>; Thomas Monjalon <tmonjalon@nvidia.com>
> Subject: RE: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
> 
> Hello,
> 
> Is there another concern about that patch ?
> Please comment.
> 
> Regards,
> Gregory
> 
> > -----Original Message-----
> > From: Gregory Etelson
> > Sent: Monday, May 31, 2021 14:10
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Morten
> Brørup
> > <mb@smartsharesystems.com>; dev@dpdk.org
> > Cc: Matan Azrad <matan@nvidia.com>; Ori Kam <orika@nvidia.com>;
> Raslan
> > Darawsheh <rasland@nvidia.com>; Iremonger, Bernard
> > <bernard.iremonger@intel.com>; Olivier Matz
> <olivier.matz@6wind.com>
> > Subject: RE: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version
> > fields
> >
> > > > > > > > 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 <getelson@nvidia.com>
> > > > > > > > ---
> > > > > > > >  app/test/test_flow_classify.c |  8 ++++----
> > > > > > > >  lib/net/rte_ip.h              | 16 +++++++++++++++-
> > > > > > > >  2 files changed, 19 insertions(+), 5 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/app/test/test_flow_classify.c
> > > > > > > > b/app/test/test_flow_classify.c index
> > > > > > > > 951606f248..4f64be5357
> > > > > > > > 100644
> > > > > > > > --- a/app/test/test_flow_classify.c
> > > > > > > > +++ b/app/test/test_flow_classify.c
> > > > > > > > @@ -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 = { diff
> > > > > > > > --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h index
> > > > > > > > 4b728969c1..684bb028b2
> > > > > > > > 100644
> > > > > > > > --- a/lib/net/rte_ip.h
> > > > > > > > +++ b/lib/net/rte_ip.h
> > > > > > > > @@ -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
> > > > > > > >
> > > > > > >
> > > > > > > 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.
> > > > > >
> > > > > > 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.)
> > > > >
> > > >
> > > > 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.
> > >
> > > 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.
> > >
> >
> > 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.
> >
> > > >
> > > >
> > > > > > > I think this patch is an improvement, and that such
> > > > > > > structure
> > > > > > modifications should be generally accepted, so:
> > > > > > >
> > > > > > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > > > > >


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

* Re: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
  2021-06-10  4:10               ` Gregory Etelson
@ 2021-06-10  9:22                 ` Olivier Matz
  2021-06-14 16:36                   ` Andrew Rybchenko
  0 siblings, 1 reply; 52+ messages in thread
From: Olivier Matz @ 2021-06-10  9:22 UTC (permalink / raw)
  To: Gregory Etelson
  Cc: Iremonger, Bernard, Morten Brørup, dev, Matan Azrad,
	Ori Kam, Raslan Darawsheh, Asaf Penso

Hi Gregory,

On Thu, Jun 10, 2021 at 04:10:25AM +0000, Gregory Etelson wrote:
> 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/20210527152858.13312-1-getelson@nvidia.com/
> 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

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

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

* Re: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
  2021-06-10  9:22                 ` Olivier Matz
@ 2021-06-14 16:36                   ` Andrew Rybchenko
  2021-06-17 16:29                     ` Ferruh Yigit
  0 siblings, 1 reply; 52+ messages in thread
From: Andrew Rybchenko @ 2021-06-14 16:36 UTC (permalink / raw)
  To: Olivier Matz, Gregory Etelson
  Cc: Iremonger, Bernard, Morten Brørup, dev, Matan Azrad,
	Ori Kam, Raslan Darawsheh, Asaf Penso, Thomas Monjalon,
	Ferruh Yigit

On 6/10/21 12:22 PM, Olivier Matz wrote:
> Hi Gregory,
> 
> On Thu, Jun 10, 2021 at 04:10:25AM +0000, Gregory Etelson wrote:
>> 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/20210527152858.13312-1-getelson@nvidia.com/
>> 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.

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.

> The DPDK ABI/API policy is described here:
> http://doc.dpdk.org/guides/contributing/abi_policy.html#the-dpdk-abi-policy
> 
>>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
> 


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

* Re: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
  2021-05-27 15:28 [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields Gregory Etelson
  2021-05-27 15:56 ` Morten Brørup
@ 2021-06-17 15:02 ` Tyler Retzlaff
  2021-09-03  7:30 ` [dpdk-dev] [PATCH v2] " getelson
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 52+ messages in thread
From: Tyler Retzlaff @ 2021-06-17 15:02 UTC (permalink / raw)
  To: Gregory Etelson
  Cc: dev, matan, orika, rasland, Bernard Iremonger, Olivier Matz

On Thu, May 27, 2021 at 06:28:58PM +0300, Gregory Etelson wrote:
> diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> index 4b728969c1..684bb028b2 100644
> --- a/lib/net/rte_ip.h
> +++ b/lib/net/rte_ip.h
> @@ -38,7 +38,21 @@ extern "C" {
>   * IPv4 Header
>   */
>  struct rte_ipv4_hdr {
> -	uint8_t  version_ihl;		/**< version and header length */
> +	__extension__

this patch reduces compiler portability, though not strictly objecting
so long as the community accepts that it may lead to conditional
compilation having to be introduced in a future change.

please also be mindful of the impact of __attribute__ ((__packed__)) in
the presence of bitfields on gcc when evaluating abi compatibility.

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

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

* Re: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
  2021-06-14 16:36                   ` Andrew Rybchenko
@ 2021-06-17 16:29                     ` Ferruh Yigit
  0 siblings, 0 replies; 52+ messages in thread
From: Ferruh Yigit @ 2021-06-17 16:29 UTC (permalink / raw)
  To: Andrew Rybchenko, Olivier Matz, Gregory Etelson
  Cc: Iremonger, Bernard, Morten Brørup, dev, Matan Azrad,
	Ori Kam, Raslan Darawsheh, Asaf Penso, Thomas Monjalon

On 6/14/2021 5:36 PM, Andrew Rybchenko wrote:
> On 6/10/21 12:22 PM, Olivier Matz wrote:
>> Hi Gregory,
>>
>> On Thu, Jun 10, 2021 at 04:10:25AM +0000, Gregory Etelson wrote:
>>> 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/20210527152858.13312-1-getelson@nvidia.com/
>>>
>>> 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.
> 
> 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.
> 

Hi Andrew,

What is broken with the flow API item updates, can you please give a sample?

> We should decide if initialization ABI breakage is a show-stopper
> for RTE flow API items switching to use network protocol headers.
> 
>> The DPDK ABI/API policy is described here:
>> http://doc.dpdk.org/guides/contributing/abi_policy.html#the-dpdk-abi-policy
>>
>>> 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
>>
> 


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

* [dpdk-dev] [PATCH v2] net: introduce IPv4 ihl and version fields
  2021-05-27 15:28 [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields Gregory Etelson
  2021-05-27 15:56 ` Morten Brørup
  2021-06-17 15:02 ` Tyler Retzlaff
@ 2021-09-03  7:30 ` getelson
  2021-10-04  7:49   ` Olivier Matz
  2021-10-04  8:46 ` [dpdk-dev] [PATCH v3] " Gregory Etelson
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 52+ messages in thread
From: getelson @ 2021-09-03  7:30 UTC (permalink / raw)
  To: dev, getelson; +Cc: thomas, bernard.iremonger, olivier.matz, rasland, matan

From: Gregory Etelson <getelson@nvidia.com>

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 <getelson@nvidia.com>

Depends-on: f7383e7c7ec1 ("net: announce changes in IPv4 header access")
---
v2: add dependency
---
 app/test/test_flow_classify.c |  8 ++++----
 lib/net/rte_ip.h              | 16 +++++++++++++++-
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/app/test/test_flow_classify.c b/app/test/test_flow_classify.c
index 951606f248..4f64be5357 100644
--- a/app/test/test_flow_classify.c
+++ b/app/test/test_flow_classify.c
@@ -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 = {
diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
index 05948b69b7..ee6ec67f5d 100644
--- a/lib/net/rte_ip.h
+++ b/lib/net/rte_ip.h
@@ -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.33.0


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

* Re: [dpdk-dev] [PATCH v2] net: introduce IPv4 ihl and version fields
  2021-09-03  7:30 ` [dpdk-dev] [PATCH v2] " getelson
@ 2021-10-04  7:49   ` Olivier Matz
  2021-10-04  8:59     ` Gregory Etelson
  0 siblings, 1 reply; 52+ messages in thread
From: Olivier Matz @ 2021-10-04  7:49 UTC (permalink / raw)
  To: getelson; +Cc: dev, thomas, bernard.iremonger, rasland, matan

On Fri, Sep 03, 2021 at 10:30:03AM +0300, getelson wrote:
> From: Gregory Etelson <getelson@nvidia.com>
> 
> 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 <getelson@nvidia.com>
> 
> Depends-on: f7383e7c7ec1 ("net: announce changes in IPv4 header access")

Acked-by: Olivier Matz <olivier.matz@6wind.com>

> --- a/lib/net/rte_ip.h
> +++ b/lib/net/rte_ip.h
> @@ -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;

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.

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

* [dpdk-dev] [PATCH v3] net: introduce IPv4 ihl and version fields
  2021-05-27 15:28 [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields Gregory Etelson
                   ` (2 preceding siblings ...)
  2021-09-03  7:30 ` [dpdk-dev] [PATCH v2] " getelson
@ 2021-10-04  8:46 ` Gregory Etelson
  2021-10-04 12:13 ` [dpdk-dev] [PATCH v4] " Gregory Etelson
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 52+ messages in thread
From: Gregory Etelson @ 2021-10-04  8:46 UTC (permalink / raw)
  To: dev, getelson; +Cc: matan, rasland, olivier.matz, thomas, Bernard Iremonger

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 <getelson@nvidia.com>

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

Acked-by: Olivier Matz <olivier.matz@6wind.com>

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

diff --git a/app/test/test_flow_classify.c b/app/test/test_flow_classify.c
index 951606f248..4f64be5357 100644
--- a/app/test/test_flow_classify.c
+++ b/app/test/test_flow_classify.c
@@ -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 = {
diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
index 05948b69b7..89a68d9433 100644
--- a/lib/net/rte_ip.h
+++ b/lib/net/rte_ip.h
@@ -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;     /**< 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 */
+#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.33.0


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

* Re: [dpdk-dev] [PATCH v2] net: introduce IPv4 ihl and version fields
  2021-10-04  7:49   ` Olivier Matz
@ 2021-10-04  8:59     ` Gregory Etelson
  0 siblings, 0 replies; 52+ messages in thread
From: Gregory Etelson @ 2021-10-04  8:59 UTC (permalink / raw)
  To: Olivier Matz
  Cc: dev, NBU-Contact-Thomas Monjalon, bernard.iremonger,
	Raslan Darawsheh, Matan Azrad

Hello Olivier,

[:snip:]
> 
> 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.

I added comments to v3.

Regards,
Gregory

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

* [dpdk-dev] [PATCH v4] net: introduce IPv4 ihl and version fields
  2021-05-27 15:28 [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields Gregory Etelson
                   ` (3 preceding siblings ...)
  2021-10-04  8:46 ` [dpdk-dev] [PATCH v3] " Gregory Etelson
@ 2021-10-04 12:13 ` Gregory Etelson
  2021-10-04 15:59   ` Stephen Hemminger
  2021-10-12 12:29 ` [dpdk-dev] [PATCH v5] " Gregory Etelson
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 52+ messages in thread
From: Gregory Etelson @ 2021-10-04 12:13 UTC (permalink / raw)
  To: dev, getelson; +Cc: matan, rasland, olivier.matz, thomas, Bernard Iremonger

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 <getelson@nvidia.com>

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

Acked-by: Olivier Matz <olivier.matz@6wind.com>

---
v2: Add dependency.
v3: Add comments.
v4: Update release notes.
---
 app/test/test_flow_classify.c          |  8 ++++----
 doc/guides/rel_notes/release_21_11.rst |  3 +++
 lib/net/rte_ip.h                       | 16 +++++++++++++++-
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/app/test/test_flow_classify.c b/app/test/test_flow_classify.c
index 951606f248..4f64be5357 100644
--- a/app/test/test_flow_classify.c
+++ b/app/test/test_flow_classify.c
@@ -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 = {
diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
index 73e377a007..deab44a92a 100644
--- a/doc/guides/rel_notes/release_21_11.rst
+++ b/doc/guides/rel_notes/release_21_11.rst
@@ -170,6 +170,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
 -----------
diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
index 05948b69b7..89a68d9433 100644
--- a/lib/net/rte_ip.h
+++ b/lib/net/rte_ip.h
@@ -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;     /**< 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 */
+#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.33.0


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

* Re: [dpdk-dev] [PATCH v4] net: introduce IPv4 ihl and version fields
  2021-10-04 12:13 ` [dpdk-dev] [PATCH v4] " Gregory Etelson
@ 2021-10-04 15:59   ` Stephen Hemminger
  2021-10-04 16:48     ` Olivier Matz
  0 siblings, 1 reply; 52+ messages in thread
From: Stephen Hemminger @ 2021-10-04 15:59 UTC (permalink / raw)
  To: Gregory Etelson
  Cc: dev, matan, rasland, olivier.matz, thomas, Bernard Iremonger

On Mon, 4 Oct 2021 15:13:22 +0300
Gregory Etelson <getelson@nvidia.com> wrote:

> diff --git a/app/test/test_flow_classify.c b/app/test/test_flow_classify.c
> index 951606f248..4f64be5357 100644
> --- a/app/test/test_flow_classify.c
> +++ b/app/test/test_flow_classify.c
> @@ -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.

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

* Re: [dpdk-dev] [PATCH v4] net: introduce IPv4 ihl and version fields
  2021-10-04 15:59   ` Stephen Hemminger
@ 2021-10-04 16:48     ` Olivier Matz
  2021-10-12 12:36       ` Gregory Etelson
  0 siblings, 1 reply; 52+ messages in thread
From: Olivier Matz @ 2021-10-04 16:48 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Gregory Etelson, dev, matan, rasland, thomas, Bernard Iremonger

On Mon, Oct 04, 2021 at 08:59:34AM -0700, Stephen Hemminger wrote:
> On Mon, 4 Oct 2021 15:13:22 +0300
> Gregory Etelson <getelson@nvidia.com> wrote:
> 
> > diff --git a/app/test/test_flow_classify.c b/app/test/test_flow_classify.c
> > index 951606f248..4f64be5357 100644
> > --- a/app/test/test_flow_classify.c
> > +++ b/app/test/test_flow_classify.c
> > @@ -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

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

* [dpdk-dev] [PATCH v5] net: introduce IPv4 ihl and version fields
  2021-05-27 15:28 [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields Gregory Etelson
                   ` (4 preceding siblings ...)
  2021-10-04 12:13 ` [dpdk-dev] [PATCH v4] " Gregory Etelson
@ 2021-10-12 12:29 ` Gregory Etelson
  2021-10-13  8:58   ` Kinsella, Ray
  2021-10-13 12:14   ` Ferruh Yigit
  2021-10-13 17:13 ` [dpdk-dev] [PATCH v6 0/2] " Gregory Etelson
  2021-10-14 17:41 ` [dpdk-dev] [PATCH v7 " Gregory Etelson
  7 siblings, 2 replies; 52+ messages in thread
From: Gregory Etelson @ 2021-10-12 12:29 UTC (permalink / raw)
  To: dev, getelson
  Cc: matan, rasland, olivier.matz, thomas, Bernard Iremonger, Ray Kinsella

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 <getelson@nvidia.com>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

---
v2: Add dependency.
v3: Add comments.
v4: Update release notes.
v5: Remove deprecation notice.
    Update the patch comment.
---
 app/test/test_flow_classify.c          |  8 ++++----
 doc/guides/rel_notes/deprecation.rst   |  6 ------
 doc/guides/rel_notes/release_21_11.rst |  3 +++
 lib/net/rte_ip.h                       | 16 +++++++++++++++-
 4 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/app/test/test_flow_classify.c b/app/test/test_flow_classify.c
index 951606f248..4f64be5357 100644
--- a/app/test/test_flow_classify.c
+++ b/app/test/test_flow_classify.c
@@ -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 = {
diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index a2fe766d4b..040f4a8868 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -167,12 +167,6 @@ Deprecation Notices
 * net: ``s_addr`` and ``d_addr`` fields of ``rte_ether_hdr`` structure
   will be renamed in DPDK 21.11 to avoid conflict with Windows Sockets headers.
 
-* net: The structure ``rte_ipv4_hdr`` will have two unions.
-  The first union is for existing ``version_ihl`` byte
-  and new bitfield for version and IHL.
-  The second union is for existing ``fragment_offset``
-  and new bitfield for fragment flags and offset.
-
 * 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.
diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
index dfc2cbdeed..4fb4a1dac4 100644
--- a/doc/guides/rel_notes/release_21_11.rst
+++ b/doc/guides/rel_notes/release_21_11.rst
@@ -187,6 +187,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
 -----------
diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
index 05948b69b7..89a68d9433 100644
--- a/lib/net/rte_ip.h
+++ b/lib/net/rte_ip.h
@@ -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;     /**< 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 */
+#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.33.0


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

* Re: [dpdk-dev] [PATCH v4] net: introduce IPv4 ihl and version fields
  2021-10-04 16:48     ` Olivier Matz
@ 2021-10-12 12:36       ` Gregory Etelson
  0 siblings, 0 replies; 52+ messages in thread
From: Gregory Etelson @ 2021-10-12 12:36 UTC (permalink / raw)
  To: Olivier Matz, Stephen Hemminger
  Cc: dev, Matan Azrad, Raslan Darawsheh, NBU-Contact-Thomas Monjalon,
	Bernard Iremonger

Hello,

..snip..

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

I removed deprecation in v5

> - the message notice also talks about union for
> fragment_offset

A union for IPv4 fragmentation fields cannot be 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.

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

* Re: [dpdk-dev] [PATCH v5] net: introduce IPv4 ihl and version fields
  2021-10-12 12:29 ` [dpdk-dev] [PATCH v5] " Gregory Etelson
@ 2021-10-13  8:58   ` Kinsella, Ray
  2021-10-13 12:14   ` Ferruh Yigit
  1 sibling, 0 replies; 52+ messages in thread
From: Kinsella, Ray @ 2021-10-13  8:58 UTC (permalink / raw)
  To: Gregory Etelson, dev
  Cc: matan, rasland, olivier.matz, thomas, Bernard Iremonger



On 12/10/2021 13:29, Gregory Etelson wrote:
> 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 <getelson@nvidia.com>
> 
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
> 
Acked-by: Ray Kinsella <mdr@ashroe.eu>

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

* Re: [dpdk-dev] [PATCH v5] net: introduce IPv4 ihl and version fields
  2021-10-12 12:29 ` [dpdk-dev] [PATCH v5] " Gregory Etelson
  2021-10-13  8:58   ` Kinsella, Ray
@ 2021-10-13 12:14   ` Ferruh Yigit
  2021-10-13 17:12     ` Gregory Etelson
  1 sibling, 1 reply; 52+ messages in thread
From: Ferruh Yigit @ 2021-10-13 12:14 UTC (permalink / raw)
  To: Gregory Etelson, dev
  Cc: matan, rasland, olivier.matz, thomas, Bernard Iremonger, Ray Kinsella

On 10/12/2021 1:29 PM, Gregory Etelson wrote:
> 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 <getelson@nvidia.com>
> 
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
> 

<...>

> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -167,12 +167,6 @@ Deprecation Notices
>   * net: ``s_addr`` and ``d_addr`` fields of ``rte_ether_hdr`` structure
>     will be renamed in DPDK 21.11 to avoid conflict with Windows Sockets headers.
>   
> -* net: The structure ``rte_ipv4_hdr`` will have two unions.
> -  The first union is for existing ``version_ihl`` byte
> -  and new bitfield for version and IHL.
> -  The second union is for existing ``fragment_offset``
> -  and new bitfield for fragment flags and offset.
> -

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?

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

* Re: [dpdk-dev] [PATCH v5] net: introduce IPv4 ihl and version fields
  2021-10-13 12:14   ` Ferruh Yigit
@ 2021-10-13 17:12     ` Gregory Etelson
  0 siblings, 0 replies; 52+ messages in thread
From: Gregory Etelson @ 2021-10-13 17:12 UTC (permalink / raw)
  To: Ferruh Yigit, dev
  Cc: Matan Azrad, Raslan Darawsheh, olivier.matz,
	NBU-Contact-Thomas Monjalon, Bernard Iremonger, Ray Kinsella

Hello,

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

I'll update the patch in v6.

Regards,
Gregory

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

* [dpdk-dev] [PATCH v6 0/2] net: introduce IPv4 ihl and version fields
  2021-05-27 15:28 [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields Gregory Etelson
                   ` (5 preceding siblings ...)
  2021-10-12 12:29 ` [dpdk-dev] [PATCH v5] " Gregory Etelson
@ 2021-10-13 17:13 ` Gregory Etelson
  2021-10-13 17:13   ` [dpdk-dev] [PATCH v6 1/2] net: fix IPv4 change announce Gregory Etelson
                     ` (2 more replies)
  2021-10-14 17:41 ` [dpdk-dev] [PATCH v7 " Gregory Etelson
  7 siblings, 3 replies; 52+ messages in thread
From: Gregory Etelson @ 2021-10-13 17:13 UTC (permalink / raw)
  To: dev, getelson; +Cc: matan, rasland, olivier.matz, thomas, ferruh.yigit

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

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

-- 
2.33.0


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

* [dpdk-dev] [PATCH v6 1/2] net: fix IPv4 change announce
  2021-10-13 17:13 ` [dpdk-dev] [PATCH v6 0/2] " Gregory Etelson
@ 2021-10-13 17:13   ` Gregory Etelson
  2021-10-14  8:37     ` Thomas Monjalon
  2021-10-13 17:13   ` [dpdk-dev] [PATCH v6 2/2] net: introduce IPv4 ihl and version fields Gregory Etelson
  2021-10-14  8:21   ` [dpdk-dev] [PATCH v6 0/2] " Ferruh Yigit
  2 siblings, 1 reply; 52+ messages in thread
From: Gregory Etelson @ 2021-10-13 17:13 UTC (permalink / raw)
  To: dev, getelson
  Cc: matan, rasland, olivier.matz, thomas, ferruh.yigit, Ray Kinsella,
	Viacheslav Ovsiienko, Ajit Khaparde, Andrew Rybchenko,
	Akhil Goyal

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 <getelson@nvidia.com>
---
 doc/guides/rel_notes/deprecation.rst | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 309f1056cf..841653fe30 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -158,11 +158,8 @@ 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 two unions.
-  The first union is for existing ``version_ihl`` byte
-  and new bitfield for version and IHL.
-  The second union is for existing ``fragment_offset``
-  and new bitfield for fragment flags and offset.
+* 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
-- 
2.33.0


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

* [dpdk-dev] [PATCH v6 2/2] net: introduce IPv4 ihl and version fields
  2021-10-13 17:13 ` [dpdk-dev] [PATCH v6 0/2] " Gregory Etelson
  2021-10-13 17:13   ` [dpdk-dev] [PATCH v6 1/2] net: fix IPv4 change announce Gregory Etelson
@ 2021-10-13 17:13   ` Gregory Etelson
  2021-10-14 15:11     ` Ferruh Yigit
  2021-10-14  8:21   ` [dpdk-dev] [PATCH v6 0/2] " Ferruh Yigit
  2 siblings, 1 reply; 52+ messages in thread
From: Gregory Etelson @ 2021-10-13 17:13 UTC (permalink / raw)
  To: dev, getelson
  Cc: matan, rasland, olivier.matz, thomas, ferruh.yigit, Ray Kinsella,
	Bernard Iremonger

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 <getelson@nvidia.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
Acked-by: Ray Kinsella <mdr@ashroe.eu>
---
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.    
---
 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                       | 16 +++++++++++++++-
 4 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/app/test/test_flow_classify.c b/app/test/test_flow_classify.c
index 951606f248..4f64be5357 100644
--- a/app/test/test_flow_classify.c
+++ b/app/test/test_flow_classify.c
@@ -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 = {
diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 841653fe30..d1f3faac39 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -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.
diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
index e1726774ba..fa60fd9a85 100644
--- a/doc/guides/rel_notes/release_21_11.rst
+++ b/doc/guides/rel_notes/release_21_11.rst
@@ -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
 -----------
diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
index b3d45e85db..1f3660e8b4 100644
--- a/lib/net/rte_ip.h
+++ b/lib/net/rte_ip.h
@@ -39,7 +39,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;     /**< 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 */
+#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.33.0


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

* Re: [dpdk-dev] [PATCH v6 0/2] net: introduce IPv4 ihl and version fields
  2021-10-13 17:13 ` [dpdk-dev] [PATCH v6 0/2] " Gregory Etelson
  2021-10-13 17:13   ` [dpdk-dev] [PATCH v6 1/2] net: fix IPv4 change announce Gregory Etelson
  2021-10-13 17:13   ` [dpdk-dev] [PATCH v6 2/2] net: introduce IPv4 ihl and version fields Gregory Etelson
@ 2021-10-14  8:21   ` Ferruh Yigit
  2021-10-14  8:30     ` Thomas Monjalon
  2021-10-14  9:29     ` Gregory Etelson
  2 siblings, 2 replies; 52+ messages in thread
From: Ferruh Yigit @ 2021-10-14  8:21 UTC (permalink / raw)
  To: Gregory Etelson, dev; +Cc: matan, rasland, olivier.matz, thomas

On 10/13/2021 6:13 PM, Gregory Etelson wrote:
> 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

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

* Re: [dpdk-dev] [PATCH v6 0/2] net: introduce IPv4 ihl and version fields
  2021-10-14  8:21   ` [dpdk-dev] [PATCH v6 0/2] " Ferruh Yigit
@ 2021-10-14  8:30     ` Thomas Monjalon
  2021-10-14 11:01       ` Ferruh Yigit
  2021-10-14  9:29     ` Gregory Etelson
  1 sibling, 1 reply; 52+ messages in thread
From: Thomas Monjalon @ 2021-10-14  8:30 UTC (permalink / raw)
  To: Gregory Etelson, Ferruh Yigit
  Cc: dev, matan, rasland, olivier.matz, andrew.rybchenko

14/10/2021 10:21, Ferruh Yigit:
> On 10/13/2021 6:13 PM, Gregory Etelson wrote:
> > 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.

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



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

* Re: [dpdk-dev] [PATCH v6 1/2] net: fix IPv4 change announce
  2021-10-13 17:13   ` [dpdk-dev] [PATCH v6 1/2] net: fix IPv4 change announce Gregory Etelson
@ 2021-10-14  8:37     ` Thomas Monjalon
  2021-10-14 11:03       ` Ferruh Yigit
  2021-10-14 12:53       ` [dpdk-dev] [EXT] " Akhil Goyal
  0 siblings, 2 replies; 52+ messages in thread
From: Thomas Monjalon @ 2021-10-14  8:37 UTC (permalink / raw)
  To: Gregory Etelson
  Cc: dev, matan, rasland, olivier.matz, ferruh.yigit, Ray Kinsella,
	Viacheslav Ovsiienko, Ajit Khaparde, Andrew Rybchenko,
	Akhil Goyal

13/10/2021 19:13, Gregory Etelson:
> 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 <getelson@nvidia.com>

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 <thomas@monjalon.net>



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

* Re: [dpdk-dev] [PATCH v6 0/2] net: introduce IPv4 ihl and version fields
  2021-10-14  8:21   ` [dpdk-dev] [PATCH v6 0/2] " Ferruh Yigit
  2021-10-14  8:30     ` Thomas Monjalon
@ 2021-10-14  9:29     ` Gregory Etelson
  2021-10-14 11:10       ` Ferruh Yigit
  1 sibling, 1 reply; 52+ messages in thread
From: Gregory Etelson @ 2021-10-14  9:29 UTC (permalink / raw)
  To: Ferruh Yigit, dev
  Cc: Matan Azrad, Raslan Darawsheh, olivier.matz, NBU-Contact-Thomas Monjalon

Hello Ferruh,

> On 10/13/2021 6:13 PM, Gregory Etelson wrote:
> > 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?
> 

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

> 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

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

* Re: [dpdk-dev] [PATCH v6 0/2] net: introduce IPv4 ihl and version fields
  2021-10-14  8:30     ` Thomas Monjalon
@ 2021-10-14 11:01       ` Ferruh Yigit
  0 siblings, 0 replies; 52+ messages in thread
From: Ferruh Yigit @ 2021-10-14 11:01 UTC (permalink / raw)
  To: Thomas Monjalon, Gregory Etelson
  Cc: dev, matan, rasland, olivier.matz, andrew.rybchenko

On 10/14/2021 9:30 AM, Thomas Monjalon wrote:
> 14/10/2021 10:21, Ferruh Yigit:
>> On 10/13/2021 6:13 PM, Gregory Etelson wrote:
>>> 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.
> 
> It makes more sense in this order I think.
> The first patch is just dropping a deprecation note, I can ack.
> 

It will be same I think, first patch can have implementation and remove
the implemented part of the deprecation notice,
remaining deprecation notice part can be removed with or without its
implementation later.

Anyway, this is for operational needs, if the first patch gets enough
ack/review timely, not update is required and I can get both patches.

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

* Re: [dpdk-dev] [PATCH v6 1/2] net: fix IPv4 change announce
  2021-10-14  8:37     ` Thomas Monjalon
@ 2021-10-14 11:03       ` Ferruh Yigit
  2021-10-14 12:21         ` Gregory Etelson
  2021-10-14 12:53       ` [dpdk-dev] [EXT] " Akhil Goyal
  1 sibling, 1 reply; 52+ messages in thread
From: Ferruh Yigit @ 2021-10-14 11:03 UTC (permalink / raw)
  To: Thomas Monjalon, Gregory Etelson
  Cc: dev, matan, rasland, olivier.matz, Ray Kinsella,
	Viacheslav Ovsiienko, Ajit Khaparde, Andrew Rybchenko,
	Akhil Goyal

On 10/14/2021 9:37 AM, Thomas Monjalon wrote:
> 13/10/2021 19:13, Gregory Etelson:
>> 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 <getelson@nvidia.com>
> 
> 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.

> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> 
> 


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

* Re: [dpdk-dev] [PATCH v6 0/2] net: introduce IPv4 ihl and version fields
  2021-10-14  9:29     ` Gregory Etelson
@ 2021-10-14 11:10       ` Ferruh Yigit
  0 siblings, 0 replies; 52+ messages in thread
From: Ferruh Yigit @ 2021-10-14 11:10 UTC (permalink / raw)
  To: Gregory Etelson, dev
  Cc: Matan Azrad, Raslan Darawsheh, olivier.matz, NBU-Contact-Thomas Monjalon

On 10/14/2021 10:29 AM, Gregory Etelson wrote:
> Hello Ferruh,
> 
>> On 10/13/2021 6:13 PM, Gregory Etelson wrote:
>>> 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?
>>
> 
> In the existing order, the code patch reflects announced changes.
> 

Overall you are not implementing the announced changes fully, but partially.

Question is how to manage announced but not implemented part. I am for
separating discussion of that part from the code change that already acked.

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


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

* Re: [dpdk-dev] [PATCH v6 1/2] net: fix IPv4 change announce
  2021-10-14 11:03       ` Ferruh Yigit
@ 2021-10-14 12:21         ` Gregory Etelson
  2021-10-14 12:32           ` Ferruh Yigit
  0 siblings, 1 reply; 52+ messages in thread
From: Gregory Etelson @ 2021-10-14 12:21 UTC (permalink / raw)
  To: Ferruh Yigit, NBU-Contact-Thomas Monjalon
  Cc: dev, Matan Azrad, Raslan Darawsheh, olivier.matz, Ray Kinsella,
	Slava Ovsiienko, Ajit Khaparde, Andrew Rybchenko, Akhil Goyal

Hello Ferruh,

> On 10/14/2021 9:37 AM, Thomas Monjalon
> wrote:
> > 13/10/2021 19:13, Gregory Etelson:
> >> 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
> <getelson@nvidia.com>
> >
> > 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.

> > Acked-by: Thomas Monjalon
> <thomas@monjalon.net>
> >
> >


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

* Re: [dpdk-dev] [PATCH v6 1/2] net: fix IPv4 change announce
  2021-10-14 12:21         ` Gregory Etelson
@ 2021-10-14 12:32           ` Ferruh Yigit
  2021-10-14 13:34             ` Ori Kam
  0 siblings, 1 reply; 52+ messages in thread
From: Ferruh Yigit @ 2021-10-14 12:32 UTC (permalink / raw)
  To: Gregory Etelson, NBU-Contact-Thomas Monjalon, Slava Ovsiienko,
	Ajit Khaparde, Andrew Rybchenko, Akhil Goyal
  Cc: dev, Matan Azrad, Raslan Darawsheh, olivier.matz, Ray Kinsella

On 10/14/2021 1:21 PM, Gregory Etelson wrote:
> Hello Ferruh,
> 
>> On 10/14/2021 9:37 AM, Thomas Monjalon
>> wrote:
>>> 13/10/2021 19:13, Gregory Etelson:
>>>> 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
>> <getelson@nvidia.com>
>>>
>>> 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.
> 

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.

>>> Acked-by: Thomas Monjalon
>> <thomas@monjalon.net>
>>>
>>>
> 


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

* Re: [dpdk-dev] [EXT] Re: [PATCH v6 1/2] net: fix IPv4 change announce
  2021-10-14  8:37     ` Thomas Monjalon
  2021-10-14 11:03       ` Ferruh Yigit
@ 2021-10-14 12:53       ` Akhil Goyal
  1 sibling, 0 replies; 52+ messages in thread
From: Akhil Goyal @ 2021-10-14 12:53 UTC (permalink / raw)
  To: Thomas Monjalon, Gregory Etelson
  Cc: dev, matan, rasland, olivier.matz, ferruh.yigit, Ray Kinsella,
	Viacheslav Ovsiienko, Ajit Khaparde, Andrew Rybchenko

> 13/10/2021 19:13, Gregory Etelson:
> > 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 <getelson@nvidia.com>
> 
> 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 <thomas@monjalon.net>
Acked-by: Akhil Goyal <gakhil@marvell.com>



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

* Re: [dpdk-dev] [PATCH v6 1/2] net: fix IPv4 change announce
  2021-10-14 12:32           ` Ferruh Yigit
@ 2021-10-14 13:34             ` Ori Kam
  0 siblings, 0 replies; 52+ messages in thread
From: Ori Kam @ 2021-10-14 13:34 UTC (permalink / raw)
  To: Ferruh Yigit, Gregory Etelson, NBU-Contact-Thomas Monjalon,
	Slava Ovsiienko, Ajit Khaparde, Andrew Rybchenko, Akhil Goyal
  Cc: dev, Matan Azrad, Raslan Darawsheh, olivier.matz, Ray Kinsella

Hi

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> 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
> 
> On 10/14/2021 1:21 PM, Gregory Etelson wrote:
> > Hello Ferruh,
> >
> >> On 10/14/2021 9:37 AM, Thomas Monjalon
> >> wrote:
> >>> 13/10/2021 19:13, Gregory Etelson:
> >>>> 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
> >> <getelson@nvidia.com>
> >>>
> >>> 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.
> >
> 
> 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.
> 
> >>> Acked-by: Thomas Monjalon
> >> <thomas@monjalon.net>
> >>>
> >>>
> >
Acked-by: Ori Kam <orika@nvidia.com>
Best,
Ori

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

* Re: [dpdk-dev] [PATCH v6 2/2] net: introduce IPv4 ihl and version fields
  2021-10-13 17:13   ` [dpdk-dev] [PATCH v6 2/2] net: introduce IPv4 ihl and version fields Gregory Etelson
@ 2021-10-14 15:11     ` Ferruh Yigit
  2021-10-14 16:04       ` Thomas Monjalon
  0 siblings, 1 reply; 52+ messages in thread
From: Ferruh Yigit @ 2021-10-14 15:11 UTC (permalink / raw)
  To: Gregory Etelson, dev
  Cc: matan, rasland, olivier.matz, thomas, Ray Kinsella, Bernard Iremonger

On 10/13/2021 6:13 PM, Gregory Etelson wrote:
>   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 */
> +#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.

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

* Re: [dpdk-dev] [PATCH v6 2/2] net: introduce IPv4 ihl and version fields
  2021-10-14 15:11     ` Ferruh Yigit
@ 2021-10-14 16:04       ` Thomas Monjalon
  2021-10-14 17:42         ` Gregory Etelson
  0 siblings, 1 reply; 52+ messages in thread
From: Thomas Monjalon @ 2021-10-14 16:04 UTC (permalink / raw)
  To: Gregory Etelson, Ferruh Yigit
  Cc: dev, matan, rasland, olivier.matz, Ray Kinsella, Bernard Iremonger

14/10/2021 17:11, Ferruh Yigit:
> On 10/13/2021 6:13 PM, Gregory Etelson wrote:
> >   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 */
> > +#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.

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.




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

* [dpdk-dev] [PATCH v7 0/2] net: introduce IPv4 ihl and version fields
  2021-05-27 15:28 [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields Gregory Etelson
                   ` (6 preceding siblings ...)
  2021-10-13 17:13 ` [dpdk-dev] [PATCH v6 0/2] " Gregory Etelson
@ 2021-10-14 17:41 ` Gregory Etelson
  2021-10-14 17:41   ` [dpdk-dev] [PATCH v7 1/2] net: fix IPv4 change announce Gregory Etelson
                     ` (2 more replies)
  7 siblings, 3 replies; 52+ messages in thread
From: Gregory Etelson @ 2021-10-14 17:41 UTC (permalink / raw)
  To: dev, getelson; +Cc: matan, rasland, olivier.matz, thomas, ferruh.yigit

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

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

-- 
2.33.0


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

* [dpdk-dev] [PATCH v7 1/2] net: fix IPv4 change announce
  2021-10-14 17:41 ` [dpdk-dev] [PATCH v7 " Gregory Etelson
@ 2021-10-14 17:41   ` Gregory Etelson
  2021-10-14 20:00     ` Slava Ovsiienko
  2021-10-14 17:41   ` [dpdk-dev] [PATCH v7 2/2] net: introduce IPv4 ihl and version fields Gregory Etelson
  2021-10-14 19:52   ` [dpdk-dev] [PATCH v7 0/2] " Ferruh Yigit
  2 siblings, 1 reply; 52+ messages in thread
From: Gregory Etelson @ 2021-10-14 17:41 UTC (permalink / raw)
  To: dev, getelson
  Cc: matan, rasland, olivier.matz, thomas, ferruh.yigit, Akhil Goyal,
	Ori Kam, Ray Kinsella, Viacheslav Ovsiienko, Andrew Rybchenko,
	Ajit Khaparde

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 <getelson@nvidia.com>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
Acked-by: Akhil Goyal <gakhil@marvell.com>
Acked-by: Ori Kam <orika@nvidia.com>
    
---
 doc/guides/rel_notes/deprecation.rst | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 309f1056cf..841653fe30 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -158,11 +158,8 @@ 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 two unions.
-  The first union is for existing ``version_ihl`` byte
-  and new bitfield for version and IHL.
-  The second union is for existing ``fragment_offset``
-  and new bitfield for fragment flags and offset.
+* 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
-- 
2.33.0


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

* [dpdk-dev] [PATCH v7 2/2] net: introduce IPv4 ihl and version fields
  2021-10-14 17:41 ` [dpdk-dev] [PATCH v7 " Gregory Etelson
  2021-10-14 17:41   ` [dpdk-dev] [PATCH v7 1/2] net: fix IPv4 change announce Gregory Etelson
@ 2021-10-14 17:41   ` Gregory Etelson
  2021-10-14 18:16     ` Ajit Khaparde
  2021-10-14 19:52   ` [dpdk-dev] [PATCH v7 0/2] " Ferruh Yigit
  2 siblings, 1 reply; 52+ messages in thread
From: Gregory Etelson @ 2021-10-14 17:41 UTC (permalink / raw)
  To: dev, getelson
  Cc: matan, rasland, olivier.matz, thomas, ferruh.yigit, Ray Kinsella,
	Bernard Iremonger

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 <getelson@nvidia.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
Acked-by: Ray Kinsella <mdr@ashroe.eu>
---
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(-)

diff --git a/app/test/test_flow_classify.c b/app/test/test_flow_classify.c
index 951606f248..4f64be5357 100644
--- a/app/test/test_flow_classify.c
+++ b/app/test/test_flow_classify.c
@@ -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 = {
diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 841653fe30..d1f3faac39 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -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.
diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
index e1726774ba..fa60fd9a85 100644
--- a/doc/guides/rel_notes/release_21_11.rst
+++ b/doc/guides/rel_notes/release_21_11.rst
@@ -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
 -----------
diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
index b3d45e85db..0ef2430607 100644
--- a/lib/net/rte_ip.h
+++ b/lib/net/rte_ip.h
@@ -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


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

* Re: [dpdk-dev] [PATCH v6 2/2] net: introduce IPv4 ihl and version fields
  2021-10-14 16:04       ` Thomas Monjalon
@ 2021-10-14 17:42         ` Gregory Etelson
  0 siblings, 0 replies; 52+ messages in thread
From: Gregory Etelson @ 2021-10-14 17:42 UTC (permalink / raw)
  To: NBU-Contact-Thomas Monjalon, Ferruh Yigit
  Cc: dev, Matan Azrad, Raslan Darawsheh, olivier.matz, Ray Kinsella,
	Bernard Iremonger

Hello Thomas,

> > >   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 */
> > > +#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.
> 
> 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.
> 
> 

I updated the patch in v7.

Regards,
Gregory

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

* Re: [dpdk-dev] [PATCH v7 2/2] net: introduce IPv4 ihl and version fields
  2021-10-14 17:41   ` [dpdk-dev] [PATCH v7 2/2] net: introduce IPv4 ihl and version fields Gregory Etelson
@ 2021-10-14 18:16     ` Ajit Khaparde
  0 siblings, 0 replies; 52+ messages in thread
From: Ajit Khaparde @ 2021-10-14 18:16 UTC (permalink / raw)
  To: Gregory Etelson
  Cc: dpdk-dev, Matan Azrad, Raslan Darawsheh, Olivier Matz,
	Thomas Monjalon, Ferruh Yigit, Ray Kinsella, Bernard Iremonger

On Thu, Oct 14, 2021 at 10:42 AM Gregory Etelson <getelson@nvidia.com> wrote:
>
> 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 <getelson@nvidia.com>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
> Acked-by: Ray Kinsella <mdr@ashroe.eu>
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>

> ---
> 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(-)
>
> diff --git a/app/test/test_flow_classify.c b/app/test/test_flow_classify.c
> index 951606f248..4f64be5357 100644
> --- a/app/test/test_flow_classify.c
> +++ b/app/test/test_flow_classify.c
> @@ -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 = {
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 841653fe30..d1f3faac39 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -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.
> diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
> index e1726774ba..fa60fd9a85 100644
> --- a/doc/guides/rel_notes/release_21_11.rst
> +++ b/doc/guides/rel_notes/release_21_11.rst
> @@ -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
>  -----------
> diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> index b3d45e85db..0ef2430607 100644
> --- a/lib/net/rte_ip.h
> +++ b/lib/net/rte_ip.h
> @@ -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
>

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

* Re: [dpdk-dev] [PATCH v7 0/2] net: introduce IPv4 ihl and version fields
  2021-10-14 17:41 ` [dpdk-dev] [PATCH v7 " Gregory Etelson
  2021-10-14 17:41   ` [dpdk-dev] [PATCH v7 1/2] net: fix IPv4 change announce Gregory Etelson
  2021-10-14 17:41   ` [dpdk-dev] [PATCH v7 2/2] net: introduce IPv4 ihl and version fields Gregory Etelson
@ 2021-10-14 19:52   ` Ferruh Yigit
  2 siblings, 0 replies; 52+ messages in thread
From: Ferruh Yigit @ 2021-10-14 19:52 UTC (permalink / raw)
  To: Gregory Etelson, dev; +Cc: matan, rasland, olivier.matz, thomas

On 10/14/2021 6:41 PM, Gregory Etelson wrote:
> Gregory Etelson (2):
>    net: fix IPv4 change announce
>    net: introduce IPv4 ihl and version fields
> 

Series applied to dpdk-next-net/main, thanks.

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

* Re: [dpdk-dev] [PATCH v7 1/2] net: fix IPv4 change announce
  2021-10-14 17:41   ` [dpdk-dev] [PATCH v7 1/2] net: fix IPv4 change announce Gregory Etelson
@ 2021-10-14 20:00     ` Slava Ovsiienko
  2021-10-21 22:00       ` Ajit Khaparde
  0 siblings, 1 reply; 52+ messages in thread
From: Slava Ovsiienko @ 2021-10-14 20:00 UTC (permalink / raw)
  To: Gregory Etelson, dev
  Cc: Matan Azrad, Raslan Darawsheh, olivier.matz,
	NBU-Contact-Thomas Monjalon, ferruh.yigit, Akhil Goyal, Ori Kam,
	Ray Kinsella, Andrew Rybchenko, Ajit Khaparde

> -----Original Message-----
> From: Gregory Etelson <getelson@nvidia.com>
> Sent: Thursday, October 14, 2021 20:41
> To: dev@dpdk.org; Gregory Etelson <getelson@nvidia.com>
> Cc: Matan Azrad <matan@nvidia.com>; Raslan Darawsheh
> <rasland@nvidia.com>; olivier.matz@6wind.com; NBU-Contact-Thomas
> Monjalon <thomas@monjalon.net>; ferruh.yigit@intel.com; Akhil Goyal
> <gakhil@marvell.com>; Ori Kam <orika@nvidia.com>; Ray Kinsella
> <mdr@ashroe.eu>; Slava Ovsiienko <viacheslavo@nvidia.com>; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>
> 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 <getelson@nvidia.com>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> Acked-by: Akhil Goyal <gakhil@marvell.com>
> Acked-by: Ori Kam <orika@nvidia.com>
Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>

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

* Re: [dpdk-dev] [PATCH v7 1/2] net: fix IPv4 change announce
  2021-10-14 20:00     ` Slava Ovsiienko
@ 2021-10-21 22:00       ` Ajit Khaparde
  0 siblings, 0 replies; 52+ messages in thread
From: Ajit Khaparde @ 2021-10-21 22:00 UTC (permalink / raw)
  To: Slava Ovsiienko
  Cc: Gregory Etelson, dev, Matan Azrad, Raslan Darawsheh,
	olivier.matz, NBU-Contact-Thomas Monjalon, ferruh.yigit,
	Akhil Goyal, Ori Kam, Ray Kinsella, Andrew Rybchenko

On Thu, Oct 14, 2021 at 1:00 PM Slava Ovsiienko <viacheslavo@nvidia.com> wrote:
>
> > -----Original Message-----
> > From: Gregory Etelson <getelson@nvidia.com>
> > Sent: Thursday, October 14, 2021 20:41
> > To: dev@dpdk.org; Gregory Etelson <getelson@nvidia.com>
> > Cc: Matan Azrad <matan@nvidia.com>; Raslan Darawsheh
> > <rasland@nvidia.com>; olivier.matz@6wind.com; NBU-Contact-Thomas
> > Monjalon <thomas@monjalon.net>; ferruh.yigit@intel.com; Akhil Goyal
> > <gakhil@marvell.com>; Ori Kam <orika@nvidia.com>; Ray Kinsella
> > <mdr@ashroe.eu>; Slava Ovsiienko <viacheslavo@nvidia.com>; Andrew
> > Rybchenko <andrew.rybchenko@oktetlabs.ru>; Ajit Khaparde
> > <ajit.khaparde@broadcom.com>
> > 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 <getelson@nvidia.com>
> > Acked-by: Thomas Monjalon <thomas@monjalon.net>
> > Acked-by: Akhil Goyal <gakhil@marvell.com>
> > Acked-by: Ori Kam <orika@nvidia.com>
> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>

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

end of thread, other threads:[~2021-10-21 22:00 UTC | newest]

Thread overview: 52+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-27 15:28 [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields Gregory Etelson
2021-05-27 15:56 ` Morten Brørup
2021-05-28 10:20   ` Ananyev, Konstantin
2021-05-28 10:52     ` Morten Brørup
2021-05-28 14:18       ` Gregory Etelson
2021-05-31  9:58         ` Ananyev, Konstantin
2021-05-31 11:10           ` Gregory Etelson
2021-06-02  9:51             ` Gregory Etelson
2021-06-10  4:10               ` Gregory Etelson
2021-06-10  9:22                 ` Olivier Matz
2021-06-14 16:36                   ` Andrew Rybchenko
2021-06-17 16:29                     ` Ferruh Yigit
2021-06-03  0:58   ` Min Hu (Connor)
2021-06-03  2:03     ` Stephen Hemminger
2021-06-03  4:59       ` Gregory Etelson
2021-06-17 15:02 ` Tyler Retzlaff
2021-09-03  7:30 ` [dpdk-dev] [PATCH v2] " getelson
2021-10-04  7:49   ` Olivier Matz
2021-10-04  8:59     ` Gregory Etelson
2021-10-04  8:46 ` [dpdk-dev] [PATCH v3] " Gregory Etelson
2021-10-04 12:13 ` [dpdk-dev] [PATCH v4] " Gregory Etelson
2021-10-04 15:59   ` Stephen Hemminger
2021-10-04 16:48     ` Olivier Matz
2021-10-12 12:36       ` Gregory Etelson
2021-10-12 12:29 ` [dpdk-dev] [PATCH v5] " Gregory Etelson
2021-10-13  8:58   ` Kinsella, Ray
2021-10-13 12:14   ` Ferruh Yigit
2021-10-13 17:12     ` Gregory Etelson
2021-10-13 17:13 ` [dpdk-dev] [PATCH v6 0/2] " Gregory Etelson
2021-10-13 17:13   ` [dpdk-dev] [PATCH v6 1/2] net: fix IPv4 change announce Gregory Etelson
2021-10-14  8:37     ` Thomas Monjalon
2021-10-14 11:03       ` Ferruh Yigit
2021-10-14 12:21         ` Gregory Etelson
2021-10-14 12:32           ` Ferruh Yigit
2021-10-14 13:34             ` Ori Kam
2021-10-14 12:53       ` [dpdk-dev] [EXT] " Akhil Goyal
2021-10-13 17:13   ` [dpdk-dev] [PATCH v6 2/2] net: introduce IPv4 ihl and version fields Gregory Etelson
2021-10-14 15:11     ` Ferruh Yigit
2021-10-14 16:04       ` Thomas Monjalon
2021-10-14 17:42         ` Gregory Etelson
2021-10-14  8:21   ` [dpdk-dev] [PATCH v6 0/2] " Ferruh Yigit
2021-10-14  8:30     ` Thomas Monjalon
2021-10-14 11:01       ` Ferruh Yigit
2021-10-14  9:29     ` Gregory Etelson
2021-10-14 11:10       ` Ferruh Yigit
2021-10-14 17:41 ` [dpdk-dev] [PATCH v7 " Gregory Etelson
2021-10-14 17:41   ` [dpdk-dev] [PATCH v7 1/2] net: fix IPv4 change announce Gregory Etelson
2021-10-14 20:00     ` Slava Ovsiienko
2021-10-21 22:00       ` Ajit Khaparde
2021-10-14 17:41   ` [dpdk-dev] [PATCH v7 2/2] net: introduce IPv4 ihl and version fields Gregory Etelson
2021-10-14 18:16     ` Ajit Khaparde
2021-10-14 19:52   ` [dpdk-dev] [PATCH v7 0/2] " Ferruh Yigit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).