Hi Mark, > -----Original Message----- > From: Kavanagh, Mark B > Sent: Wednesday, September 13, 2017 3:52 PM > To: Ananyev, Konstantin <konstantin.anan...@intel.com>; Hu, Jiayu > <jiayu...@intel.com> > Cc: dev@dpdk.org; Tan, Jianfeng <jianfeng....@intel.com> > Subject: RE: [PATCH v3 2/5] gso: add TCP/IPv4 GSO support > > >From: Ananyev, Konstantin > >Sent: Wednesday, September 13, 2017 10:38 AM > >To: Hu, Jiayu <jiayu...@intel.com> > >Cc: dev@dpdk.org; Kavanagh, Mark B <mark.b.kavan...@intel.com>; Tan, Jianfeng > ><jianfeng....@intel.com> > >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 = 1; > >> > > + > >> > > + ipv4_hdr = (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)) == > >> > > 0)) { > >> > > >> > > >> > It is not a check for fragmented packet - it is a check that > >> > fragmentation > >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] = pkt; > >> > > + return ret; > >> > > + } > >> > > + > >> > > + tcp_dl = 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 == 0)) { > >> > > + pkts_out[0] = pkt; > >> > > + return ret; > >> > > + } > >> > > + > >> > > + hdr_offset = pkt->l2_len + pkt->l3_len + pkt->l4_len; > >> > > + pyld_unit_size = 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 > calculation.
Ok, and what prevents you to set gso_ctx.gso_size = 1514; /*ether frame size without crc bytes */ ? Konstantin > > If I've missed anything, please do let me know! > > Thanks, > Mark > > > > >Konstantin