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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

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