Thread (27 messages) 27 messages, 6 authors, 2016-10-10

Re: [PATCH v4 3/3] app/testpmd: fix Tx offload on tunneling packet

From: Ananyev, Konstantin <hidden>
Date: 2016-09-19 12:09:53

Hi Jainfeng,
quoted hunk ↗ jump to hunk
-----Original Message-----
From: Tan, Jianfeng
Sent: Monday, August 1, 2016 4:57 AM
To: dev@dpdk.org
Cc: thomas.monjalon@6wind.com; De Lara Guarch, Pablo <redacted>; Ananyev, Konstantin
[off-list ref]; Wu, Jingjing [off-list ref]; Zhang, Helin [off-list ref]; Tan, Jianfeng
[off-list ref]; Tao, Zhe [off-list ref]
Subject: [PATCH v4 3/3] app/testpmd: fix Tx offload on tunneling packet

Tx offload on tunneling packet now requires applications to correctly set tunneling type. Without setting it, i40e driver does not parse
tunneling parameters. Besides that, add a check to see if NIC supports TSO on tunneling packet when executing "csum parse_tunnel on
_port"
after "tso set _size _port" or the other way around.

Fixes: b51c47536a9e ("app/testpmd: support TSO in checksum forward engine")

Signed-off-by: Zhe Tao <redacted>
Signed-off-by: Jianfeng Tan <redacted>
---
 app/test-pmd/cmdline.c  | 42 ++++++++++++++++++++++++++++++++++++------
 app/test-pmd/csumonly.c | 37 +++++++++++++++++++++++++++++--------
 2 files changed, 65 insertions(+), 14 deletions(-)
diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index f90befc..561839f 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -3426,6 +3426,26 @@ struct cmd_csum_tunnel_result {  };

 static void
+check_tunnel_tso_support(uint8_t port_id) {
+	struct rte_eth_dev_info dev_info;
+
+	rte_eth_dev_info_get(port_id, &dev_info);
+	if (!(dev_info.tx_offload_capa & DEV_TX_OFFLOAD_VXLAN_TNL_TSO))
+		printf("Warning: TSO enabled but VXLAN TUNNEL TSO not "
+		       "supported by port %d\n", port_id);
+	if (!(dev_info.tx_offload_capa & DEV_TX_OFFLOAD_GRE_TNL_TSO))
+		printf("Warning: TSO enabled but GRE TUNNEL TSO not "
+			"supported by port %d\n", port_id);
+	if (!(dev_info.tx_offload_capa & DEV_TX_OFFLOAD_IPIP_TNL_TSO))
+		printf("Warning: TSO enabled but IPIP TUNNEL TSO not "
+		       "supported by port %d\n", port_id);
+	if (!(dev_info.tx_offload_capa & DEV_TX_OFFLOAD_GENEVE_TNL_TSO))
+		printf("Warning: TSO enabled but GENEVE TUNNEL TSO not "
+		       "supported by port %d\n", port_id); }
+
+static void
 cmd_csum_tunnel_parsed(void *parsed_result,
 		       __attribute__((unused)) struct cmdline *cl,
 		       __attribute__((unused)) void *data) @@ -3435,10 +3455,13 @@ cmd_csum_tunnel_parsed(void *parsed_result,
 	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
 		return;

-	if (!strcmp(res->onoff, "on"))
+	if (!strcmp(res->onoff, "on")) {
 		ports[res->port_id].tx_ol_flags |=
 			TESTPMD_TX_OFFLOAD_PARSE_TUNNEL;
-	else
+
+		if (ports[res->port_id].tso_segsz != 0)
+			check_tunnel_tso_support(res->port_id);
+	} else
 		ports[res->port_id].tx_ol_flags &=
 			(~TESTPMD_TX_OFFLOAD_PARSE_TUNNEL);
@@ -3502,10 +3525,17 @@ cmd_tso_set_parsed(void *parsed_result,

 	/* display warnings if configuration is not supported by the NIC */
 	rte_eth_dev_info_get(res->port_id, &dev_info);
-	if ((ports[res->port_id].tso_segsz != 0) &&
-		(dev_info.tx_offload_capa & DEV_TX_OFFLOAD_TCP_TSO) == 0) {
-		printf("Warning: TSO enabled but not "
-			"supported by port %d\n", res->port_id);
+	if (ports[res->port_id].tso_segsz != 0) {
+		if (ports[res->port_id].tx_ol_flags &
+		    TESTPMD_TX_OFFLOAD_PARSE_TUNNEL)
+			check_tunnel_tso_support(res->port_id);
+		/* For packets,
+		 * (1) when tnl parse is disabled;
+		 * (2) when tnl parse is enabled but not deemed as tnl pkts
+		 */
+		if (!(dev_info.tx_offload_capa & DEV_TX_OFFLOAD_TCP_TSO))
+			printf("Warning: TSO enabled but not "
+			       "supported by port %d\n", res->port_id);
 	}
 }
diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c index ac4bd8f..0a1f95d 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -412,12 +412,10 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
 	return ol_flags;
 }

-/* Calculate the checksum of outer header (only vxlan is supported,
- * meaning IP + UDP). The caller already checked that it's a vxlan
- * packet */
+/* Calculate the checksum of outer header */
 static uint64_t
 process_outer_cksums(void *outer_l3_hdr, struct testpmd_offload_info *info,
-	uint16_t testpmd_ol_flags)
+	uint16_t testpmd_ol_flags, int tso_enabled)
 {
 	struct ipv4_hdr *ipv4_hdr = outer_l3_hdr;
 	struct ipv6_hdr *ipv6_hdr = outer_l3_hdr; @@ -438,10 +436,20 @@ process_outer_cksums(void *outer_l3_hdr, struct
testpmd_offload_info *info,
 	if (info->outer_l4_proto != IPPROTO_UDP)
 		return ol_flags;

-	/* outer UDP checksum is always done in software as we have no
-	 * hardware supporting it today, and no API for it. */
-
 	udp_hdr = (struct udp_hdr *)((char *)outer_l3_hdr + info->outer_l3_len);
+
+	/* outer UDP checksum is done in software as we have no hardware
+	 * supporting it today, and no API for it. In the other side, for
+	 * UDP tunneling, like VXLAN or Geneve, outer UDP checksum can be
+	 * set to zero.
+	 *
+	 * If a packet will be TSOed into small packets by NIC, we cannot
+	 * set/calculate a non-zero checksum, because it will be a wrong
+	 * value after the packet be split into several small packets.
+	 */
+	if (tso_enabled)
+		udp_hdr->dgram_cksum = 0;
+
 	/* do not recalculate udp cksum if it was 0 */
 	if (udp_hdr->dgram_cksum != 0) {
 		udp_hdr->dgram_cksum = 0;
@@ -704,18 +712,27 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
 		if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_PARSE_TUNNEL) {
 			if (info.l4_proto == IPPROTO_UDP) {
 				struct udp_hdr *udp_hdr;
+
 				udp_hdr = (struct udp_hdr *)((char *)l3_hdr +
 					info.l3_len);
 				parse_vxlan(udp_hdr, &info, m->packet_type);
+				if (info.is_tunnel)
+					ol_flags |= PKT_TX_TUNNEL_VXLAN;
 			} else if (info.l4_proto == IPPROTO_GRE) {
 				struct simple_gre_hdr *gre_hdr;
+
 				gre_hdr = (struct simple_gre_hdr *)
 					((char *)l3_hdr + info.l3_len);
 				parse_gre(gre_hdr, &info);
+				if (info.is_tunnel)
+					ol_flags |= PKT_TX_TUNNEL_GRE;
 			} else if (info.l4_proto == IPPROTO_IPIP) {
 				void *encap_ip_hdr;
+
 				encap_ip_hdr = (char *)l3_hdr + info.l3_len;
 				parse_encap_ip(encap_ip_hdr, &info);
+				if (info.is_tunnel)
+					ol_flags |= PKT_TX_TUNNEL_IPIP;
 			}
 		}
@@ -745,7 +762,7 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
 		 * processed in hardware. */
 		if (info.is_tunnel == 1) {
 			ol_flags |= process_outer_cksums(outer_l3_hdr, &info,
-				testpmd_ol_flags);
+				testpmd_ol_flags, ol_flags & PKT_TX_TCP_SEG);
 		}

 		/* step 4: fill the mbuf meta data (flags and header lengths) */ @@ -806,6 +823,10 @@

It was a while since I looked a t it closely, but shouldn't you also update step 4 below:

if (info.is_tunnel == 1) {
                        if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM) {
                                m->outer_l2_len = info.outer_l2_len;
                                m->outer_l3_len = info.outer_l3_len;
                                m->l2_len = info.l2_len;
                                m->l3_len = info.l3_len;
                                m->l4_len = info.l4_len;
                        }
                        else {
                                /* if there is a outer UDP cksum
                                   processed in sw and the inner in hw,
                                   the outer checksum will be wrong as
                                   the payload will be modified by the
                                   hardware */
                                m->l2_len = info.outer_l2_len +
                                        info.outer_l3_len + info.l2_len;
                                m->l3_len = info.l3_len;
                                m->l4_len = info.l4_len;
                        }


?

In particular shouldn't it be something like:
if ((testpmd_ol_flags & TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM) != 0 ||
      ((testmpd_ol_flags & TESTPMD_TX_OFFLOAD_PARSE_TUNNEL) != 0 && info.tso_segsz != 0)) {
....
?

Another thought, might be it is worth to introduce new flag: TESTPMD_TX_OFFLOAD_TSO_TUNNEL,
and new command in cmdline.c, that would set/clear that flag.
Instead of trying to make assumptions does 
user wants tso for tunneled packets based on 2 different things:
- enable/disable tso
- enable/disable tunneled packets parsing
?

Konstantin
pkt_burst_checksum_forward(struct fwd_stream *fs)
 				{ PKT_TX_OUTER_IPV4, PKT_TX_OUTER_IPV4 },
 				{ PKT_TX_OUTER_IPV6, PKT_TX_OUTER_IPV6 },
 				{ PKT_TX_TCP_SEG, PKT_TX_TCP_SEG },
+				{ PKT_TX_TUNNEL_VXLAN, PKT_TX_TUNNEL_MASK },
+				{ PKT_TX_TUNNEL_GRE, PKT_TX_TUNNEL_MASK },
+				{ PKT_TX_TUNNEL_IPIP, PKT_TX_TUNNEL_MASK },
+				{ PKT_TX_TUNNEL_GENEVE, PKT_TX_TUNNEL_MASK },
 			};
 			unsigned j;
 			const char *name;
--
2.7.4
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help