[dpdk-dev] [PATCH v3 2/5] gso: add TCP/IPv4 GSO support

[Hu, Jiayu]

Hi Konstantin,

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

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 fragment.
Only both MF and offset are 0, the packet is not fragmented.

> 
> >
> > 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.

Sorry, maybe I didn't make it clear. I don't mean that applications must count
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 just
uses gso_segsz to split packets.

Thanks,
Jiayu
> 
> Konstantin