[dpdk-dev] [PATCH v4 4/5] gso: add GRE GSO support

[Hu, Jiayu]

Hi Jianfeng,

> -----Original Message-----
> From: Tan, Jianfeng
> Sent: Wednesday, September 20, 2017 10:54 AM
> To: Hu, Jiayu <jiayu.hu at intel.com>; dev at dpdk.org
> Cc: Ananyev, Konstantin <konstantin.ananyev at intel.com>; Kavanagh, Mark
> B <mark.b.kavanagh at intel.com>; Yigit, Ferruh <ferruh.yigit at intel.com>;
> thomas at monjalon.net
> Subject: Re: [PATCH v4 4/5] gso: add GRE GSO support
> 
> Hi,
> 
> 
> On 9/19/2017 3:32 PM, Jiayu Hu wrote:
> > From: Mark Kavanagh <mark.b.kavanagh at intel.com>
> >
> > This patch adds GSO support for GRE-tunneled packets. Supported GRE
> > packets must contain an outer IPv4 header, and inner TCP/IPv4 headers.
> > They may also contain a single VLAN tag. GRE GSO doesn't check if all
> > input packets have correct checksums and doesn't update checksums for
> > output packets. Additionally, it doesn't process IP fragmented packets.
> >
> > As with VxLAN GSO, GRE GSO uses a two-segment MBUF to organize each
> > output packet, which requires multi-segment mbuf support in the TX
> > functions of the NIC driver. Also, if a packet is GSOed, GRE GSO reduces
> > its MBUF refcnt by 1. As a result, when all of its GSOed segments are
> > freed, the packet is freed automatically.
> >
> > GRE GSO clears the PKT_TX_TCP_SEG flag for the input packet and GSO
> > segments on the event of success.
> >
> > Signed-off-by: Mark Kavanagh <mark.b.kavanagh at intel.com>
> > Signed-off-by: Jiayu Hu <jiayu.hu at intel.com>
> > ---
> >   doc/guides/rel_notes/release_17_11.rst |  3 +++
> >   lib/librte_gso/gso_common.c            | 31
> +++++++++++++++++++++++++++++++
> >   lib/librte_gso/gso_common.h            | 25 +++++++++++++++++++++++++
> >   lib/librte_gso/gso_tunnel_tcp4.c       |  2 ++
> >   lib/librte_gso/rte_gso.c               |  8 +++++---
> >   5 files changed, 66 insertions(+), 3 deletions(-)
> >
> > diff --git a/doc/guides/rel_notes/release_17_11.rst
> b/doc/guides/rel_notes/release_17_11.rst
> > index 2dc6b89..119f662 100644
> > --- a/doc/guides/rel_notes/release_17_11.rst
> > +++ b/doc/guides/rel_notes/release_17_11.rst
> > @@ -51,6 +51,9 @@ New Features
> >     * VxLAN packets, which must have an outer IPv4 header (prepended by
> >       an optional VLAN tag), and contain an inner TCP/IPv4 packet (with
> >       an optional VLAN tag).
> > +  * GRE packets, which must contain an outer IPv4 header (prepended by
> > +    an optional VLAN tag), and inner TCP/IPv4 headers (with an optional
> > +    VLAN tag).
> >
> >     The GSO library doesn't check if the input packets have correct
> >     checksums, and doesn't update checksums for output packets.
> > diff --git a/lib/librte_gso/gso_common.c b/lib/librte_gso/gso_common.c
> > index 90fcb2a..3629625 100644
> > --- a/lib/librte_gso/gso_common.c
> > +++ b/lib/librte_gso/gso_common.c
> > @@ -37,6 +37,7 @@
> >   #include <rte_memcpy.h>
> >   #include <rte_mempool.h>
> >   #include <rte_ether.h>
> > +#include <rte_gre.h>
> >   #include <rte_ip.h>
> >   #include <rte_tcp.h>
> >   #include <rte_udp.h>
> > @@ -258,3 +259,33 @@ update_ipv4_vxlan_tcp4_header(struct rte_mbuf
> *pkt, uint8_t ipid_delta,
> >   		sent_seq += (segs[i]->pkt_len - segs[i]->data_len);
> >   	}
> >   }
> > +
> > +void
> > +update_ipv4_gre_tcp4_header(struct rte_mbuf *pkt, uint8_t ipid_delta,
> > +		struct rte_mbuf **segs, uint16_t nb_segs)
> > +{
> 
> This function seems to have too many duplicated code with above
> function, can we merge?

Yes, they can be merged into one.

> 
> > +	struct ipv4_hdr *ipv4_hdr;
> > +	struct tcp_hdr *tcp_hdr;
> > +	uint32_t sent_seq;
> > +	uint16_t l2_len, outer_id, inner_id, tail_idx, i;
> > +
> > +	ipv4_hdr = (struct ipv4_hdr *)(rte_pktmbuf_mtod(pkt, char *) +
> > +			pkt->outer_l2_len);
> > +	outer_id = rte_be_to_cpu_16(ipv4_hdr->packet_id);
> > +
> > +	l2_len = pkt->outer_l2_len + pkt->outer_l3_len + pkt->l2_len;
> > +	ipv4_hdr = (struct ipv4_hdr *)(rte_pktmbuf_mtod(pkt, char *) +
> l2_len);
> > +	inner_id = rte_be_to_cpu_16(ipv4_hdr->packet_id);
> > +	tcp_hdr = (struct tcp_hdr *)((char *)ipv4_hdr + pkt->l3_len);
> > +	sent_seq = rte_be_to_cpu_32(tcp_hdr->sent_seq);
> > +	tail_idx = nb_segs - 1;
> > +	for (i = 0; i < nb_segs; i++) {
> > +		__update_outer_ipv4_header(segs[i], outer_id);
> > +		outer_id += ipid_delta;
> 
> We should update both outer and inner IPID? Could you add some spec
> reference here?

I check the VxLAN RFC, but it doesn't have strict limits on the value of outer IP ID.
In the implementation of Linux GSO, the outer IP ID is always incremental, where
SKB_GSO_TCP_FIXEDID is only meaningful to inner IP ID.

If no objections, I will take the same design as Linux. Wonder your opinions,
@Jianfeng @Konstantin @Mark.

Thanks,
Jiayu

> 
> > +
> > +		__update_ipv4_tcp_header(segs[i], l2_len, inner_id,
> sent_seq,
> > +				i < tail_idx);
> > +		inner_id += ipid_delta;
> > +		sent_seq += (segs[i]->pkt_len - segs[i]->data_len);
> > +	}
> > +}
> > diff --git a/lib/librte_gso/gso_common.h b/lib/librte_gso/gso_common.h
> > index 0b0d8ed..433e952 100644
> > --- a/lib/librte_gso/gso_common.h
> > +++ b/lib/librte_gso/gso_common.h
> > @@ -53,6 +53,11 @@
> >   		(PKT_TX_TCP_SEG | PKT_TX_IPV4 | PKT_TX_OUTER_IPV4 | \
> >   		 PKT_TX_TUNNEL_VXLAN))
> >
> > +#define IS_IPV4_GRE_TCP4(flag) (((flag) & (PKT_TX_TCP_SEG |
> PKT_TX_IPV4 | \
> > +				PKT_TX_OUTER_IPV4 |
> PKT_TX_TUNNEL_GRE)) == \
> > +		(PKT_TX_TCP_SEG | PKT_TX_IPV4 | PKT_TX_OUTER_IPV4 | \
> > +		 PKT_TX_TUNNEL_GRE))
> > +
> >   /**
> >    * Internal function which updates relevant packet headers for TCP/IPv4
> >    * packets, following segmentation. This is required to update, for
> > @@ -94,6 +99,26 @@ void update_ipv4_vxlan_tcp4_header(struct
> rte_mbuf *pkt,
> >   		uint16_t nb_segs);
> >
> >   /**
> > + * Internal function which updates relevant packet headers for GRE
> > + * packets, following segmentation. This is required to update, for
> > + * example, the IPv4 'total_length' field, to reflect the reduced
> > + * length of the now-segmented packet.
> > + *
> > + * @param pkt
> > + *  The original packet.
> > + * @param ipid_delta
> > + *  The increasing uint of IP ids.
> 
> uint -> unit?
> 
> > + * @param segs
> > + *  Pointer array used for storing mbuf addresses for GSO segments.
> > + * @param nb_segs
> > + *  The number of GSO segments placed in segs.
> > + */
> > +void update_ipv4_gre_tcp4_header(struct rte_mbuf *pkt,
> > +		uint8_t ipid_delta,
> > +		struct rte_mbuf **segs,
> > +		uint16_t nb_segs);
> > +
> > +/**
> >    * Internal function which divides the input packet into small segments.
> >    * Each of the newly-created segments is organized as a two-segment
> MBUF,
> >    * where the first segment is a standard mbuf, which stores a copy of
> > diff --git a/lib/librte_gso/gso_tunnel_tcp4.c
> b/lib/librte_gso/gso_tunnel_tcp4.c
> > index cc017bd..5d5930a 100644
> > --- a/lib/librte_gso/gso_tunnel_tcp4.c
> > +++ b/lib/librte_gso/gso_tunnel_tcp4.c
> > @@ -82,6 +82,8 @@ gso_tunnel_tcp4_segment(struct rte_mbuf *pkt,
> >
> >   	if (pkt->ol_flags & PKT_TX_TUNNEL_VXLAN)
> >   		update_ipv4_vxlan_tcp4_header(pkt, ipid_delta, pkts_out,
> ret);
> > +	else if (pkt->ol_flags & PKT_TX_TUNNEL_GRE)
> > +		update_ipv4_gre_tcp4_header(pkt, ipid_delta, pkts_out, ret);
> >
> >   	return ret;
> >   }
> > diff --git a/lib/librte_gso/rte_gso.c b/lib/librte_gso/rte_gso.c
> > index d1a723b..5464831 100644
> > --- a/lib/librte_gso/rte_gso.c
> > +++ b/lib/librte_gso/rte_gso.c
> > @@ -60,8 +60,9 @@ rte_gso_segment(struct rte_mbuf *pkt,
> >
> >   	if ((gso_ctx->gso_size >= pkt->pkt_len) || (gso_ctx->gso_types &
> >   				(DEV_TX_OFFLOAD_TCP_TSO |
> > -				 DEV_TX_OFFLOAD_VXLAN_TNL_TSO)) !=
> > -				gso_ctx->gso_types) {
> > +				 DEV_TX_OFFLOAD_VXLAN_TNL_TSO |
> > +				 DEV_TX_OFFLOAD_GRE_TNL_TSO)) !=
> > +				 gso_ctx->gso_types) {
> >   		pkt->ol_flags &= (~PKT_TX_TCP_SEG);
> >   		pkts_out[0] = pkt;
> >   		return ret;
> > @@ -73,7 +74,8 @@ rte_gso_segment(struct rte_mbuf *pkt,
> >   	ipid_delta = (gso_ctx->ipid_flag != RTE_GSO_IPID_FIXED);
> >   	ol_flags = pkt->ol_flags;
> >
> > -	if (IS_IPV4_VXLAN_TCP4(pkt->ol_flags)) {
> > +	if (IS_IPV4_VXLAN_TCP4(pkt->ol_flags) ||
> > +			IS_IPV4_GRE_TCP4(pkt->ol_flags)) {
> >   		pkt->ol_flags &= (~PKT_TX_TCP_SEG);
> >   		ret = gso_tunnel_tcp4_segment(pkt, gso_size, ipid_delta,
> >   				direct_pool, indirect_pool,