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 55FD02F7D for ; Wed, 13 Sep 2017 12:23:29 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Sep 2017 03:23:25 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,387,1500966000"; d="scan'208";a="134851457" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by orsmga002.jf.intel.com with ESMTP; 13 Sep 2017 03:23:25 -0700 Received: from fmsmsx120.amr.corp.intel.com (10.18.124.208) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 13 Sep 2017 03:23:24 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx120.amr.corp.intel.com (10.18.124.208) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 13 Sep 2017 03:23:24 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.39]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.93]) with mapi id 14.03.0319.002; Wed, 13 Sep 2017 18:23:22 +0800 From: "Hu, Jiayu" To: "Ananyev, Konstantin" CC: "dev@dpdk.org" , "Kavanagh, Mark B" , "Tan, Jianfeng" Thread-Topic: [PATCH v3 2/5] gso: add TCP/IPv4 GSO support Thread-Index: AQHTLHQNWWgZCD/CFEWS1gvLV6TAzqKyld0w Date: Wed, 13 Sep 2017 10:23:21 +0000 Message-ID: References: <1504598270-60080-1-git-send-email-jiayu.hu@intel.com> <1505184211-36728-1-git-send-email-jiayu.hu@intel.com> <1505184211-36728-3-git-send-email-jiayu.hu@intel.com> <2601191342CEEE43887BDE71AB9772584F249E8E@irsmsx105.ger.corp.intel.com> <20170913024801.GB44293@dpdk15.sh.intel.com> <2601191342CEEE43887BDE71AB9772584F24A622@irsmsx105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB9772584F24A622@irsmsx105.ger.corp.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZTVmZjZmZjYtZDQ4Mi00MzNjLTgzNTctYjY0Mjc2ZjMyMDkxIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6ImFTaWJBUGFtT21kRkR1eXpnTVM2ck9oeHdEWk9EcmVmZUllOVpsamlCUlE9In0= x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v3 2/5] gso: add TCP/IPv4 GSO support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Sep 2017 10:23:31 -0000 Hi Konstantin, > -----Original Message----- > From: Ananyev, Konstantin > Sent: Wednesday, September 13, 2017 5:38 PM > To: Hu, Jiayu > Cc: dev@dpdk.org; Kavanagh, Mark B ; Tan, > Jianfeng > Subject: RE: [PATCH v3 2/5] gso: add TCP/IPv4 GSO support >=20 >=20 >=20 > > > > + > > > > +int > > > > +gso_tcp4_segment(struct rte_mbuf *pkt, > > > > + uint16_t gso_size, > > > > + uint8_t ipid_delta, > > > > + struct rte_mempool *direct_pool, > > > > + struct rte_mempool *indirect_pool, > > > > + struct rte_mbuf **pkts_out, > > > > + uint16_t nb_pkts_out) > > > > +{ > > > > + struct ipv4_hdr *ipv4_hdr; > > > > + uint16_t tcp_dl; > > > > + uint16_t pyld_unit_size; > > > > + uint16_t hdr_offset; > > > > + int ret =3D 1; > > > > + > > > > + ipv4_hdr =3D (struct ipv4_hdr *)(rte_pktmbuf_mtod(pkt, char *) + > > > > + pkt->l2_len); > > > > + /* Don't process the fragmented packet */ > > > > + if (unlikely((ipv4_hdr->fragment_offset & rte_cpu_to_be_16( > > > > + IPV4_HDR_DF_MASK)) =3D=3D 0)) > { > > > > > > > > > It is not a check for fragmented packet - it is a check that fragment= ation > is allowed for that packet. > > > Should be IPV4_HDR_DF_MASK - 1, I think. >=20 > DF bit doesn't indicate is packet fragmented or not. > It forbids to fragment packet any further. > To check is packet already fragmented or not, you have to check MF bit an= d > frag_offset. > Both have to be zero for un-fragmented packets. Yes, you are right. I checked the RFC and I misunderstood the meaning of DF= bit. When DF bit is set to 1, the packet isn't IP fragmented. When DF bit is 0, = the packet may or may not be fragmented. So it can't indicate if the packet is an IP f= ragment. Only both MF and offset are 0, the packet is not fragmented. >=20 > > > > IMO, IPV4_HDR_DF_MASK whose value is (1 << 14) is used to get DF bit. I= t's > a > > little-endian value. But ipv4_hdr->fragment_offset is big-endian order. > > So the value of DF bit should be "ipv4_hdr->fragment_offset & > rte_cpu_to_be_16( > > IPV4_HDR_DF_MASK)". If this value is 0, the input packet is fragmented. > > > > > > > > > + pkts_out[0] =3D pkt; > > > > + return ret; > > > > + } > > > > + > > > > + tcp_dl =3D rte_be_to_cpu_16(ipv4_hdr->total_length) - pkt->l3_len= - > > > > + pkt->l4_len; > > > > > > Why not use pkt->pkt_len - pkt->l2_len -pkt_l3_len - pkt_l4_len? > > > > Yes, we can use pkt->pkt_len - pkt->l2_len -pkt_l3_len - pkt_l4_len her= e. > > > > > > > > > + /* Don't process the packet without data */ > > > > + if (unlikely(tcp_dl =3D=3D 0)) { > > > > + pkts_out[0] =3D pkt; > > > > + return ret; > > > > + } > > > > + > > > > + hdr_offset =3D pkt->l2_len + pkt->l3_len + pkt->l4_len; > > > > + pyld_unit_size =3D gso_size - hdr_offset - ETHER_CRC_LEN; > > > > > > Hmm, why do we need to count CRC_LEN here? > > > > Yes, we shouldn't count ETHER_CRC_LEN here. Its length should be > > included in gso_size. >=20 > Why? > What is the point to account crc len into this computation? > Why not just assume that gso_size is already a max_frame_size - crc_len > As I remember, when we RX packet crc bytes will be already stripped, > when user populates the packet, he doesn't care about crc bytes too. Sorry, maybe I didn't make it clear. I don't mean that applications must co= unt CRC when set gso_segsz. It's related specific scenarios to decide if count = CRC in gso_segsz or not, IMO. The GSO library shouldn't be aware of CRC, and ju= st uses gso_segsz to split packets. Thanks, Jiayu >=20 > Konstantin