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

[Hu, Jiayu]

Hi Konstantin,

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Monday, September 4, 2017 5:55 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 2/5] gso/lib: add TCP/IPv4 GSO support
> 
> Hi Jiayu,
> 
> > -----Original Message-----
> > From: Hu, Jiayu
> > Sent: Monday, September 4, 2017 4:32 AM
> > To: Ananyev, Konstantin <konstantin.ananyev 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 2/5] gso/lib: add TCP/IPv4 GSO support
> >
> > Hi Konstantin,
> >
> > About the IP identifier, I check the linux codes and have some feedbacks
> inline.
> >
> > On Wed, Aug 30, 2017 at 09:38:33AM +0800, Ananyev, Konstantin wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Hu, Jiayu
> > > > Sent: Thursday, August 24, 2017 3:16 PM
> > > > To: dev at dpdk.org
> > > > Cc: Kavanagh, Mark B <mark.b.kavanagh at intel.com>; Ananyev,
> Konstantin <konstantin.ananyev at intel.com>; Tan, Jianfeng
> > > > <jianfeng.tan at intel.com>; Hu, Jiayu <jiayu.hu at intel.com>
> > > > Subject: [PATCH 2/5] gso/lib: add TCP/IPv4 GSO support
> > > >
> > > > This patch adds GSO support for TCP/IPv4 packets. Supported packets
> > > > may include a single VLAN tag. TCP/IPv4 GSO assumes that all input
> > > > packets have correct checksums, and doesn't update checksums for
> output
> > > > packets (the responsibility for this lies with the application).
> > > > Additionally, TCP/IPv4 GSO doesn't process IP fragmented packets.
> > > >
> > > > TCP/IPv4 GSO uses two chained MBUFs, one direct MBUF and one
> indrect
> > > > MBUF, to organize an output packet. Note that we refer to these two
> > > > chained MBUFs as a two-segment MBUF. The direct MBUF stores the
> packet
> > > > header, while the indirect mbuf simply points to a location within the
> > > > original packet's payload. Consequently, use of the GSO library requires
> > > > multi-segment MBUF support in the TX functions of the NIC driver.
> > > >
> > > > If a packet is GSOed, TCP/IPv4 GSO reduces its MBUF refcnt by 1. As a
> > > > result, when all of its GSOed segments are freed, the packet is freed
> > > > automatically.
> > > >
> > > > Signed-off-by: Jiayu Hu <jiayu.hu at intel.com>
> > > > Signed-off-by: Mark Kavanagh <mark.b.kavanagh at intel.com>
> > > > ---
> > > > +void
> > > > +gso_update_pkt_headers(struct rte_mbuf *pkt, uint16_t nb_segments,
> > > > +		struct rte_mbuf **out_segments)
> > > > +{
> > > > +	struct ipv4_hdr *ipv4_hdr;
> > > > +	struct tcp_hdr *tcp_hdr;
> > > > +	struct rte_mbuf *seg;
> > > > +	uint32_t sent_seq;
> > > > +	uint16_t offset, i;
> > > > +	uint16_t tail_seg_idx = nb_segments - 1, id;
> > > > +
> > > > +	switch (pkt->packet_type) {
> > > > +	case ETHER_VLAN_IPv4_TCP_PKT:
> > > > +	case ETHER_IPv4_TCP_PKT:
> > >
> > > Might be worth to put code below in a separate function:
> > > update_inner_tcp_hdr(..) or so.
> > > Then you can reuse it for tunneled cases too.
> > >
> > > > +		ipv4_hdr = (struct ipv4_hdr *)(rte_pktmbuf_mtod(pkt, char *)
> > > > +				pkt->l2_len);
> > > > +		tcp_hdr = (struct tcp_hdr *)((char *)ipv4_hdr + pkt->l3_len);
> > > > +		id = rte_be_to_cpu_16(ipv4_hdr->packet_id);
> > > > +		sent_seq = rte_be_to_cpu_32(tcp_hdr->sent_seq);
> > > > +
> > > > +		for (i = 0; i < nb_segments; i++) {
> > > > +			seg = out_segments[i];
> > > > +
> > > > +			offset = seg->l2_len;
> > > > +			update_ipv4_header(rte_pktmbuf_mtod(seg, char *),
> > > > +					offset, seg->pkt_len, id);
> > > > +			id++;
> > >
> > > Who would be responsible to make sure that we wouldn't have
> consecutive packets with the IPV4 id?
> > > Would be the upper layer that forms the packet or gso library or ...?
> >
> > Linux supports two kinds of IP identifier: fixed identifier and incremental
> identifier, and
> > which one to use depends on upper protocol modules. Specifically, if the
> protocol module
> > wants fixed identifiers, it will set SKB_GSO_TCP_FIXEDID to skb->gso_type,
> and then
> > inet_gso_segment() will keep identifiers the same. Otherwise, all segments
> will have
> > incremental identifiers. The reason for this design is that some protocols
> may choose fixed
> > IP identifiers, like TCP (from RFC791). This design also shows that linux
> ignores the issue
> > of repeated IP identifiers.
> >
> > From the perspective of DPDK, we need to solve two problems. One is if
> ignore the issue of
> > repeated IP identifiers. The other is if the GSO library provides an interface
> to upper
> > applications to enable them to choose fixed or incremental identifiers, or
> simply uses
> > incremental IP identifiers.
> >
> > Do you have any suggestions?
> 
> 
> Do the same as Linux?
> I.E. add some flag RRE_GSO_IPID_FIXED (or so) into gso_ctx?

OK, I see. We can do that.

In the GRO library, we check if the IP identifiers are incremental compulsorily. If we
enable fixed IP identifier in GSO, it seems we also need to change the GRO library.
I mean ignore IP identifier when merge packets, and don't update the IP identifier
for the merged packet. What do you think of it?

Thanks,
Jiayu

> Konstantin
> 
> >
> > Thanks,
> > Jiayu
> >
> > >
> > > > +
> > > > +			offset += seg->l3_len;
> > > > +			update_tcp_header(rte_pktmbuf_mtod(seg, char *),
> > > > +					offset, sent_seq, i < tail_seg_idx);
> > > > +			sent_seq += seg->next->data_len;
> > > > +		}
> > > > +		break;
> > > > +	}
> > > > +}
> > > > --
> > > > 2.7.4