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

[Kavanagh, Mark B]

>From: Ananyev, Konstantin
>Sent: Wednesday, September 13, 2017 10:38 AM
>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.
>
>>
>> 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.

If I've missed anything, please do let me know!

Thanks,
Mark 

>
>Konstantin