Re: [PATCH v6 2/6] gso: add TCP/IPv4 GSO support

["Kavanagh, Mark B" <mark.b.kavanagh@intel.com>]

>From: Ananyev, Konstantin
>Sent: Wednesday, October 4, 2017 3:49 PM
>To: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; dev@dpdk.org
>Cc: Hu, Jiayu <jiayu.hu@intel.com>; Tan, Jianfeng <jianfeng.tan@intel.com>;
>Yigit, Ferruh <ferruh.yigit@intel.com>; thomas@monjalon.net
>Subject: RE: [PATCH v6 2/6] gso: add TCP/IPv4 GSO support
>
>> >>  int
>> >>  rte_gso_segment(struct rte_mbuf *pkt,
>> >> @@ -41,12 +46,53 @@
>> >>  		struct rte_mbuf **pkts_out,
>> >>  		uint16_t nb_pkts_out)
>> >>  {
>> >> +	struct rte_mempool *direct_pool, *indirect_pool;
>> >> +	struct rte_mbuf *pkt_seg;
>> >> +	uint64_t ol_flags;
>> >> +	uint16_t gso_size;
>> >> +	uint8_t ipid_delta;
>> >> +	int ret = 1;
>> >> +
>> >>  	if (pkt == NULL || pkts_out == NULL || gso_ctx == NULL ||
>> >>  			nb_pkts_out < 1)
>> >>  		return -EINVAL;
>> >>
>> >> -	pkt->ol_flags &= (~PKT_TX_TCP_SEG);
>> >> -	pkts_out[0] = pkt;
>> >> +	if ((gso_ctx->gso_size >= pkt->pkt_len) || (gso_ctx->gso_types &
>> >> +				DEV_TX_OFFLOAD_TCP_TSO) !=
>> >> +			gso_ctx->gso_types) {
>> >> +		pkt->ol_flags &= (~PKT_TX_TCP_SEG);
>> >> +		pkts_out[0] = pkt;
>> >> +		return 1;
>> >> +	}
>> >> +
>> >> +	direct_pool = gso_ctx->direct_pool;
>> >> +	indirect_pool = gso_ctx->indirect_pool;
>> >> +	gso_size = gso_ctx->gso_size;
>> >> +	ipid_delta = (gso_ctx->ipid_flag != RTE_GSO_IPID_FIXED);
>> >> +	ol_flags = pkt->ol_flags;
>> >> +
>> >> +	if (IS_IPV4_TCP(pkt->ol_flags)) {
>> >> +		pkt->ol_flags &= (~PKT_TX_TCP_SEG);
>> >> +		ret = gso_tcp4_segment(pkt, gso_size, ipid_delta,
>> >> +				direct_pool, indirect_pool,
>> >> +				pkts_out, nb_pkts_out);
>> >> +	} else {
>> >> +		pkt->ol_flags &= (~PKT_TX_TCP_SEG);
>> >
>> >Not sure why do you clean this flag if you don't support that packet type
>> >and no action was perfomed?
>> >Suppose you have a mix ipv4 and ipv6 packets - gso lib would do ipv4 and
>> >someone else
>> >(HW?) can do ipv4 segmentation.
>>
>> I can't say for definite, since I didn't implement this change. However, I
>can only presume that the assumption here is that since
>> segmentation is being done in S/W that the underlying H/W does not support
>TSO.
>> Since the underlying HW can't segment the packet in HW, we should clear the
>flag; otherwise, if an mbuf marked for TCP segmentation is
>> passed to the driver of a NIC that does not support/understand that feature,
>the behavior is undefined.
>> Is this a fair assumption in your opinion, or is it the case that the packet
>would simply be transmitted un-segmented in that case, and so we
>> shouldn't clear the flag?
>
>Yes, I think if we shouldn't clear the flag if we didn't do any segmentation
>(we just encounter a packet type that we don't support).
>Konstantin

Okay, thanks for clarifying - I'll update the code accordingly.
-Mark

>
>>
>> Thanks again,
>> Mark
>>
>> >BTW, did you notice that building of shared target fails?
>> >Konstantin
>>
>> I didn't, but I'll take a look right now - thanks for the catch!
>>
>> >
>> >
>> >> +		pkts_out[0] = pkt;
>> >> +		RTE_LOG(WARNING, GSO, "Unsupported packet type\n");
>> >> +		return 1;
>> >> +	}
>> >> +
>> >> +	if (ret > 1) {
>> >> +		pkt_seg = pkt;
>> >> +		while (pkt_seg) {
>> >> +			rte_mbuf_refcnt_update(pkt_seg, -1);
>> >> +			pkt_seg = pkt_seg->next;
>> >> +		}
>> >> +	} else if (ret < 0) {
>> >> +		/* Revert the ol_flags in the event of failure. */
>> >> +		pkt->ol_flags = ol_flags;
>> >> +	}
>> >>
>> >> -	return 1;
>> >> +	return ret;
>> >>  }
>> >> --
>> >> 1.9.3