From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Kavanagh, Mark B" Subject: Re: [PATCH v3 2/5] gso: add TCP/IPv4 GSO support Date: Wed, 13 Sep 2017 14:52:11 +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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" , "Tan, Jianfeng" To: "Ananyev, Konstantin" , "Hu, Jiayu" Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 68179325B for ; Wed, 13 Sep 2017 16:52:15 +0200 (CEST) In-Reply-To: <2601191342CEEE43887BDE71AB9772584F24A622@irsmsx105.ger.corp.intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" >From: Ananyev, Konstantin >Sent: Wednesday, September 13, 2017 10:38 AM >To: Hu, Jiayu >Cc: dev@dpdk.org; Kavanagh, Mark B ; Tan, Jianf= eng > >Subject: RE: [PATCH v3 2/5] gso: add TCP/IPv4 GSO support > > > >> > > + >> > > +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 fragmenta= tion >is allowed for that packet. >> > Should be IPV4_HDR_DF_MASK - 1, I think. > >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 and >frag_offset. >Both have to be zero for un-fragmented packets. > >> >> IMO, IPV4_HDR_DF_MASK whose value is (1 << 14) is used to get DF bit. It= '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 here= . >> >> > >> > > + /* 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. > >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. Hi Konstantin, When packet is tx'd, the 4B for CRC are added back into the packet; if the = payload is already at max capacity, then the actual segment size will be 4B= larger than expected (e.g. 1522B, as opposed to 1518B). To prevent that from happening, we account for the CRC len in this calculat= ion. If I've missed anything, please do let me know! Thanks, Mark=20 > >Konstantin