Re: [PATCH v3 2/5] gso: add TCP/IPv4 GSO support

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

Hi Mark,

> -----Original Message-----
> From: Kavanagh, Mark B
> Sent: Wednesday, September 13, 2017 3:52 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Hu, Jiayu <jiayu.hu@intel.com>
> Cc: dev@dpdk.org; Tan, Jianfeng <jianfeng.tan@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.hu@intel.com>
> >Cc: dev@dpdk.org; Kavanagh, Mark B <mark.b.kavanagh@intel.com>; Tan, Jianfeng
> ><jianfeng.tan@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