From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 6911B5958 for ; Wed, 9 Sep 2015 12:47:24 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga101.fm.intel.com with ESMTP; 09 Sep 2015 03:47:23 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.17,496,1437462000"; d="scan'208";a="558214404" Received: from irsmsx103.ger.corp.intel.com ([163.33.3.157]) by FMSMGA003.fm.intel.com with ESMTP; 09 Sep 2015 03:47:23 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.51]) by IRSMSX103.ger.corp.intel.com ([169.254.3.251]) with mapi id 14.03.0224.002; Wed, 9 Sep 2015 11:47:22 +0100 From: "Ananyev, Konstantin" To: "Azarewicz, PiotrX T" Thread-Topic: [PATCH v2 1/1] ip_frag: fix creating ipv6 fragment extension header Thread-Index: AQHQ6kBsdHGUDC4AnkC7z4a7exLpBZ4z8yYAgAARqCA= Date: Wed, 9 Sep 2015 10:47:21 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725836A84A7C@irsmsx105.ger.corp.intel.com> References: <1441203181-15487-1-git-send-email-piotrx.t.azarewicz@intel.com> <1441721267-19142-1-git-send-email-piotrx.t.azarewicz@intel.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: [dpdk-dev] FW: [PATCH v2 1/1] ip_frag: fix creating ipv6 fragment extension header X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 09 Sep 2015 10:47:25 -0000 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 hea= der >=20 > 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. >=20 > v2 changes: > - remove useless union > - fix process_ipv6 function (due to remove the union above) >=20 > Signed-off-by: Piotr Azarewicz > --- > 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(-) >=20 > 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__)); >=20 > 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, >=20 > fh =3D (struct ipv6_extension_fragment *) ++dst; > fh->next_header =3D src->proto; > - fh->reserved1 =3D 0; > - fh->frag_offset =3D rte_cpu_to_be_16(fofs); > - fh->reserved2 =3D 0; > - fh->more_frags =3D rte_cpu_to_be_16(mf); > + fh->reserved =3D 0; > + fh->frag_data =3D rte_cpu_to_be_16((fofs & ~IPV6_HDR_FO_MASK) | mf); > fh->id =3D 0; > } >=20 > diff --git a/lib/librte_port/rte_port_ras.c b/lib/librte_port/rte_port_ra= s.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, stru= ct rte_mbuf *pkt) > } > } >=20 > +#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, st= ruct rte_mbuf *pkt) > struct ipv6_hdr *pkt_hdr =3D rte_pktmbuf_mtod(pkt, struct ipv6_hdr *); >=20 > struct ipv6_extension_fragment *frag_hdr; > + uint16_t frag_data =3D 0; > frag_hdr =3D rte_ipv6_frag_get_ipv6_fragment_header(pkt_hdr); > - uint16_t frag_offset =3D frag_hdr->frag_offset; > - uint16_t frag_flag =3D frag_hdr->more_frags; > + if (frag_hdr !=3D NULL) > + frag_data =3D rte_be_to_cpu_16(frag_hdr->frag_data); >=20 > /* If it is a fragmented packet, then try to reassemble */ > - if ((frag_flag =3D=3D 0) && (frag_offset =3D=3D 0)) > + if ((MORE_FRAGS(frag_data) =3D=3D 0) && (FRAG_OFFSET(frag_data) =3D=3D = 0)) > p->tx_buf[p->tx_buf_count++] =3D pkt; > else { > struct rte_mbuf *mo; > -- > 1.7.9.5 When I wrote " provide macros to read/set fragment_offset and more_flags va= lues", I meant move IPV6_HDR_*macros that are useful from lib/librte_ip_frag/rte_i= pv6_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) =3D=3D 0) && (FRAG_OFFSET(frag_data) =3D=3D 0)= ) Why do you need && here? Why just not: #define RTE_IPV6_FRAG_USED_MASK (RTE_IPV6_HDR_MF_MASK | ~RTE_IPV6_HDR_FO_M= ASK) ... if ((frag_data & RTE_IPV6_FRAG_USED_MASK) =3D=3D 0) ? Thanks Konstantin