[dpdk-dev] [PATCH 1/1] ip_frag: fix creating ipv6 fragment extension header

[dpdk-dev] [PATCH 1/1] ip_frag: fix creating ipv6 fragment extension header

[Piotr]

From: Piotr Azarewicz <piotrx.t.azarewicz@intel.com>

Previous implementation won't work on every environment. The order of
allocation of bit-fields within a unit (high-order to low-order or
low-order to high-order) is implementation-defined.
Solution: used bytes instead of bit fields.

Signed-off-by: Piotr Azarewicz <piotrx.t.azarewicz@intel.com>
---
 lib/librte_ip_frag/rte_ipv6_fragmentation.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
index 0e32aa8..7342421 100644
--- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
+++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
@@ -65,10 +65,8 @@ __fill_ipv6hdr_frag(struct ipv6_hdr *dst,
 
 	fh = (struct ipv6_extension_fragment *) ++dst;
 	fh->next_header = src->proto;
-	fh->reserved1   = 0;
-	fh->frag_offset = rte_cpu_to_be_16(fofs);
-	fh->reserved2   = 0;
-	fh->more_frags  = rte_cpu_to_be_16(mf);
+	fh->reserved1 = 0;
+	fh->frag_data = rte_cpu_to_be_16((fofs & ~IPV6_HDR_FO_MASK) | mf);
 	fh->id = 0;
 }
 
-- 
1.7.9.5

Re: [dpdk-dev] [PATCH 1/1] ip_frag: fix creating ipv6 fragment extension header

[Dumitrescu, Cristian]

Konstantin,

What do you think about this issue and the patch?

Thanks,
Cristian

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Piotr
> Sent: Wednesday, September 2, 2015 3:13 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 1/1] ip_frag: fix creating ipv6 fragment
> extension header
> 
> From: Piotr Azarewicz <piotrx.t.azarewicz@intel.com>
> 
> Previous implementation won't work on every environment. The order of
> allocation of bit-fields within a unit (high-order to low-order or
> low-order to high-order) is implementation-defined.
> Solution: used bytes instead of bit fields.
> 
> Signed-off-by: Piotr Azarewicz <piotrx.t.azarewicz@intel.com>
> ---
>  lib/librte_ip_frag/rte_ipv6_fragmentation.c |    6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> index 0e32aa8..7342421 100644
> --- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> +++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> @@ -65,10 +65,8 @@ __fill_ipv6hdr_frag(struct ipv6_hdr *dst,
> 
>  	fh = (struct ipv6_extension_fragment *) ++dst;
>  	fh->next_header = src->proto;
> -	fh->reserved1   = 0;
> -	fh->frag_offset = rte_cpu_to_be_16(fofs);
> -	fh->reserved2   = 0;
> -	fh->more_frags  = rte_cpu_to_be_16(mf);
> +	fh->reserved1 = 0;
> +	fh->frag_data = rte_cpu_to_be_16((fofs & ~IPV6_HDR_FO_MASK) |
> mf);
>  	fh->id = 0;
>  }
> 
> --
> 1.7.9.5

Re: [dpdk-dev] [PATCH 1/1] ip_frag: fix creating ipv6 fragment extension header

[Ananyev, Konstantin]

Hi Piotr,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Piotr
> Sent: Wednesday, September 02, 2015 3:13 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 1/1] ip_frag: fix creating ipv6 fragment extension header
> 
> From: Piotr Azarewicz <piotrx.t.azarewicz@intel.com>
> 
> Previous implementation won't work on every environment. The order of
> allocation of bit-fields within a unit (high-order to low-order or
> low-order to high-order) is implementation-defined.
> Solution: used bytes instead of bit fields.

Seems like right thing to do to me.
Though I think we also should replace:
union {
                struct {
                        uint16_t frag_offset:13; /**< Offset from the start of the packet */
                        uint16_t reserved2:2; /**< Reserved */
                        uint16_t more_frags:1;
                        /**< 1 if more fragments left, 0 if last fragment */
                };
                uint16_t frag_data;
                /**< union of all fragmentation data */
        }; 

With just: 
uint16_t frag_data;
 and probably provide macros to read/set fragment_offset and more_flags values.
Otherwise people might keep using the wrong layout.
Konstantin

> 
> Signed-off-by: Piotr Azarewicz <piotrx.t.azarewicz@intel.com>
> ---
>  lib/librte_ip_frag/rte_ipv6_fragmentation.c |    6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> index 0e32aa8..7342421 100644
> --- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> +++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> @@ -65,10 +65,8 @@ __fill_ipv6hdr_frag(struct ipv6_hdr *dst,
> 
>  	fh = (struct ipv6_extension_fragment *) ++dst;
>  	fh->next_header = src->proto;
> -	fh->reserved1   = 0;
> -	fh->frag_offset = rte_cpu_to_be_16(fofs);
> -	fh->reserved2   = 0;
> -	fh->more_frags  = rte_cpu_to_be_16(mf);
> +	fh->reserved1 = 0;
> +	fh->frag_data = rte_cpu_to_be_16((fofs & ~IPV6_HDR_FO_MASK) | mf);
>  	fh->id = 0;
>  }
> 
> --
> 1.7.9.5

Re: [dpdk-dev] [PATCH 1/1] ip_frag: fix creating ipv6 fragment extension header

[Dumitrescu, Cristian]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> Konstantin
> Sent: Friday, September 4, 2015 6:51 PM
> To: Azarewicz, PiotrX T; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 1/1] ip_frag: fix creating ipv6 fragment
> extension header
> 
> Hi Piotr,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Piotr
> > Sent: Wednesday, September 02, 2015 3:13 PM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH 1/1] ip_frag: fix creating ipv6 fragment
> extension header
> >
> > From: Piotr Azarewicz <piotrx.t.azarewicz@intel.com>
> >
> > Previous implementation won't work on every environment. The order of
> > allocation of bit-fields within a unit (high-order to low-order or
> > low-order to high-order) is implementation-defined.
> > Solution: used bytes instead of bit fields.
> 
> Seems like right thing to do to me.
> Though I think we also should replace:
> union {
>                 struct {
>                         uint16_t frag_offset:13; /**< Offset from the start of the packet
> */
>                         uint16_t reserved2:2; /**< Reserved */
>                         uint16_t more_frags:1;
>                         /**< 1 if more fragments left, 0 if last fragment */
>                 };
>                 uint16_t frag_data;
>                 /**< union of all fragmentation data */
>         };
> 
> With just:
> uint16_t frag_data;
>  and probably provide macros to read/set fragment_offset and more_flags
> values.
> Otherwise people might keep using the wrong layout.
> Konstantin
> 

I agree with your proposal, but wouldn't this be an ABI change? To avoid an ABI change, we should probably leave the union?

> >
> > Signed-off-by: Piotr Azarewicz <piotrx.t.azarewicz@intel.com>
> > ---
> >  lib/librte_ip_frag/rte_ipv6_fragmentation.c |    6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> > index 0e32aa8..7342421 100644
> > --- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> > +++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> > @@ -65,10 +65,8 @@ __fill_ipv6hdr_frag(struct ipv6_hdr *dst,
> >
> >  	fh = (struct ipv6_extension_fragment *) ++dst;
> >  	fh->next_header = src->proto;
> > -	fh->reserved1   = 0;
> > -	fh->frag_offset = rte_cpu_to_be_16(fofs);
> > -	fh->reserved2   = 0;
> > -	fh->more_frags  = rte_cpu_to_be_16(mf);
> > +	fh->reserved1 = 0;
> > +	fh->frag_data = rte_cpu_to_be_16((fofs & ~IPV6_HDR_FO_MASK) |
> mf);
> >  	fh->id = 0;
> >  }
> >
> > --
> > 1.7.9.5

Re: [dpdk-dev] [PATCH 1/1] ip_frag: fix creating ipv6 fragment extension header

[Ananyev, Konstantin]

> -----Original Message-----
> From: Dumitrescu, Cristian
> Sent: Monday, September 07, 2015 12:22 PM
> To: Ananyev, Konstantin; Azarewicz, PiotrX T; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH 1/1] ip_frag: fix creating ipv6 fragment extension header
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> > Konstantin
> > Sent: Friday, September 4, 2015 6:51 PM
> > To: Azarewicz, PiotrX T; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 1/1] ip_frag: fix creating ipv6 fragment
> > extension header
> >
> > Hi Piotr,
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Piotr
> > > Sent: Wednesday, September 02, 2015 3:13 PM
> > > To: dev@dpdk.org
> > > Subject: [dpdk-dev] [PATCH 1/1] ip_frag: fix creating ipv6 fragment
> > extension header
> > >
> > > From: Piotr Azarewicz <piotrx.t.azarewicz@intel.com>
> > >
> > > Previous implementation won't work on every environment. The order of
> > > allocation of bit-fields within a unit (high-order to low-order or
> > > low-order to high-order) is implementation-defined.
> > > Solution: used bytes instead of bit fields.
> >
> > Seems like right thing to do to me.
> > Though I think we also should replace:
> > union {
> >                 struct {
> >                         uint16_t frag_offset:13; /**< Offset from the start of the packet
> > */
> >                         uint16_t reserved2:2; /**< Reserved */
> >                         uint16_t more_frags:1;
> >                         /**< 1 if more fragments left, 0 if last fragment */
> >                 };
> >                 uint16_t frag_data;
> >                 /**< union of all fragmentation data */
> >         };
> >
> > With just:
> > uint16_t frag_data;
> >  and probably provide macros to read/set fragment_offset and more_flags
> > values.
> > Otherwise people might keep using the wrong layout.
> > Konstantin
> >
> 
> I agree with your proposal, but wouldn't this be an ABI change? To avoid an ABI change, we should probably leave the union?


No I don't think it would - the size of the field will remain the same: uint16_t.
Also if the bit-field is invalid what for to keep it?
Konstantin

> 
> > >
> > > Signed-off-by: Piotr Azarewicz <piotrx.t.azarewicz@intel.com>
> > > ---
> > >  lib/librte_ip_frag/rte_ipv6_fragmentation.c |    6 ++----
> > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> > b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> > > index 0e32aa8..7342421 100644
> > > --- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> > > +++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> > > @@ -65,10 +65,8 @@ __fill_ipv6hdr_frag(struct ipv6_hdr *dst,
> > >
> > >  	fh = (struct ipv6_extension_fragment *) ++dst;
> > >  	fh->next_header = src->proto;
> > > -	fh->reserved1   = 0;
> > > -	fh->frag_offset = rte_cpu_to_be_16(fofs);
> > > -	fh->reserved2   = 0;
> > > -	fh->more_frags  = rte_cpu_to_be_16(mf);
> > > +	fh->reserved1 = 0;
> > > +	fh->frag_data = rte_cpu_to_be_16((fofs & ~IPV6_HDR_FO_MASK) |
> > mf);
> > >  	fh->id = 0;
> > >  }
> > >
> > > --
> > > 1.7.9.5

Re: [dpdk-dev] [PATCH 1/1] ip_frag: fix creating ipv6 fragment extension header

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Monday, September 7, 2015 2:23 PM
> To: Dumitrescu, Cristian; Azarewicz, PiotrX T; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH 1/1] ip_frag: fix creating ipv6 fragment
> extension header
> 
> 
> 
> > -----Original Message-----
> > From: Dumitrescu, Cristian
> > Sent: Monday, September 07, 2015 12:22 PM
> > To: Ananyev, Konstantin; Azarewicz, PiotrX T; dev@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH 1/1] ip_frag: fix creating ipv6 fragment
> extension header
> >
> >
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> > > Konstantin
> > > Sent: Friday, September 4, 2015 6:51 PM
> > > To: Azarewicz, PiotrX T; dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH 1/1] ip_frag: fix creating ipv6 fragment
> > > extension header
> > >
> > > Hi Piotr,
> > >
> > > > -----Original Message-----
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Piotr
> > > > Sent: Wednesday, September 02, 2015 3:13 PM
> > > > To: dev@dpdk.org
> > > > Subject: [dpdk-dev] [PATCH 1/1] ip_frag: fix creating ipv6 fragment
> > > extension header
> > > >
> > > > From: Piotr Azarewicz <piotrx.t.azarewicz@intel.com>
> > > >
> > > > Previous implementation won't work on every environment. The order
> of
> > > > allocation of bit-fields within a unit (high-order to low-order or
> > > > low-order to high-order) is implementation-defined.
> > > > Solution: used bytes instead of bit fields.
> > >
> > > Seems like right thing to do to me.
> > > Though I think we also should replace:
> > > union {
> > >                 struct {
> > >                         uint16_t frag_offset:13; /**< Offset from the start of the
> packet
> > > */
> > >                         uint16_t reserved2:2; /**< Reserved */
> > >                         uint16_t more_frags:1;
> > >                         /**< 1 if more fragments left, 0 if last fragment */
> > >                 };
> > >                 uint16_t frag_data;
> > >                 /**< union of all fragmentation data */
> > >         };
> > >
> > > With just:
> > > uint16_t frag_data;
> > >  and probably provide macros to read/set fragment_offset and
> more_flags
> > > values.
> > > Otherwise people might keep using the wrong layout.
> > > Konstantin
> > >
> >
> > I agree with your proposal, but wouldn't this be an ABI change? To avoid an
> ABI change, we should probably leave the union?
> 
> 
> No I don't think it would - the size of the field will remain the same: uint16_t.
> Also if the bit-field is invalid what for to keep it?
> Konstantin
> 

Excellent then.

> >
> > > >
> > > > Signed-off-by: Piotr Azarewicz <piotrx.t.azarewicz@intel.com>
> > > > ---
> > > >  lib/librte_ip_frag/rte_ipv6_fragmentation.c |    6 ++----
> > > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> > > b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> > > > index 0e32aa8..7342421 100644
> > > > --- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> > > > +++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> > > > @@ -65,10 +65,8 @@ __fill_ipv6hdr_frag(struct ipv6_hdr *dst,
> > > >
> > > >  	fh = (struct ipv6_extension_fragment *) ++dst;
> > > >  	fh->next_header = src->proto;
> > > > -	fh->reserved1   = 0;
> > > > -	fh->frag_offset = rte_cpu_to_be_16(fofs);
> > > > -	fh->reserved2   = 0;
> > > > -	fh->more_frags  = rte_cpu_to_be_16(mf);
> > > > +	fh->reserved1 = 0;
> > > > +	fh->frag_data = rte_cpu_to_be_16((fofs & ~IPV6_HDR_FO_MASK) |
> > > mf);
> > > >  	fh->id = 0;
> > > >  }
> > > >
> > > > --
> > > > 1.7.9.5

[dpdk-dev] [PATCH v2 1/1] ip_frag: fix creating ipv6 fragment extension header

[Piotr Azarewicz]

Previous implementation won't work on every environment. The order of
allocation of bit-fields within a unit (high-order to low-order or
low-order to high-order) is implementation-defined.
Solution: used bytes instead of bit fields.

v2 changes:
- remove useless union
- fix process_ipv6 function (due to remove the union above)

Signed-off-by: Piotr Azarewicz <piotrx.t.azarewicz@intel.com>
---
 lib/librte_ip_frag/rte_ip_frag.h            |   13 ++-----------
 lib/librte_ip_frag/rte_ipv6_fragmentation.c |    6 ++----
 lib/librte_port/rte_port_ras.c              |   10 +++++++---
 3 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/lib/librte_ip_frag/rte_ip_frag.h b/lib/librte_ip_frag/rte_ip_frag.h
index 52f44c9..f3ca566 100644
--- a/lib/librte_ip_frag/rte_ip_frag.h
+++ b/lib/librte_ip_frag/rte_ip_frag.h
@@ -130,17 +130,8 @@ struct rte_ip_frag_tbl {
 /** IPv6 fragment extension header */
 struct ipv6_extension_fragment {
 	uint8_t next_header;            /**< Next header type */
-	uint8_t reserved1;              /**< Reserved */
-	union {
-		struct {
-			uint16_t frag_offset:13; /**< Offset from the start of the packet */
-			uint16_t reserved2:2; /**< Reserved */
-			uint16_t more_frags:1;
-			/**< 1 if more fragments left, 0 if last fragment */
-		};
-		uint16_t frag_data;
-		/**< union of all fragmentation data */
-	};
+	uint8_t reserved;               /**< Reserved */
+	uint16_t frag_data;             /**< All fragmentation data */
 	uint32_t id;                    /**< Packet ID */
 } __attribute__((__packed__));
 
diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
index 0e32aa8..ab62efd 100644
--- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
+++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
@@ -65,10 +65,8 @@ __fill_ipv6hdr_frag(struct ipv6_hdr *dst,
 
 	fh = (struct ipv6_extension_fragment *) ++dst;
 	fh->next_header = src->proto;
-	fh->reserved1   = 0;
-	fh->frag_offset = rte_cpu_to_be_16(fofs);
-	fh->reserved2   = 0;
-	fh->more_frags  = rte_cpu_to_be_16(mf);
+	fh->reserved = 0;
+	fh->frag_data = rte_cpu_to_be_16((fofs & ~IPV6_HDR_FO_MASK) | mf);
 	fh->id = 0;
 }
 
diff --git a/lib/librte_port/rte_port_ras.c b/lib/librte_port/rte_port_ras.c
index 6bd0f8c..3dbd5be 100644
--- a/lib/librte_port/rte_port_ras.c
+++ b/lib/librte_port/rte_port_ras.c
@@ -205,6 +205,9 @@ process_ipv4(struct rte_port_ring_writer_ras *p, struct rte_mbuf *pkt)
 	}
 }
 
+#define MORE_FRAGS(x) ((x) & 0x0001)
+#define FRAG_OFFSET(x) ((x) >> 3)
+
 static void
 process_ipv6(struct rte_port_ring_writer_ras *p, struct rte_mbuf *pkt)
 {
@@ -212,12 +215,13 @@ process_ipv6(struct rte_port_ring_writer_ras *p, struct rte_mbuf *pkt)
 	struct ipv6_hdr *pkt_hdr = rte_pktmbuf_mtod(pkt, struct ipv6_hdr *);
 
 	struct ipv6_extension_fragment *frag_hdr;
+	uint16_t frag_data = 0;
 	frag_hdr = rte_ipv6_frag_get_ipv6_fragment_header(pkt_hdr);
-	uint16_t frag_offset = frag_hdr->frag_offset;
-	uint16_t frag_flag = frag_hdr->more_frags;
+	if (frag_hdr != NULL)
+		frag_data = rte_be_to_cpu_16(frag_hdr->frag_data);
 
 	/* If it is a fragmented packet, then try to reassemble */
-	if ((frag_flag == 0) && (frag_offset == 0))
+	if ((MORE_FRAGS(frag_data) == 0) && (FRAG_OFFSET(frag_data) == 0))
 		p->tx_buf[p->tx_buf_count++] = pkt;
 	else {
 		struct rte_mbuf *mo;
-- 
1.7.9.5

Re: [dpdk-dev] [PATCH v2 1/1] ip_frag: fix creating ipv6 fragment extension header

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Azarewicz, PiotrX T
> Sent: Tuesday, September 8, 2015 5:08 PM
> To: dev@dpdk.org
> Cc: Dumitrescu, Cristian; Ananyev, Konstantin; Azarewicz, PiotrX T
> Subject: [PATCH v2 1/1] ip_frag: fix creating ipv6 fragment extension header
> 
> Previous implementation won't work on every environment. The order of
> allocation of bit-fields within a unit (high-order to low-order or
> low-order to high-order) is implementation-defined.
> Solution: used bytes instead of bit fields.
> 
> v2 changes:
> - remove useless union
> - fix process_ipv6 function (due to remove the union above)
> 
> Signed-off-by: Piotr Azarewicz <piotrx.t.azarewicz@intel.com>
> ---

Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

[dpdk-dev] FW: [PATCH v2 1/1] ip_frag: fix creating ipv6 fragment extension header

[Ananyev, Konstantin]

Hi Piotr,

> -----Original Message-----
> From: Azarewicz, PiotrX T
> Sent: Tuesday, September 08, 2015 3:08 PM
> To: dev@dpdk.org
> Cc: Dumitrescu, Cristian; Ananyev, Konstantin; Azarewicz, PiotrX T
> Subject: [PATCH v2 1/1] ip_frag: fix creating ipv6 fragment extension header
> 
> Previous implementation won't work on every environment. The order of
> allocation of bit-fields within a unit (high-order to low-order or
> low-order to high-order) is implementation-defined.
> Solution: used bytes instead of bit fields.
> 
> v2 changes:
> - remove useless union
> - fix process_ipv6 function (due to remove the union above)
> 
> Signed-off-by: Piotr Azarewicz <piotrx.t.azarewicz@intel.com>
> ---
>  lib/librte_ip_frag/rte_ip_frag.h            |   13 ++-----------
>  lib/librte_ip_frag/rte_ipv6_fragmentation.c |    6 ++----
>  lib/librte_port/rte_port_ras.c              |   10 +++++++---
>  3 files changed, 11 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/librte_ip_frag/rte_ip_frag.h b/lib/librte_ip_frag/rte_ip_frag.h
> index 52f44c9..f3ca566 100644
> --- a/lib/librte_ip_frag/rte_ip_frag.h
> +++ b/lib/librte_ip_frag/rte_ip_frag.h
> @@ -130,17 +130,8 @@ struct rte_ip_frag_tbl {
>  /** IPv6 fragment extension header */
>  struct ipv6_extension_fragment {
>  	uint8_t next_header;            /**< Next header type */
> -	uint8_t reserved1;              /**< Reserved */
> -	union {
> -		struct {
> -			uint16_t frag_offset:13; /**< Offset from the start of the packet */
> -			uint16_t reserved2:2; /**< Reserved */
> -			uint16_t more_frags:1;
> -			/**< 1 if more fragments left, 0 if last fragment */
> -		};
> -		uint16_t frag_data;
> -		/**< union of all fragmentation data */
> -	};
> +	uint8_t reserved;               /**< Reserved */
> +	uint16_t frag_data;             /**< All fragmentation data */
>  	uint32_t id;                    /**< Packet ID */
>  } __attribute__((__packed__));
> 
> diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> index 0e32aa8..ab62efd 100644
> --- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> +++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
> @@ -65,10 +65,8 @@ __fill_ipv6hdr_frag(struct ipv6_hdr *dst,
> 
>  	fh = (struct ipv6_extension_fragment *) ++dst;
>  	fh->next_header = src->proto;
> -	fh->reserved1   = 0;
> -	fh->frag_offset = rte_cpu_to_be_16(fofs);
> -	fh->reserved2   = 0;
> -	fh->more_frags  = rte_cpu_to_be_16(mf);
> +	fh->reserved = 0;
> +	fh->frag_data = rte_cpu_to_be_16((fofs & ~IPV6_HDR_FO_MASK) | mf);
>  	fh->id = 0;
>  }
> 
> diff --git a/lib/librte_port/rte_port_ras.c b/lib/librte_port/rte_port_ras.c
> index 6bd0f8c..3dbd5be 100644
> --- a/lib/librte_port/rte_port_ras.c
> +++ b/lib/librte_port/rte_port_ras.c
> @@ -205,6 +205,9 @@ process_ipv4(struct rte_port_ring_writer_ras *p, struct rte_mbuf *pkt)
>  	}
>  }
> 
> +#define MORE_FRAGS(x) ((x) & 0x0001)
> +#define FRAG_OFFSET(x) ((x) >> 3)
> +
>  static void
>  process_ipv6(struct rte_port_ring_writer_ras *p, struct rte_mbuf *pkt)
>  {
> @@ -212,12 +215,13 @@ process_ipv6(struct rte_port_ring_writer_ras *p, struct rte_mbuf *pkt)
>  	struct ipv6_hdr *pkt_hdr = rte_pktmbuf_mtod(pkt, struct ipv6_hdr *);
> 
>  	struct ipv6_extension_fragment *frag_hdr;
> +	uint16_t frag_data = 0;
>  	frag_hdr = rte_ipv6_frag_get_ipv6_fragment_header(pkt_hdr);
> -	uint16_t frag_offset = frag_hdr->frag_offset;
> -	uint16_t frag_flag = frag_hdr->more_frags;
> +	if (frag_hdr != NULL)
> +		frag_data = rte_be_to_cpu_16(frag_hdr->frag_data);
> 
>  	/* If it is a fragmented packet, then try to reassemble */
> -	if ((frag_flag == 0) && (frag_offset == 0))
> +	if ((MORE_FRAGS(frag_data) == 0) && (FRAG_OFFSET(frag_data) == 0))
>  		p->tx_buf[p->tx_buf_count++] = pkt;
>  	else {
>  		struct rte_mbuf *mo;
> --
> 1.7.9.5

When I wrote " provide macros to read/set fragment_offset and more_flags values",
I meant move IPV6_HDR_*macros that are useful from lib/librte_ip_frag/rte_ipv6_fragmentation.c
into rte_ip_frag.h and add new one based on them.
Obviously it seems strange to define some macros in .c file, never use most of them,
and in another .c file use hardcoded values instead of them again.

Something like:

#define RTE_IPV6_HDR_MF_MASK                     1
#define RTE_IPV6_HDR_FO_SHIFT                       3
#define RTE_IPV6_HDR_FO_MASK                       ((1 << IPV6_HDR_FO_SHIFT) - 1))

#define RTE_IPV6_GET_MF(x)                 ((x) & RTE_IPV6_HDR_MF_MASK)
#define RTE_IPV6_GET_FO(x)		((x) >> RTE_IPV6_HDR_FO_SHIFT)

#define RTE_IPV6_FRAG_DATA(fo, mf)	(((fo) & ~RTE_IPV6_HDR_FO_MASK)) | ((mf) & RTE_IPV6_HDR_MF_MASK))

And then use these macros in both .c files.

Actually another note:
+	if ((MORE_FRAGS(frag_data) == 0) && (FRAG_OFFSET(frag_data) == 0))

Why do you need && here?
Why just not:

#define RTE_IPV6_FRAG_USED_MASK		(RTE_IPV6_HDR_MF_MASK | ~RTE_IPV6_HDR_FO_MASK)
...
if ((frag_data  & RTE_IPV6_FRAG_USED_MASK) == 0)
?

Thanks
Konstantin

[dpdk-dev] [PATCH v3 1/1] ip_frag: fix creating ipv6 fragment extension header

[Piotr Azarewicz]

Previous implementation won't work on every environment. The order of
allocation of bit-fields within a unit (high-order to low-order or
low-order to high-order) is implementation-defined.
Solution: used bytes instead of bit fields.

v2 changes:
- remove useless union
- fix process_ipv6 function (due to remove the union above)

v3 changes:
- add macros to read/set fragment_offset and more_flags values

Signed-off-by: Piotr Azarewicz <piotrx.t.azarewicz@intel.com>
---
 lib/librte_ip_frag/rte_ip_frag.h            |   27 ++++++++++++++++-----------
 lib/librte_ip_frag/rte_ipv6_fragmentation.c |   12 ++----------
 lib/librte_port/rte_port_ras.c              |    7 ++++---
 3 files changed, 22 insertions(+), 24 deletions(-)

diff --git a/lib/librte_ip_frag/rte_ip_frag.h b/lib/librte_ip_frag/rte_ip_frag.h
index 52f44c9..92cedf2 100644
--- a/lib/librte_ip_frag/rte_ip_frag.h
+++ b/lib/librte_ip_frag/rte_ip_frag.h
@@ -128,19 +128,24 @@ struct rte_ip_frag_tbl {
 };
 
 /** IPv6 fragment extension header */
+#define	RTE_IPV6_EHDR_MF_SHIFT			0
+#define	RTE_IPV6_EHDR_MF_MASK			1
+#define	RTE_IPV6_EHDR_FO_SHIFT			3
+#define	RTE_IPV6_EHDR_FO_MASK			(~((1 << RTE_IPV6_EHDR_FO_SHIFT) - 1))
+
+#define RTE_IPV6_FRAG_USED_MASK			\
+	(RTE_IPV6_EHDR_MF_MASK | RTE_IPV6_EHDR_FO_MASK)
+
+#define RTE_IPV6_GET_MF(x)				((x) & RTE_IPV6_EHDR_MF_MASK)
+#define RTE_IPV6_GET_FO(x)				((x) >> RTE_IPV6_EHDR_FO_SHIFT)
+
+#define RTE_IPV6_SET_FRAG_DATA(fo, mf)	\
+	(((fo) & RTE_IPV6_EHDR_FO_MASK) | ((mf) & RTE_IPV6_EHDR_MF_MASK))
+
 struct ipv6_extension_fragment {
 	uint8_t next_header;            /**< Next header type */
-	uint8_t reserved1;              /**< Reserved */
-	union {
-		struct {
-			uint16_t frag_offset:13; /**< Offset from the start of the packet */
-			uint16_t reserved2:2; /**< Reserved */
-			uint16_t more_frags:1;
-			/**< 1 if more fragments left, 0 if last fragment */
-		};
-		uint16_t frag_data;
-		/**< union of all fragmentation data */
-	};
+	uint8_t reserved;               /**< Reserved */
+	uint16_t frag_data;             /**< All fragmentation data */
 	uint32_t id;                    /**< Packet ID */
 } __attribute__((__packed__));
 
diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
index 0e32aa8..251a4b8 100644
--- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
+++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
@@ -46,12 +46,6 @@
  *
  */
 
-/* Fragment Extension Header */
-#define	IPV6_HDR_MF_SHIFT			0
-#define	IPV6_HDR_FO_SHIFT			3
-#define	IPV6_HDR_MF_MASK			(1 << IPV6_HDR_MF_SHIFT)
-#define	IPV6_HDR_FO_MASK			((1 << IPV6_HDR_FO_SHIFT) - 1)
-
 static inline void
 __fill_ipv6hdr_frag(struct ipv6_hdr *dst,
 		const struct ipv6_hdr *src, uint16_t len, uint16_t fofs,
@@ -65,10 +59,8 @@ __fill_ipv6hdr_frag(struct ipv6_hdr *dst,
 
 	fh = (struct ipv6_extension_fragment *) ++dst;
 	fh->next_header = src->proto;
-	fh->reserved1   = 0;
-	fh->frag_offset = rte_cpu_to_be_16(fofs);
-	fh->reserved2   = 0;
-	fh->more_frags  = rte_cpu_to_be_16(mf);
+	fh->reserved = 0;
+	fh->frag_data = rte_cpu_to_be_16(RTE_IPV6_SET_FRAG_DATA(fofs, mf));
 	fh->id = 0;
 }
 
diff --git a/lib/librte_port/rte_port_ras.c b/lib/librte_port/rte_port_ras.c
index 6bd0f8c..8a2e554 100644
--- a/lib/librte_port/rte_port_ras.c
+++ b/lib/librte_port/rte_port_ras.c
@@ -212,12 +212,13 @@ process_ipv6(struct rte_port_ring_writer_ras *p, struct rte_mbuf *pkt)
 	struct ipv6_hdr *pkt_hdr = rte_pktmbuf_mtod(pkt, struct ipv6_hdr *);
 
 	struct ipv6_extension_fragment *frag_hdr;
+	uint16_t frag_data = 0;
 	frag_hdr = rte_ipv6_frag_get_ipv6_fragment_header(pkt_hdr);
-	uint16_t frag_offset = frag_hdr->frag_offset;
-	uint16_t frag_flag = frag_hdr->more_frags;
+	if (frag_hdr != NULL)
+		frag_data = rte_be_to_cpu_16(frag_hdr->frag_data);
 
 	/* If it is a fragmented packet, then try to reassemble */
-	if ((frag_flag == 0) && (frag_offset == 0))
+	if ((frag_data & RTE_IPV6_FRAG_USED_MASK) == 0)
 		p->tx_buf[p->tx_buf_count++] = pkt;
 	else {
 		struct rte_mbuf *mo;
-- 
1.7.9.5

[dpdk-dev] [PATCH v4 1/1] ip_frag: fix creating ipv6 fragment extension header

[Piotr Azarewicz]

Previous implementation won't work on every environment. The order of
allocation of bit-fields within a unit (high-order to low-order or
low-order to high-order) is implementation-defined.
Solution: used bytes instead of bit fields.

v2 changes:
- remove useless union
- fix process_ipv6 function (due to remove the union above)

v3 changes:
- add macros to read/set fragment_offset and more_flags values

v4 changes:
- two additional fixes due to remove the union and due to changes in
macros

Signed-off-by: Piotr Azarewicz <piotrx.t.azarewicz@intel.com>
---
 lib/librte_ip_frag/rte_ip_frag.h            |   27 ++++++++++++++++-----------
 lib/librte_ip_frag/rte_ipv6_fragmentation.c |   14 +++-----------
 lib/librte_ip_frag/rte_ipv6_reassembly.c    |    3 ++-
 lib/librte_port/rte_port_ras.c              |    7 ++++---
 4 files changed, 25 insertions(+), 26 deletions(-)

diff --git a/lib/librte_ip_frag/rte_ip_frag.h b/lib/librte_ip_frag/rte_ip_frag.h
index 52f44c9..92cedf2 100644
--- a/lib/librte_ip_frag/rte_ip_frag.h
+++ b/lib/librte_ip_frag/rte_ip_frag.h
@@ -128,19 +128,24 @@ struct rte_ip_frag_tbl {
 };
 
 /** IPv6 fragment extension header */
+#define	RTE_IPV6_EHDR_MF_SHIFT			0
+#define	RTE_IPV6_EHDR_MF_MASK			1
+#define	RTE_IPV6_EHDR_FO_SHIFT			3
+#define	RTE_IPV6_EHDR_FO_MASK			(~((1 << RTE_IPV6_EHDR_FO_SHIFT) - 1))
+
+#define RTE_IPV6_FRAG_USED_MASK			\
+	(RTE_IPV6_EHDR_MF_MASK | RTE_IPV6_EHDR_FO_MASK)
+
+#define RTE_IPV6_GET_MF(x)				((x) & RTE_IPV6_EHDR_MF_MASK)
+#define RTE_IPV6_GET_FO(x)				((x) >> RTE_IPV6_EHDR_FO_SHIFT)
+
+#define RTE_IPV6_SET_FRAG_DATA(fo, mf)	\
+	(((fo) & RTE_IPV6_EHDR_FO_MASK) | ((mf) & RTE_IPV6_EHDR_MF_MASK))
+
 struct ipv6_extension_fragment {
 	uint8_t next_header;            /**< Next header type */
-	uint8_t reserved1;              /**< Reserved */
-	union {
-		struct {
-			uint16_t frag_offset:13; /**< Offset from the start of the packet */
-			uint16_t reserved2:2; /**< Reserved */
-			uint16_t more_frags:1;
-			/**< 1 if more fragments left, 0 if last fragment */
-		};
-		uint16_t frag_data;
-		/**< union of all fragmentation data */
-	};
+	uint8_t reserved;               /**< Reserved */
+	uint16_t frag_data;             /**< All fragmentation data */
 	uint32_t id;                    /**< Packet ID */
 } __attribute__((__packed__));
 
diff --git a/lib/librte_ip_frag/rte_ipv6_fragmentation.c b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
index 0e32aa8..1e30004 100644
--- a/lib/librte_ip_frag/rte_ipv6_fragmentation.c
+++ b/lib/librte_ip_frag/rte_ipv6_fragmentation.c
@@ -46,12 +46,6 @@
  *
  */
 
-/* Fragment Extension Header */
-#define	IPV6_HDR_MF_SHIFT			0
-#define	IPV6_HDR_FO_SHIFT			3
-#define	IPV6_HDR_MF_MASK			(1 << IPV6_HDR_MF_SHIFT)
-#define	IPV6_HDR_FO_MASK			((1 << IPV6_HDR_FO_SHIFT) - 1)
-
 static inline void
 __fill_ipv6hdr_frag(struct ipv6_hdr *dst,
 		const struct ipv6_hdr *src, uint16_t len, uint16_t fofs,
@@ -65,10 +59,8 @@ __fill_ipv6hdr_frag(struct ipv6_hdr *dst,
 
 	fh = (struct ipv6_extension_fragment *) ++dst;
 	fh->next_header = src->proto;
-	fh->reserved1   = 0;
-	fh->frag_offset = rte_cpu_to_be_16(fofs);
-	fh->reserved2   = 0;
-	fh->more_frags  = rte_cpu_to_be_16(mf);
+	fh->reserved = 0;
+	fh->frag_data = rte_cpu_to_be_16(RTE_IPV6_SET_FRAG_DATA(fofs, mf));
 	fh->id = 0;
 }
 
@@ -118,7 +110,7 @@ rte_ipv6_fragment_packet(struct rte_mbuf *pkt_in,
 	frag_size = (uint16_t)(mtu_size - sizeof(struct ipv6_hdr));
 
 	/* Fragment size should be a multiple of 8. */
-	IP_FRAG_ASSERT((frag_size & IPV6_HDR_FO_MASK) == 0);
+	IP_FRAG_ASSERT((frag_size & ~RTE_IPV6_EHDR_FO_MASK) == 0);
 
 	/* Check that pkts_out is big enough to hold all fragments */
 	if (unlikely (frag_size * nb_pkts_out <
diff --git a/lib/librte_ip_frag/rte_ipv6_reassembly.c b/lib/librte_ip_frag/rte_ipv6_reassembly.c
index 1f1c172..8b4ef8a 100644
--- a/lib/librte_ip_frag/rte_ipv6_reassembly.c
+++ b/lib/librte_ip_frag/rte_ipv6_reassembly.c
@@ -181,7 +181,8 @@ rte_ipv6_frag_reassemble_packet(struct rte_ip_frag_tbl *tbl,
 		"tbl: %p, max_cycles: %" PRIu64 ", entry_mask: %#x, "
 		"max_entries: %u, use_entries: %u\n\n",
 		__func__, __LINE__,
-		mb, tms, IPv6_KEY_BYTES(key.src_dst), key.id, ip_ofs, ip_len, frag_hdr->more_frags,
+		mb, tms, IPv6_KEY_BYTES(key.src_dst), key.id, ip_ofs, ip_len,
+		RTE_IPV6_GET_MF(frag_hdr->frag_data),
 		tbl, tbl->max_cycles, tbl->entry_mask, tbl->max_entries,
 		tbl->use_entries);
 
diff --git a/lib/librte_port/rte_port_ras.c b/lib/librte_port/rte_port_ras.c
index 6bd0f8c..8a2e554 100644
--- a/lib/librte_port/rte_port_ras.c
+++ b/lib/librte_port/rte_port_ras.c
@@ -212,12 +212,13 @@ process_ipv6(struct rte_port_ring_writer_ras *p, struct rte_mbuf *pkt)
 	struct ipv6_hdr *pkt_hdr = rte_pktmbuf_mtod(pkt, struct ipv6_hdr *);
 
 	struct ipv6_extension_fragment *frag_hdr;
+	uint16_t frag_data = 0;
 	frag_hdr = rte_ipv6_frag_get_ipv6_fragment_header(pkt_hdr);
-	uint16_t frag_offset = frag_hdr->frag_offset;
-	uint16_t frag_flag = frag_hdr->more_frags;
+	if (frag_hdr != NULL)
+		frag_data = rte_be_to_cpu_16(frag_hdr->frag_data);
 
 	/* If it is a fragmented packet, then try to reassemble */
-	if ((frag_flag == 0) && (frag_offset == 0))
+	if ((frag_data & RTE_IPV6_FRAG_USED_MASK) == 0)
 		p->tx_buf[p->tx_buf_count++] = pkt;
 	else {
 		struct rte_mbuf *mo;
-- 
1.7.9.5

Re: [dpdk-dev] [PATCH v4 1/1] ip_frag: fix creating ipv6 fragment extension header

[Ananyev, Konstantin]

> -----Original Message-----
> From: Azarewicz, PiotrX T
> Sent: Thursday, September 10, 2015 8:09 AM
> To: dev@dpdk.org
> Cc: Dumitrescu, Cristian; Ananyev, Konstantin; Azarewicz, PiotrX T
> Subject: [PATCH v4 1/1] ip_frag: fix creating ipv6 fragment extension header
> 
> Previous implementation won't work on every environment. The order of
> allocation of bit-fields within a unit (high-order to low-order or
> low-order to high-order) is implementation-defined.
> Solution: used bytes instead of bit fields.
> 
> v2 changes:
> - remove useless union
> - fix process_ipv6 function (due to remove the union above)
> 
> v3 changes:
> - add macros to read/set fragment_offset and more_flags values
> 
> v4 changes:
> - two additional fixes due to remove the union and due to changes in
> macros
> 
> Signed-off-by: Piotr Azarewicz <piotrx.t.azarewicz@intel.com>

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> ---

Re: [dpdk-dev] [PATCH v4 1/1] ip_frag: fix creating ipv6 fragment extension header

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Azarewicz, PiotrX T
> Sent: Thursday, September 10, 2015 10:09 AM
> To: dev@dpdk.org
> Cc: Dumitrescu, Cristian; Ananyev, Konstantin; Azarewicz, PiotrX T
> Subject: [PATCH v4 1/1] ip_frag: fix creating ipv6 fragment extension header
> 
> Previous implementation won't work on every environment. The order of
> allocation of bit-fields within a unit (high-order to low-order or
> low-order to high-order) is implementation-defined.
> Solution: used bytes instead of bit fields.
> 

Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

Re: [dpdk-dev] [PATCH v4 1/1] ip_frag: fix creating ipv6 fragment extension header

[Thomas Monjalon]

2015-09-10 12:24, Dumitrescu, Cristian:
> From: Azarewicz, PiotrX T
> > Previous implementation won't work on every environment. The order of
> > allocation of bit-fields within a unit (high-order to low-order or
> > low-order to high-order) is implementation-defined.
> > Solution: used bytes instead of bit fields.
> > 
> 
> Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

Applied, thanks