[dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Jijiang Liu]

In the current codes, the "tx_checksum set (ip|udp|tcp|sctp|vxlan) (hw|sw) (port-id)" command is not easy to understand and extend, so the patch set enhances the tx_checksum command and reworks csum forwarding engine due to the change of tx_checksum command. 
The main changes of the tx_checksum command are listed below,

1> add "tx_checksum set tunnel (hw|sw|none) (port-id)" command

The command is used to set/clear tunnel flag that is used to tell the NIC that the packetg is a tunneing packet and application want hardware TX checksum offload for outer layer, or inner layer, or both.

The 'none' option means that user treat tunneling packet as ordinary packet when using the csum forward engine.
for example, let say we have a tunnel packet: eth_hdr_out/ipv4_hdr_out/udp_hdr_out/vxlan_hdr/ehtr_hdr_in/ipv4_hdr_in/tcp_hdr_in. one of several scenarios:

1) User requests HW offload for ipv4_hdr_out  checksum, and doesn't care is it a tunnelled packet or not. So he sets:

tx_checksum set tunnel none 0

tx_checksum set ip hw 0

So for such case we should set tx_tunnel to 'none'.

2> add "tx_checksum set outer-ip (hw|sw) (port-id)" command

The command is used to set/clear outer IP flag that is used to tell the NIC that application want hardware offload for outer layer.

3> remove the 'vxlan' option from the  "tx_checksum set (ip|udp|tcp|sctp|vxlan) (hw|sw) (port-id)" command

The command is used to set IP, UDP, TCP and SCTP TX checksum flag. In the case of tunneling packet, the IP, UDP, TCP and SCTP flags always concern inner layer.
 
Moreover, replace the TESTPMD_TX_OFFLOAD_VXLAN_CKSUM flag with TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM flag and add the TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM and TESTPMD_TX_OFFLOAD_NON_TUNNEL_CKSUM flag in test-pmd application.

v2 change:
     redefine the 'none' behaviour for "tx_checksum set tunnel (hw|sw|none) (port-id)" command.
v3 change:
     typo correction in cmdline help 

Jijiang Liu (3):
  add outer IP offload capability in librte_ether.
  add outer IP checksum capability in i40e PMD
  testpmd command lines of the tx_checksum and csum forwarding rework

 app/test-pmd/cmdline.c            |  201 +++++++++++++++++++++++++++++++++++--
 app/test-pmd/csumonly.c           |   35 ++++---
 app/test-pmd/testpmd.h            |    6 +-
 lib/librte_ether/rte_ethdev.h     |    1 +
 lib/librte_pmd_i40e/i40e_ethdev.c |    3 +-
 5 files changed, 218 insertions(+), 28 deletions(-)

-- 
1.7.7.6

[dpdk-dev] [PATCH v3 1/3] librte_ether:add outer IP offload capability flag

[Jijiang Liu]

If the flag is set in a PMD, which means the NIC(s) support TX checksum offload of tunneling packet.

Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
---
 lib/librte_ether/rte_ethdev.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index f66805d..bae59c3 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -916,6 +916,7 @@ struct rte_eth_conf {
 #define DEV_TX_OFFLOAD_SCTP_CKSUM  0x00000010
 #define DEV_TX_OFFLOAD_TCP_TSO     0x00000020
 #define DEV_TX_OFFLOAD_UDP_TSO     0x00000040
+#define DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM 0x00000080 /**< Used for tunneling packet. */
 
 struct rte_eth_dev_info {
 	struct rte_pci_device *pci_dev; /**< Device PCI information. */
-- 
1.7.7.6

Re: [dpdk-dev] [PATCH v3 1/3] librte_ether:add outer IP offload capability flag

[Olivier MATZ]

On 12/10/2014 02:03 AM, Jijiang Liu wrote:
> If the flag is set in a PMD, which means the NIC(s) support TX checksum offload of tunneling packet.
>
> Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
> ---
>   lib/librte_ether/rte_ethdev.h |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index f66805d..bae59c3 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -916,6 +916,7 @@ struct rte_eth_conf {
>   #define DEV_TX_OFFLOAD_SCTP_CKSUM  0x00000010
>   #define DEV_TX_OFFLOAD_TCP_TSO     0x00000020
>   #define DEV_TX_OFFLOAD_UDP_TSO     0x00000040
> +#define DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM 0x00000080 /**< Used for tunneling packet. */
>
>   struct rte_eth_dev_info {
>   	struct rte_pci_device *pci_dev; /**< Device PCI information. */
>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

(maybe there's an indent issue, Thomas, could you check when applying
the patch?)

[dpdk-dev] [PATCH v3 2/3] i40e:support outer IPv4 checksum capability

[Jijiang Liu]

The DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM flag is added in i40e capability set, which means the i40e supports TX checksum offload of tunneling packet.

Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
---
 lib/librte_pmd_i40e/i40e_ethdev.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c b/lib/librte_pmd_i40e/i40e_ethdev.c
index 87e750a..deddd0b 100644
--- a/lib/librte_pmd_i40e/i40e_ethdev.c
+++ b/lib/librte_pmd_i40e/i40e_ethdev.c
@@ -1491,7 +1491,8 @@ i40e_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 		DEV_TX_OFFLOAD_IPV4_CKSUM |
 		DEV_TX_OFFLOAD_UDP_CKSUM |
 		DEV_TX_OFFLOAD_TCP_CKSUM |
-		DEV_TX_OFFLOAD_SCTP_CKSUM;
+		DEV_TX_OFFLOAD_SCTP_CKSUM |
+		DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM;
 	dev_info->reta_size = pf->hash_lut_size;
 
 	dev_info->default_rxconf = (struct rte_eth_rxconf) {
-- 
1.7.7.6

Re: [dpdk-dev] [PATCH v3 2/3] i40e:support outer IPv4 checksum capability

[Olivier MATZ]

On 12/10/2014 02:03 AM, Jijiang Liu wrote:
> The DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM flag is added in i40e capability set, which means the i40e supports TX checksum offload of tunneling packet.
>
> Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
> ---
>   lib/librte_pmd_i40e/i40e_ethdev.c |    3 ++-
>   1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c b/lib/librte_pmd_i40e/i40e_ethdev.c
> index 87e750a..deddd0b 100644
> --- a/lib/librte_pmd_i40e/i40e_ethdev.c
> +++ b/lib/librte_pmd_i40e/i40e_ethdev.c
> @@ -1491,7 +1491,8 @@ i40e_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>   		DEV_TX_OFFLOAD_IPV4_CKSUM |
>   		DEV_TX_OFFLOAD_UDP_CKSUM |
>   		DEV_TX_OFFLOAD_TCP_CKSUM |
> -		DEV_TX_OFFLOAD_SCTP_CKSUM;
> +		DEV_TX_OFFLOAD_SCTP_CKSUM |
> +		DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM;
>   	dev_info->reta_size = pf->hash_lut_size;
>
>   	dev_info->default_rxconf = (struct rte_eth_rxconf) {
>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

[dpdk-dev] [PATCH v3 3/3] app/testpmd:change tx_checksum command and csum forwarding engine

[Jijiang Liu]

The patch enhances the tx_checksum command and reworks csum forwarding engine due to the change of tx_checksum command.
The main changes of the tx_checksum command are listed below,
 
1. add "tx_checksum set tunnel (hw|sw|none) (port-id)" command
 
2. add "tx_checksum set outer-ip (hw|sw) (port-id)" command
 
3. remove the "vxlan" option from the "tx_checksum set(ip|udp|tcp|sctp|vxlan) (hw|sw) (port-id)" command
 
Moreover, replace the TESTPMD_TX_OFFLOAD_VXLAN_CKSUM flag with TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM flag, and add the TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM and TESTPMD_TX_OFFLOAD_NON_TUNNEL_CKSUM flag.


Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
---
 app/test-pmd/cmdline.c  |  209 ++++++++++++++++++++++++++++++++++++++++++++---
 app/test-pmd/csumonly.c |   38 ++++++---
 app/test-pmd/testpmd.h  |   14 +++-
 3 files changed, 234 insertions(+), 27 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index f79ea3e..9bfa9ef 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -316,16 +316,30 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"    Disable hardware insertion of a VLAN header in"
 			" packets sent on a port.\n\n"
 
-			"tx_cksum set (ip|udp|tcp|sctp|vxlan) (hw|sw) (port_id)\n"
+			"tx_cksum set (ip|udp|tcp|sctp) (hw|sw) (port_id)\n"
 			"    Select hardware or software calculation of the"
 			" checksum with when transmitting a packet using the"
 			" csum forward engine.\n"
-			"    ip|udp|tcp|sctp always concern the inner layer.\n"
-			"    vxlan concerns the outer IP and UDP layer (in"
-			" case the packet is recognized as a vxlan packet by"
-			" the forward engine)\n"
+			"    In the case of tunneling packet, ip|udp|tcp|sctp"
+			" always concern the inner layer.\n\n"
+
+			"tx_cksum set tunnel (hw|sw|none) (port_id)\n"
+			" Select hardware or software calculation of the"
+			" checksum with when transmitting a tunneling packet"
+			" using the csum forward engine.\n"
+			" The none option means treat tunneling packet as ordinary"
+			" packet when using the csum forward engine\n."
+			"    Tunneling packet concerns the outer IP, inner IP"
+			" and inner L4\n"
 			"    Please check the NIC datasheet for HW limits.\n\n"
 
+			"tx_cksum set (outer-ip) (hw|sw) (port_id)\n"
+			"    Select hardware or software calculation of the"
+			" checksum with when transmitting a packet using the"
+			" csum forward engine.\n"
+			"    outer-ip always concern the outer layer of"
+			" tunneling packet.\n\n"
+
 			"tx_checksum show (port_id)\n"
 			"    Display tx checksum offload configuration\n\n"
 
@@ -2861,6 +2875,181 @@ cmdline_parse_inst_t cmd_tx_vlan_reset = {
 	},
 };
 
+/* ENABLE HARDWARE INSERTION OF CHECKSUM IN TX PACKETS FOR TUNNELING */
+struct cmd_tx_cksum_tunnel_result {
+	cmdline_fixed_string_t tx_cksum;
+	cmdline_fixed_string_t mode;
+	cmdline_fixed_string_t type;
+	cmdline_fixed_string_t hwsw;
+	uint8_t port_id;
+};
+
+static void
+cmd_tx_cksum_tunnel_parsed(void *parsed_result,
+		       __attribute__((unused)) struct cmdline *cl,
+		       __attribute__((unused)) void *data)
+{
+	struct cmd_tx_cksum_tunnel_result *res = parsed_result;
+	int hw = 0;
+	uint16_t ol_flags, mask = 0;
+
+	if (port_id_is_invalid(res->port_id)) {
+		printf("invalid port %d\n", res->port_id);
+		return;
+	}
+
+	if (!strcmp(res->mode, "set")) {
+
+		if (!strcmp(res->hwsw, "hw"))
+			hw = 1;
+		else if (!strcmp(res->hwsw, "none")) {
+			ports[res->port_id].tx_ol_flags &=
+				~(TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM
+				| TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM);
+			ports[res->port_id].tx_ol_flags |=
+				TESTPMD_TX_OFFLOAD_NON_TUNNEL_CKSUM;
+			return;
+		}
+
+		ports[res->port_id].tx_ol_flags &=
+				~TESTPMD_TX_OFFLOAD_NON_TUNNEL_CKSUM;
+		mask = TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM;
+
+		if (hw)
+			ports[res->port_id].tx_ol_flags |= mask;
+		else
+			ports[res->port_id].tx_ol_flags &= (~mask);
+	}
+
+	ol_flags = ports[res->port_id].tx_ol_flags;
+	printf("Tunnel checksum offload is %s\n",
+		(ol_flags & TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM) ? "hw" : "sw");
+}
+
+cmdline_parse_token_string_t cmd_tx_cksum_tunnel_tx_cksum =
+	TOKEN_STRING_INITIALIZER(struct cmd_tx_cksum_tunnel_result,
+				tx_cksum, "tx_checksum");
+cmdline_parse_token_string_t cmd_tx_cksum_tunnel_mode =
+	TOKEN_STRING_INITIALIZER(struct cmd_tx_cksum_tunnel_result,
+				mode, "set");
+cmdline_parse_token_string_t cmd_tx_cksum_tunnel_type =
+	TOKEN_STRING_INITIALIZER(struct cmd_tx_cksum_tunnel_result,
+				type, "tunnel");
+cmdline_parse_token_string_t cmd_tx_cksum_tunnel_hwsw =
+	TOKEN_STRING_INITIALIZER(struct cmd_tx_cksum_tunnel_result,
+				hwsw, "hw#sw#none");
+cmdline_parse_token_num_t cmd_tx_cksum_tunnel_portid =
+	TOKEN_NUM_INITIALIZER(struct cmd_tx_cksum_tunnel_result,
+				port_id, UINT8);
+
+cmdline_parse_inst_t cmd_tx_cksum_tunnel_set = {
+	.f = cmd_tx_cksum_tunnel_parsed,
+	.data = NULL,
+	.help_str = "enable/disable hardware calculation of tunneling "
+		"packet checksum when using csum forward engine: tx_cksum set "
+		"tunnel hw|sw|none <port>",
+	.tokens = {
+		(void *)&cmd_tx_cksum_tunnel_tx_cksum,
+		(void *)&cmd_tx_cksum_tunnel_mode,
+		(void *)&cmd_tx_cksum_tunnel_type,
+		(void *)&cmd_tx_cksum_tunnel_hwsw,
+		(void *)&cmd_tx_cksum_tunnel_portid,
+		NULL,
+	},
+};
+
+/* ENABLE HARDWARE INSERTION OF OUTER CHECKSUM IN TX PACKETS FOR TUNNELING  */
+struct cmd_tx_cksum_outer_result {
+	cmdline_fixed_string_t tx_cksum;
+	cmdline_fixed_string_t mode;
+	cmdline_fixed_string_t proto;
+	cmdline_fixed_string_t hwsw;
+	uint8_t port_id;
+};
+
+static void
+cmd_tx_cksum_outer_parsed(void *parsed_result,
+		       __attribute__((unused)) struct cmdline *cl,
+		       __attribute__((unused)) void *data)
+{
+	struct cmd_tx_cksum_outer_result *res = parsed_result;
+	int hw = 0;
+	uint16_t ol_flags, mask = 0;
+	struct rte_eth_dev_info dev_info;
+
+	if (port_id_is_invalid(res->port_id)) {
+		printf("invalid port %d\n", res->port_id);
+		return;
+	}
+
+	if (!strcmp(res->mode, "set")) {
+
+		if (!strcmp(res->hwsw, "hw"))
+			hw = 1;
+
+		if (!strcmp(res->proto, "outer-ip")) {
+			if (ports[res->port_id].tx_ol_flags &
+				TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM)
+				mask = TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM;
+			else {
+				printf("Tunnel checksum flag must be"
+					" configured before enabling HW"
+					" outer IP checksum\n");
+				return;
+			}
+		}
+
+		if (hw)
+			ports[res->port_id].tx_ol_flags |= mask;
+		else
+			ports[res->port_id].tx_ol_flags &= (~mask);
+	}
+
+	ol_flags = ports[res->port_id].tx_ol_flags;
+	printf("Outer IP checksum offload is %s\n",
+		(ol_flags & TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM) ? "hw" : "sw");
+
+	/* display warnings if configuration is not supported by the NIC */
+	rte_eth_dev_info_get(res->port_id, &dev_info);
+	if ((ol_flags & TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM) &&
+		(dev_info.tx_offload_capa & DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM)
+			== 0) {
+		printf("Warning: hardware Outer IP checksum enabled but not "
+			"supported by port %d\n", res->port_id);
+	}
+}
+
+cmdline_parse_token_string_t cmd_tx_cksum_outer_tx_cksum =
+	TOKEN_STRING_INITIALIZER(struct cmd_tx_cksum_outer_result,
+				tx_cksum, "tx_checksum");
+cmdline_parse_token_string_t cmd_tx_cksum_outer_mode =
+	TOKEN_STRING_INITIALIZER(struct cmd_tx_cksum_outer_result,
+				mode, "set");
+cmdline_parse_token_string_t cmd_tx_cksum_outer_proto =
+	TOKEN_STRING_INITIALIZER(struct cmd_tx_cksum_outer_result,
+				proto, "outer-ip");
+cmdline_parse_token_string_t cmd_tx_cksum_outer_hwsw =
+	TOKEN_STRING_INITIALIZER(struct cmd_tx_cksum_outer_result,
+				hwsw, "hw#sw");
+cmdline_parse_token_num_t cmd_tx_cksum_outer_portid =
+	TOKEN_NUM_INITIALIZER(struct cmd_tx_cksum_outer_result,
+				port_id, UINT8);
+
+cmdline_parse_inst_t cmd_tx_cksum_outer_set = {
+	.f = cmd_tx_cksum_outer_parsed,
+	.data = NULL,
+	.help_str = "enable/disable hardware calculation of outer L3"
+		" checksum for tunneling packet when using csum forward"
+		" engine:tx_cksum set outer-ip hw|sw <port>",
+	.tokens = {
+		(void *)&cmd_tx_cksum_outer_tx_cksum,
+		(void *)&cmd_tx_cksum_outer_mode,
+		(void *)&cmd_tx_cksum_outer_proto,
+		(void *)&cmd_tx_cksum_outer_hwsw,
+		(void *)&cmd_tx_cksum_outer_portid,
+		NULL,
+	},
+};
 
 /* *** ENABLE HARDWARE INSERTION OF CHECKSUM IN TX PACKETS *** */
 struct cmd_tx_cksum_result {
@@ -2899,8 +3088,6 @@ cmd_tx_cksum_parsed(void *parsed_result,
 			mask = TESTPMD_TX_OFFLOAD_TCP_CKSUM;
 		} else if (!strcmp(res->proto, "sctp")) {
 			mask = TESTPMD_TX_OFFLOAD_SCTP_CKSUM;
-		} else if (!strcmp(res->proto, "vxlan")) {
-			mask = TESTPMD_TX_OFFLOAD_VXLAN_CKSUM;
 		}
 
 		if (hw)
@@ -2918,8 +3105,6 @@ cmd_tx_cksum_parsed(void *parsed_result,
 		(ol_flags & TESTPMD_TX_OFFLOAD_TCP_CKSUM) ? "hw" : "sw");
 	printf("SCTP checksum offload is %s\n",
 		(ol_flags & TESTPMD_TX_OFFLOAD_SCTP_CKSUM) ? "hw" : "sw");
-	printf("VxLAN checksum offload is %s\n",
-		(ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM) ? "hw" : "sw");
 
 	/* display warnings if configuration is not supported by the NIC */
 	rte_eth_dev_info_get(res->port_id, &dev_info);
@@ -2953,7 +3138,7 @@ cmdline_parse_token_string_t cmd_tx_cksum_mode =
 				mode, "set");
 cmdline_parse_token_string_t cmd_tx_cksum_proto =
 	TOKEN_STRING_INITIALIZER(struct cmd_tx_cksum_result,
-				proto, "ip#tcp#udp#sctp#vxlan");
+				proto, "ip#tcp#udp#sctp");
 cmdline_parse_token_string_t cmd_tx_cksum_hwsw =
 	TOKEN_STRING_INITIALIZER(struct cmd_tx_cksum_result,
 				hwsw, "hw#sw");
@@ -2965,7 +3150,7 @@ cmdline_parse_inst_t cmd_tx_cksum_set = {
 	.f = cmd_tx_cksum_parsed,
 	.data = NULL,
 	.help_str = "enable/disable hardware calculation of L3/L4 checksum when "
-		"using csum forward engine: tx_cksum set ip|tcp|udp|sctp|vxlan hw|sw <port>",
+		"using csum forward engine: tx_cksum set ip|tcp|udp|sctp hw|sw <port>",
 	.tokens = {
 		(void *)&cmd_tx_cksum_tx_cksum,
 		(void *)&cmd_tx_cksum_mode,
@@ -8749,6 +8934,8 @@ cmdline_parse_ctx_t main_ctx[] = {
 	(cmdline_parse_inst_t *)&cmd_tx_vlan_reset,
 	(cmdline_parse_inst_t *)&cmd_tx_vlan_set_pvid,
 	(cmdline_parse_inst_t *)&cmd_tx_cksum_set,
+	(cmdline_parse_inst_t *)&cmd_tx_cksum_tunnel_set,
+	(cmdline_parse_inst_t *)&cmd_tx_cksum_outer_set,
 	(cmdline_parse_inst_t *)&cmd_tx_cksum_show,
 	(cmdline_parse_inst_t *)&cmd_tso_set,
 	(cmdline_parse_inst_t *)&cmd_tso_show,
diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index 41711fd..fa07b1f 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -256,17 +256,16 @@ process_outer_cksums(void *outer_l3_hdr, uint16_t outer_ethertype,
 	struct udp_hdr *udp_hdr;
 	uint64_t ol_flags = 0;
 
-	if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM)
-		ol_flags |= PKT_TX_UDP_TUNNEL_PKT;
-
 	if (outer_ethertype == _htons(ETHER_TYPE_IPv4)) {
 		ipv4_hdr->hdr_checksum = 0;
 
-		if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM)
+		if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM)
 			ol_flags |= PKT_TX_OUTER_IP_CKSUM;
-		else
+		else {
 			ipv4_hdr->hdr_checksum = rte_ipv4_cksum(ipv4_hdr);
-	} else if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM)
+			ol_flags |= PKT_TX_OUTER_IPV4;
+		}
+	} else if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM)
 		ol_flags |= PKT_TX_OUTER_IPV6;
 
 	udp_hdr = (struct udp_hdr *)((char *)outer_l3_hdr + outer_l3_len);
@@ -300,11 +299,14 @@ process_outer_cksums(void *outer_l3_hdr, uint16_t outer_ethertype,
  *   Ether / (vlan) / outer IP|IP6 / outer UDP / VxLAN / Ether / IP|IP6 /
  *           UDP|TCP|SCTP
  *
- * The testpmd command line for this forward engine sets the flags
+ * These testpmd command lines for this forward engine sets the flags
  * TESTPMD_TX_OFFLOAD_* in ports[tx_port].tx_ol_flags. They control
- * wether a checksum must be calculated in software or in hardware. The
- * IP, UDP, TCP and SCTP flags always concern the inner layer.  The
- * VxLAN flag concerns the outer IP (if packet is recognized as a vxlan packet).
+ * wether a checksum must be calculated in software or in hardware.
+ * In the case of tunneling packet, the IP, UDP, TCP and SCTP flags
+ * always concern the inner layer; the outer IP flag always concern
+ * the outer layer; the tunnel flag is used to tell the NIC that it
+ * is a tunneing packet, want hardware offload for outer layer,
+ * or inner layer, or both.
  */
 static void
 pkt_burst_checksum_forward(struct fwd_stream *fs)
@@ -376,7 +378,9 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
 		l3_hdr = (char *)eth_hdr + l2_len;
 
 		/* check if it's a supported tunnel (only vxlan for now) */
-		if (l4_proto == IPPROTO_UDP) {
+		if (((testpmd_ol_flags &
+			TESTPMD_TX_OFFLOAD_NON_TUNNEL_CKSUM) == 0)
+			&& (l4_proto == IPPROTO_UDP)) {
 			udp_hdr = (struct udp_hdr *)((char *)l3_hdr + l3_len);
 
 			/* check udp destination port, 4789 is the default
@@ -386,17 +390,23 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
 				tunnel = 1;
 
 			/* currently, this flag is set by i40e only if the
-			 * packet is vxlan */
+			 * packet is a tunneling packet */
 			} else if (m->ol_flags & (PKT_RX_TUNNEL_IPV4_HDR |
 					PKT_RX_TUNNEL_IPV6_HDR))
 				tunnel = 1;
 
 			if (tunnel == 1) {
+
+				if (testpmd_ol_flags
+					& TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM)
+					ol_flags |= PKT_TX_UDP_TUNNEL_PKT;
+
 				outer_ethertype = ethertype;
 				outer_l2_len = l2_len;
 				outer_l3_len = l3_len;
 				outer_l3_hdr = l3_hdr;
 
+				/* currently, only VXLAN packet is supported */
 				eth_hdr = (struct ether_hdr *)((char *)udp_hdr +
 					sizeof(struct udp_hdr) +
 					sizeof(struct vxlan_hdr));
@@ -434,7 +444,7 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
 		/* step 4: fill the mbuf meta data (flags and header lengths) */
 
 		if (tunnel == 1) {
-			if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM) {
+			if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM) {
 				m->outer_l2_len = outer_l2_len;
 				m->outer_l3_len = outer_l3_len;
 				m->l2_len = l4_tun_len + l2_len;
@@ -505,7 +515,7 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
 					"m->l4_len=%d\n",
 					m->l2_len, m->l3_len, m->l4_len);
 			if ((tunnel == 1) &&
-				(testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM))
+				(testpmd_ol_flags & TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM))
 				printf("tx: m->outer_l2_len=%d m->outer_l3_len=%d\n",
 					m->outer_l2_len, m->outer_l3_len);
 			if (tso_segsz != 0)
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index f8b0740..09caa6a 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -125,10 +125,20 @@ struct fwd_stream {
 #define TESTPMD_TX_OFFLOAD_TCP_CKSUM         0x0004
 /** Offload SCTP checksum in csum forward engine */
 #define TESTPMD_TX_OFFLOAD_SCTP_CKSUM        0x0008
-/** Offload VxLAN checksum in csum forward engine */
-#define TESTPMD_TX_OFFLOAD_VXLAN_CKSUM       0x0010
+/** Offload tunneling packet checksum in csum forward engine */
+#define TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM      0x0010
 /** Insert VLAN header in forward engine */
 #define TESTPMD_TX_OFFLOAD_INSERT_VLAN       0x0020
+/**
+ * Offload outer-IP checksum in csum forward engine
+ * for tunneling packet
+ */
+#define TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM    0x0040
+/**
+ * For a tunneling packet, user requests HW offload for its outer
+ * layer checksum, and don't care is it a tunneled packet or not.
+ */
+#define TESTPMD_TX_OFFLOAD_NON_TUNNEL_CKSUM  0x0080
 
 /**
  * The data structure associated with each port.
-- 
1.7.7.6

Re: [dpdk-dev] [PATCH v3 3/3] app/testpmd:change tx_checksum command and csum forwarding engine

[Olivier MATZ]

Hi Jijiang,

Some more comments, in addition to the one I've made in the cover
letter. Reference link for patchwork readers:
http://dpdk.org/ml/archives/dev/2014-December/009886.html

On 12/10/2014 02:03 AM, Jijiang Liu wrote:
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -316,16 +316,30 @@ static void cmd_help_long_parsed(void *parsed_result,
>   			"    Disable hardware insertion of a VLAN header in"
>   			" packets sent on a port.\n\n"
>
> -			"tx_cksum set (ip|udp|tcp|sctp|vxlan) (hw|sw) (port_id)\n"
> +			"tx_cksum set (ip|udp|tcp|sctp) (hw|sw) (port_id)\n"
>   			"    Select hardware or software calculation of the"
>   			" checksum with when transmitting a packet using the"
>   			" csum forward engine.\n"
> -			"    ip|udp|tcp|sctp always concern the inner layer.\n"
> -			"    vxlan concerns the outer IP and UDP layer (in"
> -			" case the packet is recognized as a vxlan packet by"
> -			" the forward engine)\n"
> +			"    In the case of tunneling packet, ip|udp|tcp|sctp"
> +			" always concern the inner layer.\n\n"
> +
> +			"tx_cksum set tunnel (hw|sw|none) (port_id)\n"
> +			" Select hardware or software calculation of the"
> +			" checksum with when transmitting a tunneling packet"
> +			" using the csum forward engine.\n"
> +			" The none option means treat tunneling packet as ordinary"
> +			" packet when using the csum forward engine\n."
> +			"    Tunneling packet concerns the outer IP, inner IP"
> +			" and inner L4\n"
>   			"    Please check the NIC datasheet for HW limits.\n\n"
>
> +			"tx_cksum set (outer-ip) (hw|sw) (port_id)\n"
> +			"    Select hardware or software calculation of the"
> +			" checksum with when transmitting a packet using the"
> +			" csum forward engine.\n"
> +			"    outer-ip always concern the outer layer of"
> +			" tunneling packet.\n\n"
> +
>   			"tx_checksum show (port_id)\n"
>   			"    Display tx checksum offload configuration\n\n"
>

not sure we need 2 different commands for tx_cksum set (outer-ip) and
tx_cksum set (ip|udp|tcp|sctp). As the syntax is exactly the same, it
may result in less code to have only one command.


> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -256,17 +256,16 @@ process_outer_cksums(void *outer_l3_hdr, uint16_t outer_ethertype,
>   	struct udp_hdr *udp_hdr;
>   	uint64_t ol_flags = 0;
>
> -	if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM)
> -		ol_flags |= PKT_TX_UDP_TUNNEL_PKT;
> -
>   	if (outer_ethertype == _htons(ETHER_TYPE_IPv4)) {
>   		ipv4_hdr->hdr_checksum = 0;
>
> -		if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM)
> +		if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM)
>   			ol_flags |= PKT_TX_OUTER_IP_CKSUM;
> -		else
> +		else {
>   			ipv4_hdr->hdr_checksum = rte_ipv4_cksum(ipv4_hdr);
> -	} else if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM)
> +			ol_flags |= PKT_TX_OUTER_IPV4;
> +		}
> +	} else if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM)
>   		ol_flags |= PKT_TX_OUTER_IPV6;
>
>   	udp_hdr = (struct udp_hdr *)((char *)outer_l3_hdr + outer_l3_len);
> @@ -300,11 +299,14 @@ process_outer_cksums(void *outer_l3_hdr, uint16_t outer_ethertype,
>    *   Ether / (vlan) / outer IP|IP6 / outer UDP / VxLAN / Ether / IP|IP6 /
>    *           UDP|TCP|SCTP
>    *
> - * The testpmd command line for this forward engine sets the flags
> + * These testpmd command lines for this forward engine sets the flags
>    * TESTPMD_TX_OFFLOAD_* in ports[tx_port].tx_ol_flags. They control
> - * wether a checksum must be calculated in software or in hardware. The
> - * IP, UDP, TCP and SCTP flags always concern the inner layer.  The
> - * VxLAN flag concerns the outer IP (if packet is recognized as a vxlan packet).
> + * wether a checksum must be calculated in software or in hardware.
> + * In the case of tunneling packet, the IP, UDP, TCP and SCTP flags
> + * always concern the inner layer; the outer IP flag always concern
> + * the outer layer; the tunnel flag is used to tell the NIC that it
> + * is a tunneing packet, want hardware offload for outer layer,
> + * or inner layer, or both.

tunneing -> tunneling

"the tunnel flag is used to tell the NIC that it is a tunneing packet,
want hardware offload for outer layer, or inner layer, or both."

what does that mean?


>    */
>   static void
>   pkt_burst_checksum_forward(struct fwd_stream *fs)
> @@ -376,7 +378,9 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
>   		l3_hdr = (char *)eth_hdr + l2_len;
>
>   		/* check if it's a supported tunnel (only vxlan for now) */
> -		if (l4_proto == IPPROTO_UDP) {
> +		if (((testpmd_ol_flags &
> +			TESTPMD_TX_OFFLOAD_NON_TUNNEL_CKSUM) == 0)
> +			&& (l4_proto == IPPROTO_UDP)) {
>   			udp_hdr = (struct udp_hdr *)((char *)l3_hdr + l3_len);
>
>   			/* check udp destination port, 4789 is the default
> @@ -386,17 +390,23 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
>   				tunnel = 1;
>
>   			/* currently, this flag is set by i40e only if the
> -			 * packet is vxlan */
> +			 * packet is a tunneling packet */
>   			} else if (m->ol_flags & (PKT_RX_TUNNEL_IPV4_HDR |
>   					PKT_RX_TUNNEL_IPV6_HDR))
>   				tunnel = 1;
>
>   			if (tunnel == 1) {
> +
> +				if (testpmd_ol_flags
> +					& TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM)
> +					ol_flags |= PKT_TX_UDP_TUNNEL_PKT;
> +
>   				outer_ethertype = ethertype;
>   				outer_l2_len = l2_len;
>   				outer_l3_len = l3_len;
>   				outer_l3_hdr = l3_hdr;
>
> +				/* currently, only VXLAN packet is supported */
>   				eth_hdr = (struct ether_hdr *)((char *)udp_hdr +
>   					sizeof(struct udp_hdr) +
>   					sizeof(struct vxlan_hdr));
> @@ -434,7 +444,7 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
>   		/* step 4: fill the mbuf meta data (flags and header lengths) */
>
>   		if (tunnel == 1) {
> -			if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM) {
> +			if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM) {
>   				m->outer_l2_len = outer_l2_len;
>   				m->outer_l3_len = outer_l3_len;
>   				m->l2_len = l4_tun_len + l2_len;
> @@ -505,7 +515,7 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
>   					"m->l4_len=%d\n",
>   					m->l2_len, m->l3_len, m->l4_len);
>   			if ((tunnel == 1) &&
> -				(testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM))
> +				(testpmd_ol_flags & TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM))
>   				printf("tx: m->outer_l2_len=%d m->outer_l3_len=%d\n",
>   					m->outer_l2_len, m->outer_l3_len);
>   			if (tso_segsz != 0)
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index f8b0740..09caa6a 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -125,10 +125,20 @@ struct fwd_stream {
>   #define TESTPMD_TX_OFFLOAD_TCP_CKSUM         0x0004
>   /** Offload SCTP checksum in csum forward engine */
>   #define TESTPMD_TX_OFFLOAD_SCTP_CKSUM        0x0008
> -/** Offload VxLAN checksum in csum forward engine */
> -#define TESTPMD_TX_OFFLOAD_VXLAN_CKSUM       0x0010
> +/** Offload tunneling packet checksum in csum forward engine */
> +#define TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM      0x0010
>   /** Insert VLAN header in forward engine */
>   #define TESTPMD_TX_OFFLOAD_INSERT_VLAN       0x0020
> +/**
> + * Offload outer-IP checksum in csum forward engine
> + * for tunneling packet
> + */
> +#define TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM    0x0040
> +/**
> + * For a tunneling packet, user requests HW offload for its outer
> + * layer checksum, and don't care is it a tunneled packet or not.
> + */
> +#define TESTPMD_TX_OFFLOAD_NON_TUNNEL_CKSUM  0x0080
>
>   /**
>    * The data structure associated with each port.
>

For now, I did not check the implementation of the patch. I'm not
sure I understand the specifications, so I cannot check if the code
conforms to it.

How this patch is tested? I think a report similar to
http://dpdk.org/ml/archives/dev/2014-November/007991.html would help
to verify that it works (at least on one driver), and to understand
the test-pmd API and the different use cases.

Regards,
Olivier

Re: [dpdk-dev] [PATCH v3 3/3] app/testpmd:change tx_checksum command and csum forwarding engine

[Liu, Jijiang]

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Thursday, December 11, 2014 6:53 PM
> To: Liu, Jijiang; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 3/3] app/testpmd:change tx_checksum
> command and csum forwarding engine
> 
> Hi Jijiang,
> 
> Some more comments, in addition to the one I've made in the cover letter.
> Reference link for patchwork readers:
> http://dpdk.org/ml/archives/dev/2014-December/009886.html
> 
> On 12/10/2014 02:03 AM, Jijiang Liu wrote:
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -316,16 +316,30 @@ static void cmd_help_long_parsed(void
> *parsed_result,
> >   			"    Disable hardware insertion of a VLAN header in"
> >   			" packets sent on a port.\n\n"
> >
> > -			"tx_cksum set (ip|udp|tcp|sctp|vxlan) (hw|sw)
> (port_id)\n"
> > +			"tx_cksum set (ip|udp|tcp|sctp) (hw|sw) (port_id)\n"
> >   			"    Select hardware or software calculation of the"
> >   			" checksum with when transmitting a packet using the"
> >   			" csum forward engine.\n"
> > -			"    ip|udp|tcp|sctp always concern the inner layer.\n"
> > -			"    vxlan concerns the outer IP and UDP layer (in"
> > -			" case the packet is recognized as a vxlan packet by"
> > -			" the forward engine)\n"
> > +			"    In the case of tunneling packet, ip|udp|tcp|sctp"
> > +			" always concern the inner layer.\n\n"
> > +
> > +			"tx_cksum set tunnel (hw|sw|none) (port_id)\n"
> > +			" Select hardware or software calculation of the"
> > +			" checksum with when transmitting a tunneling packet"
> > +			" using the csum forward engine.\n"
> > +			" The none option means treat tunneling packet as
> ordinary"
> > +			" packet when using the csum forward engine\n."
> > +			"    Tunneling packet concerns the outer IP, inner IP"
> > +			" and inner L4\n"
> >   			"    Please check the NIC datasheet for HW limits.\n\n"
> >
> > +			"tx_cksum set (outer-ip) (hw|sw) (port_id)\n"
> > +			"    Select hardware or software calculation of the"
> > +			" checksum with when transmitting a packet using the"
> > +			" csum forward engine.\n"
> > +			"    outer-ip always concern the outer layer of"
> > +			" tunneling packet.\n\n"
> > +
> >   			"tx_checksum show (port_id)\n"
> >   			"    Display tx checksum offload configuration\n\n"
> >
> 
> not sure we need 2 different commands for tx_cksum set (outer-ip) and tx_cksum
> set (ip|udp|tcp|sctp). As the syntax is exactly the same, it may result in less code
> to have only one command.

Why do we have a separate command for outer layer, I have explained this in other mail.
Do you agree on this?
 
> 
> > --- a/app/test-pmd/csumonly.c
> > +++ b/app/test-pmd/csumonly.c
> > @@ -256,17 +256,16 @@ process_outer_cksums(void *outer_l3_hdr, uint16_t
> outer_ethertype,
> >   	struct udp_hdr *udp_hdr;
> >   	uint64_t ol_flags = 0;
> >
> > -	if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM)
> > -		ol_flags |= PKT_TX_UDP_TUNNEL_PKT;
> > -
> >   	if (outer_ethertype == _htons(ETHER_TYPE_IPv4)) {
> >   		ipv4_hdr->hdr_checksum = 0;
> >
> > -		if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM)
> > +		if (testpmd_ol_flags &
> TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM)
> >   			ol_flags |= PKT_TX_OUTER_IP_CKSUM;
> > -		else
> > +		else {
> >   			ipv4_hdr->hdr_checksum = rte_ipv4_cksum(ipv4_hdr);
> > -	} else if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM)
> > +			ol_flags |= PKT_TX_OUTER_IPV4;
> > +		}
> > +	} else if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM)
> >   		ol_flags |= PKT_TX_OUTER_IPV6;
> >
> >   	udp_hdr = (struct udp_hdr *)((char *)outer_l3_hdr + outer_l3_len);
> > @@ -300,11 +299,14 @@ process_outer_cksums(void *outer_l3_hdr, uint16_t
> outer_ethertype,
> >    *   Ether / (vlan) / outer IP|IP6 / outer UDP / VxLAN / Ether / IP|IP6 /
> >    *           UDP|TCP|SCTP
> >    *
> > - * The testpmd command line for this forward engine sets the flags
> > + * These testpmd command lines for this forward engine sets the flags
> >    * TESTPMD_TX_OFFLOAD_* in ports[tx_port].tx_ol_flags. They control
> > - * wether a checksum must be calculated in software or in hardware.
> > The
> > - * IP, UDP, TCP and SCTP flags always concern the inner layer.  The
> > - * VxLAN flag concerns the outer IP (if packet is recognized as a vxlan packet).
> > + * wether a checksum must be calculated in software or in hardware.
> > + * In the case of tunneling packet, the IP, UDP, TCP and SCTP flags
> > + * always concern the inner layer; the outer IP flag always concern
> > + * the outer layer; the tunnel flag is used to tell the NIC that it
> > + * is a tunneing packet, want hardware offload for outer layer,
> > + * or inner layer, or both.
> 
> tunneing -> tunneling
> 
> "the tunnel flag is used to tell the NIC that it is a tunneing packet, want hardware
> offload for outer layer, or inner layer, or both."
> 
> what does that mean?

Ok,
Will replace the above the description with the following:
The tunnel flag  is used to set/clear the flag of enabling TX tunneling packet checksum hardware offload in application.

> 
> 
> >    */
> >   static void
> >   pkt_burst_checksum_forward(struct fwd_stream *fs) @@ -376,7 +378,9
> > @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
> >   		l3_hdr = (char *)eth_hdr + l2_len;
> >
> >   		/* check if it's a supported tunnel (only vxlan for now) */
> > -		if (l4_proto == IPPROTO_UDP) {
> > +		if (((testpmd_ol_flags &
> > +			TESTPMD_TX_OFFLOAD_NON_TUNNEL_CKSUM) == 0)
> > +			&& (l4_proto == IPPROTO_UDP)) {
> >   			udp_hdr = (struct udp_hdr *)((char *)l3_hdr + l3_len);
> >
> >   			/* check udp destination port, 4789 is the default @@ -
> 386,17
> > +390,23 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
> >   				tunnel = 1;
> >
> >   			/* currently, this flag is set by i40e only if the
> > -			 * packet is vxlan */
> > +			 * packet is a tunneling packet */
> >   			} else if (m->ol_flags & (PKT_RX_TUNNEL_IPV4_HDR |
> >   					PKT_RX_TUNNEL_IPV6_HDR))
> >   				tunnel = 1;
> >
> >   			if (tunnel == 1) {
> > +
> > +				if (testpmd_ol_flags
> > +					&
> TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM)
> > +					ol_flags |= PKT_TX_UDP_TUNNEL_PKT;
> > +
> >   				outer_ethertype = ethertype;
> >   				outer_l2_len = l2_len;
> >   				outer_l3_len = l3_len;
> >   				outer_l3_hdr = l3_hdr;
> >
> > +				/* currently, only VXLAN packet is supported */
> >   				eth_hdr = (struct ether_hdr *)((char *)udp_hdr +
> >   					sizeof(struct udp_hdr) +
> >   					sizeof(struct vxlan_hdr));
> > @@ -434,7 +444,7 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
> >   		/* step 4: fill the mbuf meta data (flags and header lengths) */
> >
> >   		if (tunnel == 1) {
> > -			if (testpmd_ol_flags &
> TESTPMD_TX_OFFLOAD_VXLAN_CKSUM) {
> > +			if (testpmd_ol_flags &
> TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM) {
> >   				m->outer_l2_len = outer_l2_len;
> >   				m->outer_l3_len = outer_l3_len;
> >   				m->l2_len = l4_tun_len + l2_len; @@ -505,7
> +515,7 @@
> > pkt_burst_checksum_forward(struct fwd_stream *fs)
> >   					"m->l4_len=%d\n",
> >   					m->l2_len, m->l3_len, m->l4_len);
> >   			if ((tunnel == 1) &&
> > -				(testpmd_ol_flags &
> TESTPMD_TX_OFFLOAD_VXLAN_CKSUM))
> > +				(testpmd_ol_flags &
> TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM))
> >   				printf("tx: m->outer_l2_len=%d m-
> >outer_l3_len=%d\n",
> >   					m->outer_l2_len, m->outer_l3_len);
> >   			if (tso_segsz != 0)
> > diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index
> > f8b0740..09caa6a 100644
> > --- a/app/test-pmd/testpmd.h
> > +++ b/app/test-pmd/testpmd.h
> > @@ -125,10 +125,20 @@ struct fwd_stream {
> >   #define TESTPMD_TX_OFFLOAD_TCP_CKSUM         0x0004
> >   /** Offload SCTP checksum in csum forward engine */
> >   #define TESTPMD_TX_OFFLOAD_SCTP_CKSUM        0x0008
> > -/** Offload VxLAN checksum in csum forward engine */
> > -#define TESTPMD_TX_OFFLOAD_VXLAN_CKSUM       0x0010
> > +/** Offload tunneling packet checksum in csum forward engine */
> > +#define TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM      0x0010
> >   /** Insert VLAN header in forward engine */
> >   #define TESTPMD_TX_OFFLOAD_INSERT_VLAN       0x0020
> > +/**
> > + * Offload outer-IP checksum in csum forward engine
> > + * for tunneling packet
> > + */
> > +#define TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM    0x0040
> > +/**
> > + * For a tunneling packet, user requests HW offload for its outer
> > + * layer checksum, and don't care is it a tunneled packet or not.
> > + */
> > +#define TESTPMD_TX_OFFLOAD_NON_TUNNEL_CKSUM  0x0080
> >
> >   /**
> >    * The data structure associated with each port.
> >
> 
> For now, I did not check the implementation of the patch. I'm not sure I
> understand the specifications, so I cannot check if the code conforms to it.
> 
> How this patch is tested? 
Yes, it is tested for i40e and ixgbe.

> I think a report similar to
> http://dpdk.org/ml/archives/dev/2014-November/007991.html would help to
> verify that it works (at least on one driver), and to understand the test-pmd API
> and the different use cases.
Ok, will send test report.

> Regards,
> Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Olivier MATZ]

Hi Jijiang,

Sorry for the late review, I was very busy these last days. Please find
my comments below.

On 12/10/2014 02:03 AM, Jijiang Liu wrote:
> In the current codes, the "tx_checksum set (ip|udp|tcp|sctp|vxlan) (hw|sw) (port-id)" command is not easy to understand and extend, so the patch set enhances the tx_checksum command and reworks csum forwarding engine due to the change of tx_checksum command.
> The main changes of the tx_checksum command are listed below,
>
> 1> add "tx_checksum set tunnel (hw|sw|none) (port-id)" command
>
> The command is used to set/clear tunnel flag that is used to tell the NIC that the packetg is a tunneing packet and application want hardware TX checksum offload for outer layer, or inner layer, or both.

packetg -> packet
tunneing -> tunneling

I don't understand the description: this flag cannot be set to tell the
NIC that it's a tunnel packet because it's a configuration flag.
Whatever the value of this configuration option, the packets can be
either tunnel or non-tunnel packets. The real question is, what is
the behavior for each packet type for each value for this option.

> The 'none' option means that user treat tunneling packet as ordinary packet when using the csum forward engine.
> for example, let say we have a tunnel packet: eth_hdr_out/ipv4_hdr_out/udp_hdr_out/vxlan_hdr/ehtr_hdr_in/ipv4_hdr_in/tcp_hdr_in. one of several scenarios:
>
> 1) User requests HW offload for ipv4_hdr_out  checksum, and doesn't care is it a tunnelled packet or not. So he sets:

tunnelled -> tunneled

>
> tx_checksum set tunnel none 0
>
> tx_checksum set ip hw 0
>
> So for such case we should set tx_tunnel to 'none'.
>
> 2> add "tx_checksum set outer-ip (hw|sw) (port-id)" command
>
> The command is used to set/clear outer IP flag that is used to tell the NIC that application want hardware offload for outer layer.
>
> 3> remove the 'vxlan' option from the  "tx_checksum set (ip|udp|tcp|sctp|vxlan) (hw|sw) (port-id)" command
>
> The command is used to set IP, UDP, TCP and SCTP TX checksum flag. In the case of tunneling packet, the IP, UDP, TCP and SCTP flags always concern inner layer.
>
> Moreover, replace the TESTPMD_TX_OFFLOAD_VXLAN_CKSUM flag with TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM flag and add the TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM and TESTPMD_TX_OFFLOAD_NON_TUNNEL_CKSUM flag in test-pmd application.

You are mixing scenario descriptions with what you do in your patchset:
1/ is a scenario
2/ and 3/ are descriptions of added/removed commands

Let's first sumarize what was the behavior before this patch. This
is the description in csumonly code.

Receive a burst of packets, and for each packet:
  - parse packet, and try to recognize a supported packet type (1)
  - if it's not a supported packet type, don't touch the packet, else:
  - modify the IPs in inner headers and in outer headers if any
  - reprocess the checksum of all supported layers. This is done in SW
    or HW, depending on testpmd command line configuration
  - if TSO is enabled in testpmd command line, also flag the mbuf for TCP
    segmentation offload (this implies HW TCP checksum)
Then transmit packets on the output port.

(1) Supported packets are:
   Ether / (vlan) / IP|IP6 / UDP|TCP|SCTP .
   Ether / (vlan) / outer IP|IP6 / outer UDP / VxLAN / Ether / IP|IP6 /
           UDP|TCP|SCTP

The testpmd command line for this forward engine sets the flags
TESTPMD_TX_OFFLOAD_* in ports[tx_port].tx_ol_flags. They control
wether a checksum must be calculated in software or in hardware. The
IP, UDP, TCP and SCTP flags always concern the inner layer.  The
VxLAN flag concerns the outer IP (if packet is recognized as a vxlan 
packet).

 From this description, it is easy to deduct this table:

Packet type 1:
  Ether / (vlan) / IP|IP6 / UDP|TCP|SCTP .

Packet type 2
   Ether / (vlan) / outer IP|IP6 / outer UDP / VxLAN / Ether / IP|IP6 /
       UDP|TCP|SCTP

+---------------------------+-------------------+-------------------+
|test-pmd config \ packets  |Packet type 1      |Packet type 2      |
+---------------------------+-------------------+-------------------+
|whatever the flags         |IP addresses       |inner and outer IP |
|                           |incremented        |addresses          |
|                           |                   |incremented        |
+---------------------------+-------------------+-------------------+
|IP = sw                    |IP cksum is sw     |inner IP cksum is  |
|                           |                   |sw                 |
+---------------------------+-------------------+-------------------+
|IP = hw                    |IP cksum is hw     |inner IP cksum is  |
|                           |(using lX_len)     |hw (using lX_len)  |
+---------------------------+-------------------+-------------------+
|UDP or TCP or SCTP = sw    |L4 cksum is sw     |inner L4 cksum is  |
|                           |                   |sw                 |
+---------------------------+-------------------+-------------------+
|UDP or TCP or SCTP = hw    |L4 cksum is hw     |inner L4 cksum is  |
|                           |(using lX_len)     |hw (using lX_len)  |
+---------------------------+-------------------+-------------------+
|VxLAN = sw                 |N/A                |outer IP cksum is  |
|                           |                   |sw, outer UDP cksum|
|                           |                   |is sw              |
+---------------------------+-------------------+-------------------+
|VxLAN = hw                 |N/A                |outer IP cksum is  |
|                           |                   |hw (using          |
|                           |                   |outer_lX_len),     |
|                           |                   |outer UDP cksum is |
|                           |                   |sw (no hw support) |
+---------------------------+-------------------+-------------------+


It could be really helpful to have a table like this for what you
are implementing, because I feel there are too many options. Here
is an example of what could be done here (if options are not
independent like before, it can be presented in a different way
in the first column).

Another thing: you don't describe what you want to be able to do:

1/ packet type 1: compute L3 and/or L4 checksum using lX_len
2/ packet type 2: compute inner L3 and/or L4 checksum using lX_len
3/ packet type 2: compute outer L3 and/or L4 checksum using lX_len
4/ packet type 2: compute inner L3 and/or L4 checksum using lX_len
    and outer L3 and/or L4 checksum using outer_lX_len

why not having the 2 following commands:

tx_checksum set 
(ip|udp|tcp|sctp|outer-ip|outer-udp|outer-tcp|outer-sctp) <portid>

   select if we use hw or sw calculation for each header type

tx_checksum tunnel (inner|outer|both)

   when a tunnel packet is received in csum only, control wether
   we want to process inner, outer or both headers

The resulting table would be:

+------------------+-----------+-------------------------------------+
|test-pmd \ packets|Packet type|Packet type 2                        |
|                  |1          +-----------+-----------+-------------+
|                  |           |tun = inner|tun = outer|tun = both   |
|                  |           |           |           |             |
|                  |           |           |           |             |
+------------------+-----------+-----------+-----------+-------------+
|whatever the flags|IP         |inner IP   |outer IP   |inner and    |
|                  |addresses  |addresses  |addresses  |outer IP     |
|                  |incremented|incremented|incremented|addresses    |
|                  |           |           |           |incremented  |
+------------------+-----------+-----------+-----------+-------------+
|IP = sw           |IP cksum is|inner IP   |           |inner IP     |
|                  |sw         |cksum is sw|           |cksum is sw  |
|                  |           |           |           |             |
+------------------+-----------+-----------+-----------+-------------+
|IP = hw           |IP cksum is|inner IP   |           |inner IP     |
|                  |hw (using  |cksum is hw|           |cksum is hw  |
|                  |lX_len)    |(using     |           |(using       |
|                  |           |lX_len)    |           |lX_len)      |
+------------------+-----------+-----------+-----------+-------------+
|UDP or TCP or SCTP|L4 cksum is|inner L4   |           |inner L4     |
|= sw              |sw         |cksum is sw|           |cksum is sw  |
|                  |           |           |           |             |
+------------------+-----------+-----------+-----------+-------------+
|UDP or TCP or SCTP|L4 cksum is|inner L4   |           |inner L4     |
|= hw              |hw (using  |cksum is hw|           |cksum is hw  |
|                  |lX_len)    |(using     |           |(using       |
|                  |           |lX_len)    |           |lX_len)      |
+------------------+-----------+-----------+-----------+-------------+
|outer IP = sw     |N/A        |           |outer IP   |outer IP     |
|                  |           |           |cksum is sw|cksum is sw  |
|                  |           |           |           |             |
+------------------+-----------+-----------+-----------+-------------+
|outer IP = hw     |N/A        |           |outer IP   |outer IP     |
|                  |           |           |cksum is hw|cksum is hw  |
|                  |           |           |(using     |(using       |
|                  |           |           |lX_len)    |outer_lX_len)|
|                  |           |           |           |             |
+------------------+-----------+-----------+-----------+-------------+
|outer UDP or TCP  |N/A        |           |outer L4   |outer L4     |
|or SCTP = sw      |           |           |cksum is sw|cksum is sw  |
|                  |           |           |           |             |
+------------------+-----------+-----------+-----------+-------------+
|outer UDP or TCP  |N/A        |           |outer L4   |outer L4     |
|or SCTP = hw      |           |           |cksum is hw|cksum is hw  |
|                  |           |           |(using     |(using       |
|                  |           |           |lX_len)    |outer_lX_len,|
|                  |           |           |           |knowing that |
|                  |           |           |           |no hw support|
|                  |           |           |           |it today)    |
+------------------+-----------+-----------+-----------+-------------+

> v2 change:
>       redefine the 'none' behaviour for "tx_checksum set tunnel (hw|sw|none) (port-id)" command.
> v3 change:
>       typo correction in cmdline help
>
> Jijiang Liu (3):
>    add outer IP offload capability in librte_ether.
>    add outer IP checksum capability in i40e PMD
>    testpmd command lines of the tx_checksum and csum forwarding rework
>
>   app/test-pmd/cmdline.c            |  201 +++++++++++++++++++++++++++++++++++--
>   app/test-pmd/csumonly.c           |   35 ++++---
>   app/test-pmd/testpmd.h            |    6 +-
>   lib/librte_ether/rte_ethdev.h     |    1 +
>   lib/librte_pmd_i40e/i40e_ethdev.c |    3 +-
>   5 files changed, 218 insertions(+), 28 deletions(-)
>

One more comment, which is not critical. I think the commit log would
be more readable if the lines are truncated at 72 columns, like
described here:
http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html

Regards,
Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Liu, Jijiang]

Hi Olivier,

Thanks for your comments.

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Thursday, December 11, 2014 6:18 PM
> To: Liu, Jijiang; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and
> csum forwarding engine
> 
> Hi Jijiang,
> 
> Sorry for the late review, I was very busy these last days. Please find my
> comments below.
> 
> On 12/10/2014 02:03 AM, Jijiang Liu wrote:
> > In the current codes, the "tx_checksum set (ip|udp|tcp|sctp|vxlan) (hw|sw)
> (port-id)" command is not easy to understand and extend, so the patch set
> enhances the tx_checksum command and reworks csum forwarding engine due
> to the change of tx_checksum command.
> > The main changes of the tx_checksum command are listed below,
> >
> > 1> add "tx_checksum set tunnel (hw|sw|none) (port-id)" command
> >
> > The command is used to set/clear tunnel flag that is used to tell the NIC that the
> packetg is a tunneing packet and application want hardware TX checksum offload
> for outer layer, or inner layer, or both.
> 
> packetg -> packet
> tunneing -> tunneling
> 
> I don't understand the description: this flag cannot be set to tell the NIC that it's a
> tunnel packet because it's a configuration flag.
> Whatever the value of this configuration option, the packets can be either tunnel
> or non-tunnel packets. The real question is, what is the behavior for each packet
> type for each value for this option.

Ok,
Will replace the above the description with the following:

The 'hw/sw' option  is used to set/clear the flag of enabling TX tunneling packet checksum hardware offload in testpmd application.


> > The 'none' option means that user treat tunneling packet as ordinary packet
> when using the csum forward engine.
> > for example, let say we have a tunnel packet:
> eth_hdr_out/ipv4_hdr_out/udp_hdr_out/vxlan_hdr/ehtr_hdr_in/ipv4_hdr_in/tcp
> _hdr_in. one of several scenarios:
> >
> > 1) User requests HW offload for ipv4_hdr_out  checksum, and doesn't care is it
> a tunnelled packet or not. So he sets:
> 
> tunnelled -> tunneled
> 
> >
> > tx_checksum set tunnel none 0
> >
> > tx_checksum set ip hw 0
> >
> > So for such case we should set tx_tunnel to 'none'.
> >
> > 2> add "tx_checksum set outer-ip (hw|sw) (port-id)" command
> >
> > The command is used to set/clear outer IP flag that is used to tell the NIC that
> application want hardware offload for outer layer.
> >
> > 3> remove the 'vxlan' option from the  "tx_checksum set
> > 3> (ip|udp|tcp|sctp|vxlan) (hw|sw) (port-id)" command
> >
> > The command is used to set IP, UDP, TCP and SCTP TX checksum flag. In the case
> of tunneling packet, the IP, UDP, TCP and SCTP flags always concern inner layer.
> >
> > Moreover, replace the TESTPMD_TX_OFFLOAD_VXLAN_CKSUM flag with
> TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM flag and add the
> TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM and
> TESTPMD_TX_OFFLOAD_NON_TUNNEL_CKSUM flag in test-pmd application.
> 
> You are mixing scenario descriptions with what you do in your patchset:
> 1/ is a scenario
> 2/ and 3/ are descriptions of added/removed commands

No.
Please note the symbols for command descriptions and  scenario descriptions.

The command descriptions with ">"  symbol.
 1> add "tx_checksum set tunnel (hw|sw|none) (port-id)" command
 2> add "tx_checksum set outer-ip (hw|sw) (port-id)" command
3> (ip|udp|tcp|sctp|vxlan) (hw|sw) (port-id)" command

The scenario descriptions with ")"  symbol.
1) User requests HW offload for ipv4_hdr_out  checksum, and doesn't care is it a tunneled packet or not. So he sets:


> Let's first sumarize what was the behavior before this patch. This is the
> description in csumonly code.
> 
> Receive a burst of packets, and for each packet:
>   - parse packet, and try to recognize a supported packet type (1)
>   - if it's not a supported packet type, don't touch the packet, else:
>   - modify the IPs in inner headers and in outer headers if any
>   - reprocess the checksum of all supported layers. This is done in SW
>     or HW, depending on testpmd command line configuration
>   - if TSO is enabled in testpmd command line, also flag the mbuf for TCP
>     segmentation offload (this implies HW TCP checksum) Then transmit packets on
> the output port.
> 
> (1) Supported packets are:
>    Ether / (vlan) / IP|IP6 / UDP|TCP|SCTP .
>    Ether / (vlan) / outer IP|IP6 / outer UDP / VxLAN / Ether / IP|IP6 /
>            UDP|TCP|SCTP
> 
> The testpmd command line for this forward engine sets the flags
> TESTPMD_TX_OFFLOAD_* in ports[tx_port].tx_ol_flags. They control wether a
> checksum must be calculated in software or in hardware. The IP, UDP, TCP and
> SCTP flags always concern the inner layer.  The VxLAN flag concerns the outer IP
> (if packet is recognized as a vxlan packet).
> 
>  From this description, it is easy to deduct this table:
> 
> Packet type 1:
>   Ether / (vlan) / IP|IP6 / UDP|TCP|SCTP .
> 
> Packet type 2
>    Ether / (vlan) / outer IP|IP6 / outer UDP / VxLAN / Ether / IP|IP6 /
>        UDP|TCP|SCTP
> 
> +---------------------------+-------------------+-------------------+
> |test-pmd config \ packets  |Packet type 1      |Packet type 2      |
> +---------------------------+-------------------+-------------------+
> |whatever the flags         |IP addresses       |inner and outer IP |
> |                           |incremented        |addresses          |
> |                           |                   |incremented        |
> +---------------------------+-------------------+-------------------+
> |IP = sw                    |IP cksum is sw     |inner IP cksum is  |
> |                           |                   |sw                 |
> +---------------------------+-------------------+-------------------+
> |IP = hw                    |IP cksum is hw     |inner IP cksum is  |
> |                           |(using lX_len)     |hw (using lX_len)  |
> +---------------------------+-------------------+-------------------+
> |UDP or TCP or SCTP = sw    |L4 cksum is sw     |inner L4 cksum is  |
> |                           |                   |sw                 |
> +---------------------------+-------------------+-------------------+
> |UDP or TCP or SCTP = hw    |L4 cksum is hw     |inner L4 cksum is  |
> |                           |(using lX_len)     |hw (using lX_len)  |
> +---------------------------+-------------------+-------------------+
> |VxLAN = sw                 |N/A                |outer IP cksum is  |
> |                           |                   |sw, outer UDP cksum|
> |                           |                   |is sw              |
> +---------------------------+-------------------+-------------------+
> |VxLAN = hw                 |N/A                |outer IP cksum is  |
> |                           |                   |hw (using          |
> |                           |                   |outer_lX_len),     |
> |                           |                   |outer UDP cksum is |
> |                           |                   |sw (no hw support) |
> +---------------------------+-------------------+-------------------+
> 
> 
> It could be really helpful to have a table like this for what you are implementing,
> because I feel there are too many options. Here is an example of what could be
> done here (if options are not independent like before, it can be presented in a
> different way in the first column).
> 

> Another thing: you don't describe what you want to be able to do:
> 
> 1/ packet type 1: compute L3 and/or L4 checksum using lX_len 2/ packet type 2:
> compute inner L3 and/or L4 checksum using lX_len 3/ packet type 2: compute
> outer L3 and/or L4 checksum using lX_len 4/ packet type 2: compute inner L3
> and/or L4 checksum using lX_len
>     and outer L3 and/or L4 checksum using outer_lX_len

These details have already covered in http://dpdk.org/ml/archives/dev/2014-December/009213.html,
if the patch set is applied, and we aslo have to update the some documents.


> why not having the 2 following commands:
>

I have talked about why we need the current 3 commands in another mail loop, let me explain it here again.
 
First. We  still think we need some command to enable/disable tunneling  support in testpmd, that's why the command 1 is needed.

1. tx_checksum set tunnel (hw|sw|none) (port-id) command

2. tx_cksum set (outer-ip)  (hw|sw) (port_id)

3. tx_cksum set (ip|l4) (hw|sw) (port_id)

Secondly, in most of cases,   user application use non-tunneling packet, so he just care how to use 3, don't need to care 1 and 2, don't you think  it becomes simpler?  
If we mix tunneling packet command and non-tunneling packet together, and the commands will become more complicated and  not easy to understand.


> tx_checksum set
> (ip|udp|tcp|sctp|outer-ip|outer-udp|outer-tcp|outer-sctp) <portid>

As far as I know, so far,  there is no a type of tunneling packet with outer-tcp and outer-sctp.

>    select if we use hw or sw calculation for each header type
> 
> tx_checksum tunnel (inner|outer|both)
> 
>    when a tunnel packet is received in csum only, control wether
>    we want to process inner, outer or both headers

This command can't meet/match our previous discussions and current implementation.  In terms of 'inner' option, which can't meet the two following cases.

B) User is aware that it is a tunneled packet and requests HW offload for ipv4_hdr_in and tcp_hdr_in *only*.
He doesn't care about outer IP checksum offload.
In that case, for FVL  he has 2 choices:
   1. Treat that packet as a 'proper' tunnelled packet, and fill all the fields:
     mb->l2_len =  udp_hdr_len + vxlan_hdr_len + eth_hdr_in;
     mb->l3_len = ipv4_hdr_in;
     mb->outer_l2_len = eth_hdr_out;
     mb->outer_l3_len = ipv4_hdr_out;
     mb->ol_flags |= PKT_TX_UDP_TUNNEL | PKT_TX_IP_CKSUM |  PKT_TX_TCP_CKSUM;

   2. As user doesn't care about outer IP hdr checksum, he can treat everything before ipv4_hdr_in as L2 header.
   So he knows, that it is a tunneled packet, but makes HW to treat it as ordinary (non-tunneled) packet:
     mb->l2_len = eth_hdr_out + ipv4_hdr_out + udp_hdr_out + vxlan_hdr + ehtr_hdr_in;
     mb->l3_len = ipv4_hdr_in;
     mb->ol_flags |= PKT_TX_IP_CKSUM |  PKT_TX_TCP_CKSUM;

> 
> The resulting table would be:
> 
> +------------------+-----------+-------------------------------------+
> |test-pmd \ packets|Packet type|Packet type 2                        |
> |                  |1          +-----------+-----------+-------------+
> |                  |           |tun = inner|tun = outer|tun = both   |
> |                  |           |           |           |             |
> |                  |           |           |           |             |
> +------------------+-----------+-----------+-----------+-------------+
> |whatever the flags|IP         |inner IP   |outer IP   |inner and    |
> |                  |addresses  |addresses  |addresses  |outer IP     |
> |                  |incremented|incremented|incremented|addresses    |
> |                  |           |           |           |incremented  |
> +------------------+-----------+-----------+-----------+-------------+
> |IP = sw           |IP cksum is|inner IP   |           |inner IP     |
> |                  |sw         |cksum is sw|           |cksum is sw  |
> |                  |           |           |           |             |
> +------------------+-----------+-----------+-----------+-------------+
> |IP = hw           |IP cksum is|inner IP   |           |inner IP     |
> |                  |hw (using  |cksum is hw|           |cksum is hw  |
> |                  |lX_len)    |(using     |           |(using       |
> |                  |           |lX_len)    |           |lX_len)      |
> +------------------+-----------+-----------+-----------+-------------+
> |UDP or TCP or SCTP|L4 cksum is|inner L4   |           |inner L4     |
> |= sw              |sw         |cksum is sw|           |cksum is sw  |
> |                  |           |           |           |             |
> +------------------+-----------+-----------+-----------+-------------+
> |UDP or TCP or SCTP|L4 cksum is|inner L4   |           |inner L4     |
> |= hw              |hw (using  |cksum is hw|           |cksum is hw  |
> |                  |lX_len)    |(using     |           |(using       |
> |                  |           |lX_len)    |           |lX_len)      |
> +------------------+-----------+-----------+-----------+-------------+
> |outer IP = sw     |N/A        |           |outer IP   |outer IP     |
> |                  |           |           |cksum is sw|cksum is sw  |
> |                  |           |           |           |             |
> +------------------+-----------+-----------+-----------+-------------+
> |outer IP = hw     |N/A        |           |outer IP   |outer IP     |
> |                  |           |           |cksum is hw|cksum is hw  |
> |                  |           |           |(using     |(using       |
> |                  |           |           |lX_len)    |outer_lX_len)|
> |                  |           |           |           |             |
> +------------------+-----------+-----------+-----------+-------------+
> |outer UDP or TCP  |N/A        |           |outer L4   |outer L4     |
> |or SCTP = sw      |           |           |cksum is sw|cksum is sw  |
> |                  |           |           |           |             |
> +------------------+-----------+-----------+-----------+-------------+
> |outer UDP or TCP  |N/A        |           |outer L4   |outer L4     |
> |or SCTP = hw      |           |           |cksum is hw|cksum is hw  |
> |                  |           |           |(using     |(using       |
> |                  |           |           |lX_len)    |outer_lX_len,|
> |                  |           |           |           |knowing that |
> |                  |           |           |           |no hw support|
> |                  |           |           |           |it today)    |
> +------------------+-----------+-----------+-----------+-------------+
> 
> > v2 change:
> >       redefine the 'none' behaviour for "tx_checksum set tunnel (hw|sw|none)
> (port-id)" command.
> > v3 change:
> >       typo correction in cmdline help
> >
> > Jijiang Liu (3):
> >    add outer IP offload capability in librte_ether.
> >    add outer IP checksum capability in i40e PMD
> >    testpmd command lines of the tx_checksum and csum forwarding rework
> >
> >   app/test-pmd/cmdline.c            |  201
> +++++++++++++++++++++++++++++++++++--
> >   app/test-pmd/csumonly.c           |   35 ++++---
> >   app/test-pmd/testpmd.h            |    6 +-
> >   lib/librte_ether/rte_ethdev.h     |    1 +
> >   lib/librte_pmd_i40e/i40e_ethdev.c |    3 +-
> >   5 files changed, 218 insertions(+), 28 deletions(-)
> >
> 
> One more comment, which is not critical. I think the commit log would be more
> readable if the lines are truncated at 72 columns, like described here:
> http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
> 
> Regards,
> Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Olivier MATZ]

Hello,

On 12/12/2014 04:48 AM, Liu, Jijiang wrote:
> The 'hw/sw' option  is used to set/clear the flag of enabling TX tunneling packet checksum hardware offload in testpmd application.

This is not clear at all.
In your command, there is (hw|sw|none).
Are you talking about inner or outer?
Is this command useful for any kind of packet?
How does it combine with "tx_checksum set outer-ip (hw|sw)"?

>> You are mixing scenario descriptions with what you do in your patchset:
>> 1/ is a scenario
>> 2/ and 3/ are descriptions of added/removed commands
> 
> No.
> Please note the symbols for command descriptions and  scenario descriptions.
> 
> The command descriptions with ">"  symbol.
>  1> add "tx_checksum set tunnel (hw|sw|none) (port-id)" command
>  2> add "tx_checksum set outer-ip (hw|sw) (port-id)" command
> 3> (ip|udp|tcp|sctp|vxlan) (hw|sw) (port-id)" command
> 
> The scenario descriptions with ")"  symbol.
> 1) User requests HW offload for ipv4_hdr_out  checksum, and doesn't care is it a tunneled packet or not. So he sets:

I read again your cover letter. You enumerate *one* scenario.
An enumeration starts with 2 items.
Then you mix 1> and 1) numbers, which does not make things readable.

>> Another thing: you don't describe what you want to be able to do:
>>
>> 1/ packet type 1: compute L3 and/or L4 checksum using lX_len 2/ packet type 2:
>> compute inner L3 and/or L4 checksum using lX_len 3/ packet type 2: compute
>> outer L3 and/or L4 checksum using lX_len 4/ packet type 2: compute inner L3
>> and/or L4 checksum using lX_len
>>     and outer L3 and/or L4 checksum using outer_lX_len
> 
> These details have already covered in http://dpdk.org/ml/archives/dev/2014-December/009213.html,
> if the patch set is applied, and we aslo have to update the some documents.

First, this link was not referenced in the cover letter or patchset.
I think it would help to first try to explain what problem is fixed,
what is the need, and why. I think in this case it should not require
lines and it could be done in a simple way.

Indeed, yesterday I spent more than an hour to review your patches
and read your descriptions. After that, I still don't understand how
the commands impact the behavior of the csumonly application. The
possible reasons are:

1) I am too dumb to understand. In this case, it would be better
   for you and the community to find another reviewer.

2) Your descriptions are not clear. In this case, you need to think
   about how to reword them so they can be understood, or even maybe
   think about rework your commands if they cannot be explained with
   simple words.

Note that 1) and 2) are not exclusive ;)

>> why not having the 2 following commands:
>>
> 
> I have talked about why we need the current 3 commands in another mail loop, let me explain it here again.

The community does not have this private thread.
And, that's right, I remember this thread: in it, I asked 3 times some
precisions about the commands without any clear answer.

> First. We  still think we need some command to enable/disable tunneling  support in testpmd, that's why the command 1 is needed.

What does enable/disable tunneling support mean?

> 1. tx_checksum set tunnel (hw|sw|none) (port-id) command
> 
> 2. tx_cksum set (outer-ip)  (hw|sw) (port_id)
> 
> 3. tx_cksum set (ip|l4) (hw|sw) (port_id)
> 
> Secondly, in most of cases,   user application use non-tunneling packet, so he just care how to use 3, don't need to care 1 and 2, don't you think  it becomes simpler?  
> If we mix tunneling packet command and non-tunneling packet together, and the commands will become more complicated and  not easy to understand.

Really no, it is not simpler. But if you are able to explain it
in few words what is done by csumonly, maybe I can change my mind.

>> tx_checksum set
>> (ip|udp|tcp|sctp|outer-ip|outer-udp|outer-tcp|outer-sctp) <portid>
> 
> As far as I know, so far,  there is no a type of tunneling packet with outer-tcp and outer-sctp.

For TCP, there is STT, which is used in storage.
For SCTP, it could probably be removed.

>>    select if we use hw or sw calculation for each header type
>>
>> tx_checksum tunnel (inner|outer|both)
>>
>>    when a tunnel packet is received in csum only, control wether
>>    we want to process inner, outer or both headers
> 
> This command can't meet/match our previous discussions and current implementation.  In terms of 'inner' option, which can't meet the two following cases.
> 
> B) User is aware that it is a tunneled packet and requests HW offload for ipv4_hdr_in and tcp_hdr_in *only*.
> He doesn't care about outer IP checksum offload.
> In that case, for FVL  he has 2 choices:
>    1. Treat that packet as a 'proper' tunnelled packet, and fill all the fields:
>      mb->l2_len =  udp_hdr_len + vxlan_hdr_len + eth_hdr_in;
>      mb->l3_len = ipv4_hdr_in;
>      mb->outer_l2_len = eth_hdr_out;
>      mb->outer_l3_len = ipv4_hdr_out;
>      mb->ol_flags |= PKT_TX_UDP_TUNNEL | PKT_TX_IP_CKSUM |  PKT_TX_TCP_CKSUM;
> 
>    2. As user doesn't care about outer IP hdr checksum, he can treat everything before ipv4_hdr_in as L2 header.
>    So he knows, that it is a tunneled packet, but makes HW to treat it as ordinary (non-tunneled) packet:
>      mb->l2_len = eth_hdr_out + ipv4_hdr_out + udp_hdr_out + vxlan_hdr + ehtr_hdr_in;
>      mb->l3_len = ipv4_hdr_in;
>      mb->ol_flags |= PKT_TX_IP_CKSUM |  PKT_TX_TCP_CKSUM;

Indeed, there are 2 choices for fortville. I could argue that you also
have 2 choices to offload the IP checksum of Ether/IP/UDP/xxx:
 - use lX_len
 - use outer_lX_len

You have also the choice to use lX_len or outer_lX_len if you want
to offload the outer IP checksum of Ether/IP/UDP/vxlan/Ether/IP/UDP/xxx

That's exactly why I asked to first describe what problem you want
to solve in your patchset. You can extend the list I've send in my
first answer.

Maybe your solution supports all these cases, but as a user, I
have no idea how to configure it even after reading your desriptions
during a long time, so the result is the same than not supporting.

The question is: are all these casez useful? To me, it's equally
important that a user can use the application and understand what it
does. If they are required I'm sure we can find a good user API that
is able to test all these cases.

Regards,
Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Liu, Jijiang]

Hi Olivier,

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Saturday, December 13, 2014 12:33 AM
> To: Liu, Jijiang
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and
> csum forwarding engine
>
> Hello,
>
> On 12/12/2014 04:48 AM, Liu, Jijiang wrote:
> > The 'hw/sw' option  is used to set/clear the flag of enabling TX tunneling packet
> checksum hardware offload in testpmd application.
>
> This is not clear at all.
> In your command, there is (hw|sw|none).
> Are you talking about inner or outer?
> Is this command useful for any kind of packet?
> How does it combine with "tx_checksum set outer-ip (hw|sw)"?
>

I rethink these TX checksum commands in this patch set and agree with you that we should make some changes for having clear meaning for them.

There are  3 commands in patch set as follows,
1. tx_checksum set tunnel (hw|sw|none) (port-id)

Now I also think the command 1 may confuse user, they probably don't understand  why we need 'hw' or 'sw' option and when  to use the two option,
so I will replace the command with 'tx_checksum set hw-tunnel-mode (on|off) (port-id)' command.

2. tx_checksum set outer-ip (hw|sw) (port-id)
3. tx_checksum set  (ip|udp|tcp|sctp) (hw|sw) (port-id)

The command 2 will be merged into command 3, the new command is ' tx_checksum set  (outer-ip|ip|udp|tcp|sctp) (hw|sw) (port-id)'.

These most of the cases in http://dpdk.org/ml/archives/dev/2014-December/009213.html will be covered by using the two commands

The command 'tx_checksum set hw-tunnel-mode (on|off)  (port-id)' is used to set/clear  TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM flag.
Actually, the PKT_TX_UDP_TUNNEL_PKT offload flag will be set if the testpmd flag is set, which tell driver/HW treat  that transmit packet as a tunneling packet.

When 'on' is set, which is able to meet Method B.1 and method C.

When 'off' is set, the TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM is not needed to set, so the PKT_TX_UDP_TUNNEL_PKT offload flag is not needed to set,  then HW treat  that transmit packet as a non-tunneling packet. It is able to meet Method B.2.

As to case A, I think it is not mandatory to cover it in csum fwd engine for tunneling packet.

Is the above description clear for you?

> Regards,
> Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Ananyev, Konstantin]

Hi Frank,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Liu, Jijiang
> Sent: Wednesday, January 07, 2015 2:04 AM
> To: 'Olivier MATZ'
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine
> 
> Hi Olivier,
> 
> > -----Original Message-----
> > From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> > Sent: Saturday, December 13, 2014 12:33 AM
> > To: Liu, Jijiang
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and
> > csum forwarding engine
> >
> > Hello,
> >
> > On 12/12/2014 04:48 AM, Liu, Jijiang wrote:
> > > The 'hw/sw' option  is used to set/clear the flag of enabling TX tunneling packet
> > checksum hardware offload in testpmd application.
> >
> > This is not clear at all.
> > In your command, there is (hw|sw|none).
> > Are you talking about inner or outer?
> > Is this command useful for any kind of packet?
> > How does it combine with "tx_checksum set outer-ip (hw|sw)"?
> >
> 
> I rethink these TX checksum commands in this patch set and agree with you that we should make some changes for having clear
> meaning for them.
> 
> There are  3 commands in patch set as follows,
> 1. tx_checksum set tunnel (hw|sw|none) (port-id)
> 
> Now I also think the command 1 may confuse user, they probably don't understand  why we need 'hw' or 'sw' option and when  to
> use the two option,
> so I will replace the command with 'tx_checksum set hw-tunnel-mode (on|off) (port-id)' command.

I am a bit confused here, could you explain what would be a behaviour for 'on' and 'off'?
Konstantin

> 
> 2. tx_checksum set outer-ip (hw|sw) (port-id)
> 3. tx_checksum set  (ip|udp|tcp|sctp) (hw|sw) (port-id)
> 
> The command 2 will be merged into command 3, the new command is ' tx_checksum set  (outer-ip|ip|udp|tcp|sctp) (hw|sw) (port-
> id)'.
> 
> These most of the cases in http://dpdk.org/ml/archives/dev/2014-December/009213.html will be covered by using the two
> commands
> 
> The command 'tx_checksum set hw-tunnel-mode (on|off)  (port-id)' is used to set/clear  TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM
> flag.
> Actually, the PKT_TX_UDP_TUNNEL_PKT offload flag will be set if the testpmd flag is set, which tell driver/HW treat  that transmit
> packet as a tunneling packet.
> 
> When 'on' is set, which is able to meet Method B.1 and method C.
> 
> When 'off' is set, the TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM is not needed to set, so the PKT_TX_UDP_TUNNEL_PKT offload flag is
> not needed to set,  then HW treat  that transmit packet as a non-tunneling packet. It is able to meet Method B.2.
> 
> As to case A, I think it is not mandatory to cover it in csum fwd engine for tunneling packet.
> 
> Is the above description clear for you?
> 
> > Regards,
> > Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Liu, Jijiang]

Hi Konstantin,

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Wednesday, January 7, 2015 5:59 PM
> To: Liu, Jijiang; 'Olivier MATZ'
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and
> csum forwarding engine
> 
> Hi Frank,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Liu, Jijiang
> > Sent: Wednesday, January 07, 2015 2:04 AM
> > To: 'Olivier MATZ'
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and
> > csum forwarding engine
> >
> > Hi Olivier,
> >
> > > -----Original Message-----
> > > From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> > > Sent: Saturday, December 13, 2014 12:33 AM
> > > To: Liu, Jijiang
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command
> > > and csum forwarding engine
> > >
> > > Hello,
> > >
> > > On 12/12/2014 04:48 AM, Liu, Jijiang wrote:
> > > > The 'hw/sw' option  is used to set/clear the flag of enabling TX
> > > > tunneling packet
> > > checksum hardware offload in testpmd application.
> > >
> > > This is not clear at all.
> > > In your command, there is (hw|sw|none).
> > > Are you talking about inner or outer?
> > > Is this command useful for any kind of packet?
> > > How does it combine with "tx_checksum set outer-ip (hw|sw)"?
> > >
> >
> > I rethink these TX checksum commands in this patch set and agree with
> > you that we should make some changes for having clear meaning for them.
> >
> > There are  3 commands in patch set as follows, 1. tx_checksum set
> > tunnel (hw|sw|none) (port-id)
> >
> > Now I also think the command 1 may confuse user, they probably don't
> > understand  why we need 'hw' or 'sw' option and when  to use the two
> > option, so I will replace the command with 'tx_checksum set hw-tunnel-mode
> (on|off) (port-id)' command.
> 
> I am a bit confused here, could you explain what would be a behaviour for 'on' and
> 'off'?
> Konstantin

I have explained the behaviour for 'on' and'off' below,

The command 'tx_checksum set hw-tunnel-mode (on|off)  (port-id)' is 
used to set/clear  TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM flag.

Actually, the PKT_TX_UDP_TUNNEL_PKT offload flag will be set if the 
testpmd flag is set, which means to tell HW treat  that transmit packet as a tunneling packet to do checksum offload
 When 'on' is set, which is able to meet Method B.1 and method C.

When 'off' is set, the TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM is not needed 
to set, so the PKT_TX_UDP_TUNNEL_PKT offload flag is not needed to set,  then HW treat  that transmit packet as a non-tunneling packet. It is able to meet Method B.2.

Is the explanation not clear?

>
> >
> > 2. tx_checksum set outer-ip (hw|sw) (port-id) 3. tx_checksum set
> > (ip|udp|tcp|sctp) (hw|sw) (port-id)
> >
> > The command 2 will be merged into command 3, the new command is '
> > tx_checksum set  (outer-ip|ip|udp|tcp|sctp) (hw|sw) (port- id)'.
> >
> > These most of the cases in
> > http://dpdk.org/ml/archives/dev/2014-December/009213.html will be
> > covered by using the two commands
> >
> > The command 'tx_checksum set hw-tunnel-mode (on|off)  (port-id)' is
> > used to set/clear  TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM flag.
> > Actually, the PKT_TX_UDP_TUNNEL_PKT offload flag will be set if the
> > testpmd flag is set, which tell driver/HW treat  that transmit packet as a
> tunneling packet.
> >
> > When 'on' is set, which is able to meet Method B.1 and method C.
> >
> > When 'off' is set, the TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM is not needed
> > to set, so the PKT_TX_UDP_TUNNEL_PKT offload flag is not needed to set,  then
> HW treat  that transmit packet as a non-tunneling packet. It is able to meet
> Method B.2.
> >
> > As to case A, I think it is not mandatory to cover it in csum fwd engine for
> tunneling packet.
> >
> > Is the above description clear for you?
> >
> > > Regards,
> > > Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Ananyev, Konstantin]

> -----Original Message-----
> From: Liu, Jijiang
> Sent: Wednesday, January 07, 2015 11:39 AM
> To: Ananyev, Konstantin; 'Olivier MATZ'
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine
> 
> Hi Konstantin,
> 
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Wednesday, January 7, 2015 5:59 PM
> > To: Liu, Jijiang; 'Olivier MATZ'
> > Cc: dev@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and
> > csum forwarding engine
> >
> > Hi Frank,
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Liu, Jijiang
> > > Sent: Wednesday, January 07, 2015 2:04 AM
> > > To: 'Olivier MATZ'
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and
> > > csum forwarding engine
> > >
> > > Hi Olivier,
> > >
> > > > -----Original Message-----
> > > > From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> > > > Sent: Saturday, December 13, 2014 12:33 AM
> > > > To: Liu, Jijiang
> > > > Cc: dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command
> > > > and csum forwarding engine
> > > >
> > > > Hello,
> > > >
> > > > On 12/12/2014 04:48 AM, Liu, Jijiang wrote:
> > > > > The 'hw/sw' option  is used to set/clear the flag of enabling TX
> > > > > tunneling packet
> > > > checksum hardware offload in testpmd application.
> > > >
> > > > This is not clear at all.
> > > > In your command, there is (hw|sw|none).
> > > > Are you talking about inner or outer?
> > > > Is this command useful for any kind of packet?
> > > > How does it combine with "tx_checksum set outer-ip (hw|sw)"?
> > > >
> > >
> > > I rethink these TX checksum commands in this patch set and agree with
> > > you that we should make some changes for having clear meaning for them.
> > >
> > > There are  3 commands in patch set as follows, 1. tx_checksum set
> > > tunnel (hw|sw|none) (port-id)
> > >
> > > Now I also think the command 1 may confuse user, they probably don't
> > > understand  why we need 'hw' or 'sw' option and when  to use the two
> > > option, so I will replace the command with 'tx_checksum set hw-tunnel-mode
> > (on|off) (port-id)' command.
> >
> > I am a bit confused here, could you explain what would be a behaviour for 'on' and
> > 'off'?
> > Konstantin
> 
> I have explained the behaviour for 'on' and'off' below,
> 
> The command 'tx_checksum set hw-tunnel-mode (on|off)  (port-id)' is
> used to set/clear  TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM flag.
> 
> Actually, the PKT_TX_UDP_TUNNEL_PKT offload flag will be set if the
> testpmd flag is set, which means to tell HW treat  that transmit packet as a tunneling packet to do checksum offload
>  When 'on' is set, which is able to meet Method B.1 and method C.
> 
> When 'off' is set, the TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM is not needed
> to set, so the PKT_TX_UDP_TUNNEL_PKT offload flag is not needed to set,  then HW treat  that transmit packet as a non-tunneling
> packet. It is able to meet Method B.2.
> 
> Is the explanation not clear?

Ok, and how I can set method A (testpmd treat all packets as non-tunnelling and never look beyond outer L4 header) then?
Konstantin

> 
> >
> > >
> > > 2. tx_checksum set outer-ip (hw|sw) (port-id) 3. tx_checksum set
> > > (ip|udp|tcp|sctp) (hw|sw) (port-id)
> > >
> > > The command 2 will be merged into command 3, the new command is '
> > > tx_checksum set  (outer-ip|ip|udp|tcp|sctp) (hw|sw) (port- id)'.
> > >
> > > These most of the cases in
> > > http://dpdk.org/ml/archives/dev/2014-December/009213.html will be
> > > covered by using the two commands
> > >
> > > The command 'tx_checksum set hw-tunnel-mode (on|off)  (port-id)' is
> > > used to set/clear  TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM flag.
> > > Actually, the PKT_TX_UDP_TUNNEL_PKT offload flag will be set if the
> > > testpmd flag is set, which tell driver/HW treat  that transmit packet as a
> > tunneling packet.
> > >
> > > When 'on' is set, which is able to meet Method B.1 and method C.
> > >
> > > When 'off' is set, the TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM is not needed
> > > to set, so the PKT_TX_UDP_TUNNEL_PKT offload flag is not needed to set,  then
> > HW treat  that transmit packet as a non-tunneling packet. It is able to meet
> > Method B.2.
> > >
> > > As to case A, I think it is not mandatory to cover it in csum fwd engine for
> > tunneling packet.
> > >
> > > Is the above description clear for you?
> > >
> > > > Regards,
> > > > Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Liu, Jijiang]

Hi,

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Wednesday, January 7, 2015 8:07 PM
> To: Liu, Jijiang; 'Olivier MATZ'
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and
> csum forwarding engine
> 
> 
> 
> > -----Original Message-----
> > From: Liu, Jijiang
> > Sent: Wednesday, January 07, 2015 11:39 AM
> > To: Ananyev, Konstantin; 'Olivier MATZ'
> > Cc: dev@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and
> > csum forwarding engine
> >
> > Hi Konstantin,
> >
> > > -----Original Message-----
> > > From: Ananyev, Konstantin
> > > Sent: Wednesday, January 7, 2015 5:59 PM
> > > To: Liu, Jijiang; 'Olivier MATZ'
> > > Cc: dev@dpdk.org
> > > Subject: RE: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command
> > > and csum forwarding engine
> > >
> > > Hi Frank,
> > >
> > > > -----Original Message-----
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Liu, Jijiang
> > > > Sent: Wednesday, January 07, 2015 2:04 AM
> > > > To: 'Olivier MATZ'
> > > > Cc: dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command
> > > > and csum forwarding engine
> > > >
> > > > Hi Olivier,
> > > >
> > > > > -----Original Message-----
> > > > > From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> > > > > Sent: Saturday, December 13, 2014 12:33 AM
> > > > > To: Liu, Jijiang
> > > > > Cc: dev@dpdk.org
> > > > > Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum
> > > > > command and csum forwarding engine
> > > > >
> > > > > Hello,
> > > > >
> > > > > On 12/12/2014 04:48 AM, Liu, Jijiang wrote:
> > > > > > The 'hw/sw' option  is used to set/clear the flag of enabling
> > > > > > TX tunneling packet
> > > > > checksum hardware offload in testpmd application.
> > > > >
> > > > > This is not clear at all.
> > > > > In your command, there is (hw|sw|none).
> > > > > Are you talking about inner or outer?
> > > > > Is this command useful for any kind of packet?
> > > > > How does it combine with "tx_checksum set outer-ip (hw|sw)"?
> > > > >
> > > >
> > > > I rethink these TX checksum commands in this patch set and agree
> > > > with you that we should make some changes for having clear meaning for
> them.
> > > >
> > > > There are  3 commands in patch set as follows, 1. tx_checksum set
> > > > tunnel (hw|sw|none) (port-id)
> > > >
> > > > Now I also think the command 1 may confuse user, they probably
> > > > don't understand  why we need 'hw' or 'sw' option and when  to use
> > > > the two option, so I will replace the command with 'tx_checksum
> > > > set hw-tunnel-mode
> > > (on|off) (port-id)' command.
> > >
> > > I am a bit confused here, could you explain what would be a
> > > behaviour for 'on' and 'off'?
> > > Konstantin
> >
> > I have explained the behaviour for 'on' and'off' below,
> >
> > The command 'tx_checksum set hw-tunnel-mode (on|off)  (port-id)' is
> > used to set/clear  TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM flag.
> >
> > Actually, the PKT_TX_UDP_TUNNEL_PKT offload flag will be set if the
> > testpmd flag is set, which means to tell HW treat  that transmit
> > packet as a tunneling packet to do checksum offload  When 'on' is set, which is
> able to meet Method B.1 and method C.
> >
> > When 'off' is set, the TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM is not needed
> > to set, so the PKT_TX_UDP_TUNNEL_PKT offload flag is not needed to
> > set,  then HW treat  that transmit packet as a non-tunneling packet. It is able to
> meet Method B.2.
> >
> > Is the explanation not clear?
> 
> Ok, and how I can set method A (testpmd treat all packets as non-tunnelling and
> never look beyond outer L4 header) then?
> Konstantin


> > > > As to case A, I think it is not mandatory to cover it in csum fwd
> > > > engine for tunneling packet.

If you think the case A is essential, and it must be covered in  csum fwd, then we can add a command:

tx_checksum set sw-tunnel-mode (on|off)  (port-id)

if the 'off' is set ,  csum fwd engine don't check  if that packet is a tunneling packet and treat all packets as non-tunneling and never look beyond outer L4 header.

if the 'on' is set,  csum fwd engine will check if that packet is a tunneling packet.

And we are able to test all of cases in  http://dpdk.org/ml/archives/dev/2014-December/009213.html

Test case A:

tx_checksum set sw-tunnel-mode off
tx_checksum set hw-tunnel-mode off
tx_checksum set  ip   hw

test case B.1:

tx_checksum set sw-tunnel-mode on
tx_checksum set hw-tunnel-mode on
tx_checksum set  ip   hw
tx_checksum set  tcp   hw

test case B.2:

tx_checksum set sw-tunnel-mode on
tx_checksum set hw-tunnel-mode off
tx_checksum set  ip   hw

test case C:

tx_checksum set sw-tunnel-mode on
tx_checksum set hw-tunnel-mode on
tx_checksum set  outer-ip   hw
tx_checksum set  ip   hw
tx_checksum set  tcp   hw


 In addition, the reason of discarding ' tx_checksum set  tunnel (hw|sw|none) (port-id)' command is that  user probably confuse the following case.
 tx_checksum set  tunnel sw
tx_checksum set  ip   hw

In fact, we are still using hardware TX checksum offload in this case,  but the command " tx_checksum set  tunnel sw"  seems tell user that software compute the checksum.

> >
> > >
> > > >
> > > > 2. tx_checksum set outer-ip (hw|sw) (port-id) 3. tx_checksum set
> > > > (ip|udp|tcp|sctp) (hw|sw) (port-id)
> > > >
> > > > The command 2 will be merged into command 3, the new command is '
> > > > tx_checksum set  (outer-ip|ip|udp|tcp|sctp) (hw|sw) (port- id)'.
> > > >
> > > > These most of the cases in
> > > > http://dpdk.org/ml/archives/dev/2014-December/009213.html will be
> > > > covered by using the two commands
> > > >
> > > > The command 'tx_checksum set hw-tunnel-mode (on|off)  (port-id)'
> > > > is used to set/clear  TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM flag.
> > > > Actually, the PKT_TX_UDP_TUNNEL_PKT offload flag will be set if
> > > > the testpmd flag is set, which tell driver/HW treat  that transmit
> > > > packet as a
> > > tunneling packet.
> > > >
> > > > When 'on' is set, which is able to meet Method B.1 and method C.
> > > >
> > > > When 'off' is set, the TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM is not
> > > > needed to set, so the PKT_TX_UDP_TUNNEL_PKT offload flag is not
> > > > needed to set,  then
> > > HW treat  that transmit packet as a non-tunneling packet. It is able
> > > to meet Method B.2.
> > > >
> > > > As to case A, I think it is not mandatory to cover it in csum fwd
> > > > engine for
> > > tunneling packet.
> > > >
> > > > Is the above description clear for you?
> > > >
> > > > > Regards,
> > > > > Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Ananyev, Konstantin]

Hi Frank,

> -----Original Message-----
> From: Liu, Jijiang
> Sent: Thursday, January 08, 2015 8:52 AM
> To: Ananyev, Konstantin; 'Olivier MATZ'
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine
> 
> Hi,
> 
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Wednesday, January 7, 2015 8:07 PM
> > To: Liu, Jijiang; 'Olivier MATZ'
> > Cc: dev@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and
> > csum forwarding engine
> >
> >
> >
> > > -----Original Message-----
> > > From: Liu, Jijiang
> > > Sent: Wednesday, January 07, 2015 11:39 AM
> > > To: Ananyev, Konstantin; 'Olivier MATZ'
> > > Cc: dev@dpdk.org
> > > Subject: RE: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and
> > > csum forwarding engine
> > >
> > > Hi Konstantin,
> > >
> > > > -----Original Message-----
> > > > From: Ananyev, Konstantin
> > > > Sent: Wednesday, January 7, 2015 5:59 PM
> > > > To: Liu, Jijiang; 'Olivier MATZ'
> > > > Cc: dev@dpdk.org
> > > > Subject: RE: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command
> > > > and csum forwarding engine
> > > >
> > > > Hi Frank,
> > > >
> > > > > -----Original Message-----
> > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Liu, Jijiang
> > > > > Sent: Wednesday, January 07, 2015 2:04 AM
> > > > > To: 'Olivier MATZ'
> > > > > Cc: dev@dpdk.org
> > > > > Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command
> > > > > and csum forwarding engine
> > > > >
> > > > > Hi Olivier,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> > > > > > Sent: Saturday, December 13, 2014 12:33 AM
> > > > > > To: Liu, Jijiang
> > > > > > Cc: dev@dpdk.org
> > > > > > Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum
> > > > > > command and csum forwarding engine
> > > > > >
> > > > > > Hello,
> > > > > >
> > > > > > On 12/12/2014 04:48 AM, Liu, Jijiang wrote:
> > > > > > > The 'hw/sw' option  is used to set/clear the flag of enabling
> > > > > > > TX tunneling packet
> > > > > > checksum hardware offload in testpmd application.
> > > > > >
> > > > > > This is not clear at all.
> > > > > > In your command, there is (hw|sw|none).
> > > > > > Are you talking about inner or outer?
> > > > > > Is this command useful for any kind of packet?
> > > > > > How does it combine with "tx_checksum set outer-ip (hw|sw)"?
> > > > > >
> > > > >
> > > > > I rethink these TX checksum commands in this patch set and agree
> > > > > with you that we should make some changes for having clear meaning for
> > them.
> > > > >
> > > > > There are  3 commands in patch set as follows, 1. tx_checksum set
> > > > > tunnel (hw|sw|none) (port-id)
> > > > >
> > > > > Now I also think the command 1 may confuse user, they probably
> > > > > don't understand  why we need 'hw' or 'sw' option and when  to use
> > > > > the two option, so I will replace the command with 'tx_checksum
> > > > > set hw-tunnel-mode
> > > > (on|off) (port-id)' command.
> > > >
> > > > I am a bit confused here, could you explain what would be a
> > > > behaviour for 'on' and 'off'?
> > > > Konstantin
> > >
> > > I have explained the behaviour for 'on' and'off' below,
> > >
> > > The command 'tx_checksum set hw-tunnel-mode (on|off)  (port-id)' is
> > > used to set/clear  TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM flag.
> > >
> > > Actually, the PKT_TX_UDP_TUNNEL_PKT offload flag will be set if the
> > > testpmd flag is set, which means to tell HW treat  that transmit
> > > packet as a tunneling packet to do checksum offload  When 'on' is set, which is
> > able to meet Method B.1 and method C.
> > >
> > > When 'off' is set, the TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM is not needed
> > > to set, so the PKT_TX_UDP_TUNNEL_PKT offload flag is not needed to
> > > set,  then HW treat  that transmit packet as a non-tunneling packet. It is able to
> > meet Method B.2.
> > >
> > > Is the explanation not clear?
> >
> > Ok, and how I can set method A (testpmd treat all packets as non-tunnelling and
> > never look beyond outer L4 header) then?
> > Konstantin
> 
> 
> > > > > As to case A, I think it is not mandatory to cover it in csum fwd
> > > > > engine for tunneling packet.
> 
> If you think the case A is essential, and it must be covered in  csum fwd, then we can add a command:
> 
> tx_checksum set sw-tunnel-mode (on|off)  (port-id)
> 
> if the 'off' is set ,  csum fwd engine don't check  if that packet is a tunneling packet and treat all packets as non-tunneling and never
> look beyond outer L4 header.
> 
> if the 'on' is set,  csum fwd engine will check if that packet is a tunneling packet.
> 
> And we are able to test all of cases in  http://dpdk.org/ml/archives/dev/2014-December/009213.html
> 
> Test case A:
> 
> tx_checksum set sw-tunnel-mode off
> tx_checksum set hw-tunnel-mode off
> tx_checksum set  ip   hw
> 
> test case B.1:
> 
> tx_checksum set sw-tunnel-mode on
> tx_checksum set hw-tunnel-mode on
> tx_checksum set  ip   hw
> tx_checksum set  tcp   hw
> 
> test case B.2:
> 
> tx_checksum set sw-tunnel-mode on
> tx_checksum set hw-tunnel-mode off
> tx_checksum set  ip   hw
> 
> test case C:
> 
> tx_checksum set sw-tunnel-mode on
> tx_checksum set hw-tunnel-mode on
> tx_checksum set  outer-ip   hw
> tx_checksum set  ip   hw
> tx_checksum set  tcp   hw
> 
> 
>  In addition, the reason of discarding ' tx_checksum set  tunnel (hw|sw|none) (port-id)' command is that  user probably confuse the
> following case.
>  tx_checksum set  tunnel sw
> tx_checksum set  ip   hw
> 
> In fact, we are still using hardware TX checksum offload in this case,  but the command " tx_checksum set  tunnel sw"  seems tell user
> that software compute the checksum.

So  ' set hw-tunnel-mode on' implies that ' sw-tunnel-mode' is already set to 'on'?
What would happen if user would do:
tx_checksum set sw-tunnel-mode off
tx_checksum set hw-tunnel-mode on
Or this combination is not allowed? 

As long as we have all 4 cases covered, I don't really care would it be:
tx_checksum set  tunnel (hw|sw|none)
or 
tx_checksum set sw-tunnel-mode   (on|off)
tx_checksum set hw-tunnel-mode  (on|off)

Though having 2 commands instead of 1 seems like overcomplicating and  more error prone...
Might be:
tx_checksum set tunnel-mode (none | hw | sw)
?

Konstantin



> 
> > >
> > > >
> > > > >
> > > > > 2. tx_checksum set outer-ip (hw|sw) (port-id) 3. tx_checksum set
> > > > > (ip|udp|tcp|sctp) (hw|sw) (port-id)
> > > > >
> > > > > The command 2 will be merged into command 3, the new command is '
> > > > > tx_checksum set  (outer-ip|ip|udp|tcp|sctp) (hw|sw) (port- id)'.
> > > > >
> > > > > These most of the cases in
> > > > > http://dpdk.org/ml/archives/dev/2014-December/009213.html will be
> > > > > covered by using the two commands
> > > > >
> > > > > The command 'tx_checksum set hw-tunnel-mode (on|off)  (port-id)'
> > > > > is used to set/clear  TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM flag.
> > > > > Actually, the PKT_TX_UDP_TUNNEL_PKT offload flag will be set if
> > > > > the testpmd flag is set, which tell driver/HW treat  that transmit
> > > > > packet as a
> > > > tunneling packet.
> > > > >
> > > > > When 'on' is set, which is able to meet Method B.1 and method C.
> > > > >
> > > > > When 'off' is set, the TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM is not
> > > > > needed to set, so the PKT_TX_UDP_TUNNEL_PKT offload flag is not
> > > > > needed to set,  then
> > > > HW treat  that transmit packet as a non-tunneling packet. It is able
> > > > to meet Method B.2.
> > > > >
> > > > > As to case A, I think it is not mandatory to cover it in csum fwd
> > > > > engine for
> > > > tunneling packet.
> > > > >
> > > > > Is the above description clear for you?
> > > > >
> > > > > > Regards,
> > > > > > Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Olivier MATZ]

Hi,

Thank you Jijiang for taking the time to get back on this.

On 01/08/2015 11:54 AM, Ananyev, Konstantin wrote:
>> And we are able to test all of cases in  http://dpdk.org/ml/archives/dev/2014-December/009213.html
>>
>> Test case A:
>>
>> tx_checksum set sw-tunnel-mode off
>> tx_checksum set hw-tunnel-mode off
>> tx_checksum set  ip   hw
>>
>> test case B.1:
>>
>> tx_checksum set sw-tunnel-mode on
>> tx_checksum set hw-tunnel-mode on
>> tx_checksum set  ip   hw
>> tx_checksum set  tcp   hw
>>
>> test case B.2:
>>
>> tx_checksum set sw-tunnel-mode on
>> tx_checksum set hw-tunnel-mode off
>> tx_checksum set  ip   hw
>>
>> test case C:
>>
>> tx_checksum set sw-tunnel-mode on
>> tx_checksum set hw-tunnel-mode on
>> tx_checksum set  outer-ip   hw
>> tx_checksum set  ip   hw
>> tx_checksum set  tcp   hw

There is something I don't understand. A forward engine takes any packet
in input and output. Packets can be of any kind (eth/arp, eth/ip/tcp,
eth/vlan/ip/udp/vxlan/eth/ip/tcp, ...)

Today, the behavior of csum forward engine is defined for any kind of
packet. See the description and the table in
http://dpdk.org/ml/archives/dev/2014-December/009886.html

All packets that are not "Ether[/vlan]/(IP|IP6)/(UDP|TCP|SCTP)" or
"Ether[/vlan]/(IP|IP6)/UDP/VxLAN/Ether[/vlan]/(IP|IP6)/UDP|TCP|SCTP"
are forwarded without beeing modified.

In my understanding, the use-case you are describing correspond to
specific packet types. So a configuration would work only for one
packet format only. Is it correct?

I think that the test-pmd command API should define a behavior for
the csum forward engine for any packet. What do you think?

Regards,
Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Liu, Jijiang]

Hi,

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Friday, January 9, 2015 6:45 PM
> To: Ananyev, Konstantin; Liu, Jijiang
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and
> csum forwarding engine
> 
> Hi,
> 
> Thank you Jijiang for taking the time to get back on this.
> 
> On 01/08/2015 11:54 AM, Ananyev, Konstantin wrote:
> >> And we are able to test all of cases in
> >> http://dpdk.org/ml/archives/dev/2014-December/009213.html
> >>
> >> Test case A:
> >>
> >> tx_checksum set sw-tunnel-mode off
> >> tx_checksum set hw-tunnel-mode off
> >> tx_checksum set  ip   hw
> >>
> >> test case B.1:
> >>
> >> tx_checksum set sw-tunnel-mode on
> >> tx_checksum set hw-tunnel-mode on
> >> tx_checksum set  ip   hw
> >> tx_checksum set  tcp   hw
> >>
> >> test case B.2:
> >>
> >> tx_checksum set sw-tunnel-mode on
> >> tx_checksum set hw-tunnel-mode off
> >> tx_checksum set  ip   hw
> >>
> >> test case C:
> >>
> >> tx_checksum set sw-tunnel-mode on
> >> tx_checksum set hw-tunnel-mode on
> >> tx_checksum set  outer-ip   hw
> >> tx_checksum set  ip   hw
> >> tx_checksum set  tcp   hw
> 
> There is something I don't understand. A forward engine takes any packet in input
> and output. Packets can be of any kind (eth/arp, eth/ip/tcp,
> eth/vlan/ip/udp/vxlan/eth/ip/tcp, ...)
Yes.

> Today, the behavior of csum forward engine is defined for any kind of packet. See
> the description and the table in http://dpdk.org/ml/archives/dev/2014-
> December/009886.html
> 
> All packets that are not "Ether[/vlan]/(IP|IP6)/(UDP|TCP|SCTP)" or
> "Ether[/vlan]/(IP|IP6)/UDP/VxLAN/Ether[/vlan]/(IP|IP6)/UDP|TCP|SCTP"
> are forwarded without beeing modified.
Yes

> In my understanding, the use-case you are describing correspond to specific
> packet types. So a configuration would work only for one packet format only. Is it
> correct?

The use-cases I described correspond to all tunneling packet type, not just for "Ether[/vlan]/(IP|IP6)/UDP/VxLAN/Ether[/vlan]/(IP|IP6)/UDP|TCP|SCTP"  packet format.
Our goal is to have TX checksum framework that can cover all the case in the http://dpdk.org/ml/archives/dev/2014-December/009213.html  and all packet types in csum fwd engine.
If so, it is easy to support other tunneling type in csum fwd engine in the future. 

There are some examples for the different packet types:

1. For L2 Packet types:
MAC, ARP
MAC, PAY2
...
They are forwarded without beeing modified no matter if these above commands are set.

 2. For Non Tunneled IPv4/6 packet
MAC, IPV4, UDP, PAY4
MAC, IPV6, UDP, PAY4
...
Ipv4:
tx_checksum set  ip   hw
tx_checksum set  udp   hw

IPv6:
tx_checksum set  udp   hw

They are forwarded with TX checksum offload if these above commands are set.

3. For Tunneled IPv4/6 packet

See the above test cases:
Test case A
test case B.1
test case B.2
test case C

They are forwarded with TX checksum offload if these above commands are set.

> I think that the test-pmd command API should define a behavior for the csum
> forward engine for any packet. What do you think?

Agree.

Let me explain the checksum offload behavior of different packet type below,

1. For L2 Packet types:
Checksum offload behavior definition:
tx_checksum set sw-tunnel-mode on :               NONE
tx_checksum set hw-tunnel-mode on:               NONE
tx_checksum set  outer-ip|ip|tcp|udp|sctp   hw:     NONE

2. For Non Tunneled IPv4/6 packet

Checksum offload behavior definition:

tx_checksum set sw-tunnel-mode on :                NONE
tx_checksum set hw-tunnel-mode on:                 NONE      
tx_checksum set  outer-ip|ip|tcp|udp|sctp   hw:     ip|tcp|udp|sctp options   are VALID

3. For Tunneled IPv4/6 packet
Checksum offload behavior definition:

tx_checksum set sw-tunnel-mode on :                VALID
tx_checksum set hw-tunnel-mode on:                 VALID 
tx_checksum set  outer-ip|ip|tcp|udp|sctp   hw:     VALID

It is very welcome if you have better solution that is able to cover all the case in the http://dpdk.org/ml/archives/dev/2014-December/009213.html  and all packet types in csum fwd engine.

> 
> Regards,
> Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Olivier MATZ]

Hi Jijiang,

Please find some comments below.

On 01/12/2015 04:41 AM, Liu, Jijiang wrote:
> There are some examples for the different packet types:
> 
> 1. For L2 Packet types:
> MAC, ARP
> MAC, PAY2
> ...
> They are forwarded without beeing modified no matter if these above commands are set.

ok

>  2. For Non Tunneled IPv4/6 packet
> MAC, IPV4, UDP, PAY4
> MAC, IPV6, UDP, PAY4
> ...
> Ipv4:
> tx_checksum set  ip   hw
> tx_checksum set  udp   hw
> 
> IPv6:
> tx_checksum set  udp   hw
> 
> They are forwarded with TX checksum offload if these above commands are set.

Two questions here:

- today, we also have the "sw" argument that allows to calculate the
  checksum in software. Do you plan to keep this behavior?

- today, the csumonly forward engine modifies the IP addresses to
  validate that it is able to recalculate the checksum. Do you plan
  to keep this behavior? I'm not opposed to remove it if it makes
  the code more complex.

> 3. For Tunneled IPv4/6 packet
> 
> See the above test cases:
> Test case A
> test case B.1
> test case B.2
> test case C
> 
> They are forwarded with TX checksum offload if these above commands are set.
> 
>> I think that the test-pmd command API should define a behavior for the csum
>> forward engine for any packet. What do you think?
> 
> Agree.
> 
> Let me explain the checksum offload behavior of different packet type below,
> 
> 1. For L2 Packet types:
> Checksum offload behavior definition:
> tx_checksum set sw-tunnel-mode on :               NONE
> tx_checksum set hw-tunnel-mode on:               NONE
> tx_checksum set  outer-ip|ip|tcp|udp|sctp   hw:     NONE
> 
> 2. For Non Tunneled IPv4/6 packet
> 
> Checksum offload behavior definition:
> 
> tx_checksum set sw-tunnel-mode on :                NONE
> tx_checksum set hw-tunnel-mode on:                 NONE      
> tx_checksum set  outer-ip|ip|tcp|udp|sctp   hw:     ip|tcp|udp|sctp options   are VALID
> 
> 3. For Tunneled IPv4/6 packet
> Checksum offload behavior definition:
> 
> tx_checksum set sw-tunnel-mode on :                VALID
> tx_checksum set hw-tunnel-mode on:                 VALID 
> tx_checksum set  outer-ip|ip|tcp|udp|sctp   hw:     VALID
> 
> It is very welcome if you have better solution that is able to cover all the case in the http://dpdk.org/ml/archives/dev/2014-December/009213.html  and all packet types in csum fwd engine.

Thank you for your efforts to explain your proposition. I still have
some difficulties to understand the naming "sw-tunnel" and "hw-tunnel".
>From the user point of view "sw" means "software" and "hw" means
"hardware". I think it's difficult to understand how both can be on
at the same time. Maybe it's just a naming problem?

Also, is it still possible to compute the checksum in software?

And will it be possible to support future hardware that will be able
to compute both outer l3, outer l4, l3 and l4 checksums?


I have another idea, please let me know if you find it clearer or not.
The commands format would be:

tx_checksum <pkt-type> <field1> <action1> <field2> <action2> ...

List of commands:

# select behavior for non tunnel packets
tx_checksum ip-udp l3 off|sw|hw l4 off|sw|hw
tx_checksum ip-tcp l3 off|sw|hw l4 off|sw|hw
tx_checksum ip-sctp l3 off|sw|hw l4 off|sw|hw
tx_checksum ip-other l3 off|sw|hw

# select behavior for vxlan packets
tx_checksum vxlan outer-l3 off|sw|hw outer-l4 off|sw|hw
tx_checksum vxlan-ip-udp l3 off|sw|hw l4 off|sw|hw
tx_checksum vxlan-ip-tcp l3 off|sw|hw l4 off|sw|hw
tx_checksum vxlan-ip-sctp l3 off|sw|hw l4 off|sw|hw
tx_checksum vxlan-ip-other l3 off|sw|hw

Examples:

1. calculate l3 and l4 checksum of ip/udp packets in hw, and ip/tcp
   packets in sw. Do nothing for the other packet types

# assume all is off by default
tx_checksum ip-udp l3 hw l4 hw
tx_checksum ip-tcp l3 sw l4 sw

2. calculate outer checksums of tunnel packets (your case A.)

# assume all is off by default
tx_checksum vxlan outer-l3 hw outer-l4 hw

3. calculate inner checksums of tunnel packets (your case B.1)

# assume all is off by default
tx_checksum vxlan-ip-udp l3 hw l4 hw
tx_checksum vxlan-ip-tcp l3 hw l4 hw
tx_checksum vxlan-ip-sctp l3 hw l4 hw

4. calculate all checksums of tunnel packets (your case C)

# assume all is off by default
tx_checksum vxlan outer-l3 hw outer-l4 hw
tx_checksum vxlan-ip-udp l3 hw l4 hw
tx_checksum vxlan-ip-tcp l3 hw l4 hw
tx_checksum vxlan-ip-sctp l3 hw l4 hw


Advantages:

- clearer from use point of view: the user knows what is done for
  each packet type

- software checksum is supported for comparison with hw

- the syntax already supports future hw that can do outer l3, outer l4,
  l3 and l4 at the same time.

- we can add future tunnel packets in the same model:

  tx_checksum gre outer-l3 off|sw|hw
  tx_checksum gre-ip-udp l3 off|sw|hw l4 off|sw|hw

Cons:

- with the definition above, we cannot do B.2. But if we really want
  it, we could change the commands:

  tx_checksum xxx l3 off|sw|hw|outer-hw l4 off|sw|hw|outer-hw

  "outer-hw" means: use the outer mbuf flags to do the checksum
  It could be replaced in all commands

- this commands may lead to impossible configurations, but it can be
  checked by testpmd and a warning can be displayed.


What do you think?

Regards,
Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Liu, Jijiang]

Hi,

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Monday, January 12, 2015 7:43 PM
> To: Liu, Jijiang; Ananyev, Konstantin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and
> csum forwarding engine
> 
> Hi Jijiang,
> 
> Please find some comments below.
> 
> On 01/12/2015 04:41 AM, Liu, Jijiang wrote:
> > There are some examples for the different packet types:
> >
> > 1. For L2 Packet types:
> > MAC, ARP
> > MAC, PAY2
> > ...
> > They are forwarded without beeing modified no matter if these above
> commands are set.
> 
> ok
> 
> >  2. For Non Tunneled IPv4/6 packet
> > MAC, IPV4, UDP, PAY4
> > MAC, IPV6, UDP, PAY4
> > ...
> > Ipv4:
> > tx_checksum set  ip   hw
> > tx_checksum set  udp   hw
> >
> > IPv6:
> > tx_checksum set  udp   hw
> >
> > They are forwarded with TX checksum offload if these above commands are
> set.
> 
> Two questions here:
> 
> - today, we also have the "sw" argument that allows to calculate the
>   checksum in software. Do you plan to keep this behavior?

Yes

> - today, the csumonly forward engine modifies the IP addresses to
>   validate that it is able to recalculate the checksum. Do you plan
>   to keep this behavior? 
Yes

> I'm not opposed to remove it if it makes
>   the code more complex.


> > 3. For Tunneled IPv4/6 packet
> >
> > See the above test cases:
> > Test case A
> > test case B.1
> > test case B.2
> > test case C
> >
> > They are forwarded with TX checksum offload if these above commands are
> set.
> >
> >> I think that the test-pmd command API should define a behavior for
> >> the csum forward engine for any packet. What do you think?
> >
> > Agree.
> >
> > Let me explain the checksum offload behavior of different packet type
> > below,
> >
> > 1. For L2 Packet types:
> > Checksum offload behavior definition:
> > tx_checksum set sw-tunnel-mode on :               NONE
> > tx_checksum set hw-tunnel-mode on:               NONE
> > tx_checksum set  outer-ip|ip|tcp|udp|sctp   hw:     NONE
> >
> > 2. For Non Tunneled IPv4/6 packet
> >
> > Checksum offload behavior definition:
> >
> > tx_checksum set sw-tunnel-mode on :                NONE
> > tx_checksum set hw-tunnel-mode on:                 NONE
> > tx_checksum set  outer-ip|ip|tcp|udp|sctp   hw:     ip|tcp|udp|sctp options
> are VALID
> >
> > 3. For Tunneled IPv4/6 packet
> > Checksum offload behavior definition:
> >
> > tx_checksum set sw-tunnel-mode on :                VALID
> > tx_checksum set hw-tunnel-mode on:                 VALID
> > tx_checksum set  outer-ip|ip|tcp|udp|sctp   hw:     VALID
> >
> > It is very welcome if you have better solution that is able to cover all the case in
> the http://dpdk.org/ml/archives/dev/2014-December/009213.html  and all
> packet types in csum fwd engine.
> 
> Thank you for your efforts to explain your proposition. I still have some difficulties
> to understand the naming "sw-tunnel" and "hw-tunnel".


Again,  I'd like to explain what behaviors  the following two commands are.

1. tx_checksum set sw-tunnel-mode on/off

2. tx_checksum set hw-tunnel-mode on/off

For command 1, If the sw-tunnel-mode is set/clear, which will set/clear a testpmd flag that is used in the process of analyzing incoming packet., the pseudo-codes are list below,

If (sw-tunnel-mode) 

	Csum fwd engine will analyze if incoming packet is a tunneling packet.
               tunnel = 1;
else
           Csum fwd engine will not analyze if incoming packet is a tunneling packet, and treat all the incoming packets as non-tunneling packets. 
           It is used for A.


For command 2, If the hw-tunnel-mode is set/clear, which will set/clear a testpmd flag that is used in the process of how to handle tunneling packet, the pseudo-codes are list below,

if (tunnel == 1) { // this is a tunneling packet
            If (hw-tunnel-mode) 
                      ol_flags |= PKT_TX_UDP_TUNNEL_PKT;

	       Csum fwd engine set PKT_TX_UDP_TUNNEL_PKT offload flag, which means to tell HW treat  the transmit packet as a tunneling packet to do checksum offload.
	       It is used for B.1
           Else
                      Csum fwd engine doesn't  set PKT_TX_UDP_TUNNEL_PKT offload flag, which means  tell HW to treat the packet as ordinary (non-tunnelled) packet.
	      It is used for B.2
}


> From the user point of view "sw" means "software" and "hw" means "hardware".
> I think it's difficult to understand how both can be on at the same time. Maybe it's
> just a naming problem?
>


Yes.
Your comments make sense. And I think it's just a naming problem, I will combine the two hw/sw-tunnel-mode commands into a command in order to make it  as simple and  understandable as possible.

tx_checksum set tunnel-mode (hw|none)

when user set 'hw' option,   the TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM flag will be set in cmdline; actually, the PKT_TX_UDP_TUNNEL_PKT offload flag will be set if  the testpmd flag is set, which tell driver/HW treat  that  transmit packet as a tunneling packet.

When user set 'none' option, which means software and hardware will treat that packet as ordinary packet(non-tunneling).

List of commands for those cases.

Test case A:
tx_checksum set tunnel-mode none
tx_checksum set  ip   hw

test case B.1:
tx_checksum set tunnel-mode hw
tx_checksum set  ip   hw
tx_checksum set  tcp   hw

test case B.2:
tx_checksum set  ip   hw

test case C:
tx_checksum set tunnel-mode hw
tx_checksum set  outer-ip   hw
tx_checksum set  ip   hw
tx_checksum set  tcp   hw



> Also, is it still possible to compute the checksum in software?
Yes

> And will it be possible to support future hardware that will be able to compute
> both outer l3, outer l4, l3 and l4 checksums?

Yes. 
Currently, i40e support outer l3, outer l4, l3 and l4 checksums offload at the same time.

test case C:
tx_checksum set tunnel-mode hw
tx_checksum set  outer-ip   hw
tx_checksum set  ip   hw
tx_checksum set  tcp   hw

Of course, outer udp is not listed here for VXLAN.

> 
> I have another idea, please let me know if you find it clearer or not.
> The commands format would be:
> 
> tx_checksum <pkt-type> <field1> <action1> <field2> <action2> ...
> 
> List of commands:
> 
> # select behavior for non tunnel packets

 > tx_checksum ip-udp l3 off|sw|hw l4 off|sw|hw 

> tx_checksum ip-tcp l3 off|sw|hw l4 off|sw|hw 

> tx_checksum ip-sctp l off|sw|hw l4 off|sw|hw

 > tx_checksum ip-other l3 off|sw|hw

> 
> # select behavior for vxlan packets
> tx_checksum vxlan outer-l3 off|sw|hw outer-l4 off|sw|hw

> tx_checksum vxlan- ip-udp l3 off|sw|hw l4 off|sw|hw 

> tx_checksum vxlan-ip-tcp l3 off|sw|hw l4off|sw|hw 

>tx_checksum vxlan-ip-sctp l3 off|sw|hw l4 off|sw|hw 

> tx_checksum vxlan-ip-other l3 off|sw|hw

> 
> Examples:
> 
> 1. calculate l3 and l4 checksum of ip/udp packets in hw, and ip/tcp
>    packets in sw. Do nothing for the other packet types
> 
> # assume all is off by default
> tx_checksum ip-udp l3 hw l4 hw
> tx_checksum ip-tcp l3 sw l4 sw
> 
> 2. calculate outer checksums of tunnel packets (your case A.)
> 
> # assume all is off by default
> tx_checksum vxlan outer-l3 hw outer-l4 hw
> 
> 3. calculate inner checksums of tunnel packets (your case B.1)
> 
> # assume all is off by default
> tx_checksum vxlan-ip-udp l3 hw l4 hw
> tx_checksum vxlan-ip-tcp l3 hw l4 hw
> tx_checksum vxlan-ip-sctp l3 hw l4 hw
> 
> 4. calculate all checksums of tunnel packets (your case C)
> 
> # assume all is off by default
> tx_checksum vxlan outer-l3 hw outer-l4 hw tx_checksum vxlan-ip-udp l3 hw l4
> hw tx_checksum vxlan-ip-tcp l3 hw l4 hw tx_checksum vxlan-ip-sctp l3 hw l4 hw
> 
> 
> Advantages:
> 
> - clearer from use point of view: the user knows what is done for
>   each packet type
> 
> - software checksum is supported for comparison with hw
> 
> - the syntax already supports future hw that can do outer l3, outer l4,
>   l3 and l4 at the same time.
> 
> - we can add future tunnel packets in the same model:
> 
>   tx_checksum gre outer-l3 off|sw|hw
>   tx_checksum gre-ip-udp l3 off|sw|hw l4 off|sw|hw
> 
> Cons:
> 
> - with the definition above, we cannot do B.2. But if we really want
>   it, we could change the commands:
> 
>   tx_checksum xxx l3 off|sw|hw|outer-hw l4 off|sw|hw|outer-hw
> 
>   "outer-hw" means: use the outer mbuf flags to do the checksum
>   It could be replaced in all commands
> 
> - this commands may lead to impossible configurations, but it can be
>   checked by testpmd and a warning can be displayed.
> 
> 
> What do you think?

Thanks for your proposal.
It is clear for me.

But there are two questions for me.

As I know, in current command line framework, the option in command line is exact match, so you probably have to add duplicated codes when you want to support a new packet types.

Other question:

Currently, the following testpmd flag is for per port, not for per packet type, when they are set, which will affect whole port, not just for packet type or format, if you  add  <pkt-type> option in cmdline, which means you have to other changes.

/** Offload IP checksum in csum forward engine */
#define TESTPMD_TX_OFFLOAD_IP_CKSUM          0x0001
/** Offload UDP checksum in csum forward engine */
#define TESTPMD_TX_OFFLOAD_UDP_CKSUM         0x0002
/** Offload TCP checksum in csum forward engine */
#define TESTPMD_TX_OFFLOAD_TCP_CKSUM         0x0004
/** Offload SCTP checksum in csum forward engine */
#define TESTPMD_TX_OFFLOAD_SCTP_CKSUM        0x0008
/** Offload VxLAN checksum in csum forward engine */
#define TESTPMD_TX_OFFLOAD_VXLAN_CKSUM       0x0010

Of course, it is welcome if you can send this patch set with this idea for community review.

> Regards,
> Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Olivier MATZ]

Hi Jijiang,

On 01/13/2015 04:04 AM, Liu, Jijiang wrote:
> the following two commands are.
>
> 1. tx_checksum set sw-tunnel-mode on/off
>
> 2. tx_checksum set hw-tunnel-mode on/off
>
> For command 1, If the sw-tunnel-mode is set/clear, which will set/clear a testpmd flag that is used in the process of analyzing incoming packet., the pseudo-codes are list below,
>
> If (sw-tunnel-mode)
>
> 	Csum fwd engine will analyze if incoming packet is a tunneling packet.
>                 tunnel = 1;
> else
>             Csum fwd engine will not analyze if incoming packet is a tunneling packet, and treat all the incoming packets as non-tunneling packets.
>             It is used for A.

What about "recognize-tunnel" instead of "sw-tunnel-mode"?
Or "parse-tunnel"?

To me, using "sw-" or "hw-" prefix is confusing because in any case
the checksums can be calculated in software or hardware depending on
"tx_checksum set outer-ip hw|sw".

Moreover, this command has an impact on receive side, but the name
is still "tx_checksum". Maybe this is also confusing.

> For command 2, If the hw-tunnel-mode is set/clear, which will set/clear a testpmd flag that is used in the process of how to handle tunneling packet, the pseudo-codes are list below,
>
> if (tunnel == 1) { // this is a tunneling packet
>              If (hw-tunnel-mode)
>                        ol_flags |= PKT_TX_UDP_TUNNEL_PKT;
>
> 	       Csum fwd engine set PKT_TX_UDP_TUNNEL_PKT offload flag, which means to tell HW treat  the transmit packet as a tunneling packet to do checksum offload.
> 	       It is used for B.1
>             Else
>                        Csum fwd engine doesn't  set PKT_TX_UDP_TUNNEL_PKT offload flag, which means  tell HW to treat the packet as ordinary (non-tunnelled) packet.
> 	      It is used for B.2
> }

What about:
   tx_checksum set tunnel-method normal|outer

It would select if we use lX_len or outer_lX_len. Is it what you mean?

And this only makes sense when we use hw checksum right?

>> And will it be possible to support future hardware that will be able to compute
>> both outer l3, outer l4, l3 and l4 checksums?
>
> Yes.
> Currently, i40e support outer l3, outer l4, l3 and l4 checksums offload at the same time.

I probably missed something here: we only have PKT_TX_OUTER_IP_CKSUM
but there is no PKT_TX_OUTER_UDP_CKSUM. Is outer UDP checksum supported

> test case C:
> tx_checksum set tunnel-mode hw
> tx_checksum set  outer-ip   hw
> tx_checksum set  ip   hw
> tx_checksum set  tcp   hw
>
> Of course, outer udp is not listed here for VXLAN.

I don't understand why. Could you detail it?

>> I have another idea, please let me know if you find it clearer or not.
>> The commands format would be:
>>
>> tx_checksum <pkt-type> <field1> <action1> <field2> <action2> ...
>>
>> [...]
>>
>> What do you think?
>
> Thanks for your proposal.
> It is clear for me.
>
> But there are two questions for me.
>
> As I know, in current command line framework, the option in command line is exact match, so you probably have to add duplicated codes when you want to support a new packet types.

I don't think it's really a problem. The cmdline library supports
string list, so can have the following 3 commands definitions:

1. tx_checksum 
ip-udp|ip-tcp|ip-sctp|vxlan-ip-udp|vxlan-ip-tcp|vxlan-ip-sctp l3 
off|sw|hw l4 off|sw|hw
2. tx_checksum ip-other|vxlan-ip-other l3 off|sw|hw
3. tx_checksum vxlan outer-l3 off|sw|hw outer-l4 off|sw|hw

Maybe 1 and 2 could be splitted in non-vxlan and vxlan. But only
the structure should be redefined to have a different help string,
not the callback function.

> Other question:
>
> Currently, the following testpmd flag is for per port, not for per packet type, when they are set, which will affect whole port, not just for packet type or format, if you  add  <pkt-type> option in cmdline, which means you have to other changes.
>
> /** Offload IP checksum in csum forward engine */
> #define TESTPMD_TX_OFFLOAD_IP_CKSUM          0x0001
> /** Offload UDP checksum in csum forward engine */
> #define TESTPMD_TX_OFFLOAD_UDP_CKSUM         0x0002
> /** Offload TCP checksum in csum forward engine */
> #define TESTPMD_TX_OFFLOAD_TCP_CKSUM         0x0004
> /** Offload SCTP checksum in csum forward engine */
> #define TESTPMD_TX_OFFLOAD_SCTP_CKSUM        0x0008
> /** Offload VxLAN checksum in csum forward engine */
> #define TESTPMD_TX_OFFLOAD_VXLAN_CKSUM       0x0010

We can add a portid in each command.

> Of course, it is welcome if you can send this patch set with this idea for community review.

Let's first agree on the user API :)

Regards,
Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Liu, Jijiang]

Hi Olivier,

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, January 13, 2015 5:56 PM
> To: Liu, Jijiang
> Cc: dev@dpdk.org; Ananyev, Konstantin
> Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and
> csum forwarding engine
> 
> Hi Jijiang,
> 
> On 01/13/2015 04:04 AM, Liu, Jijiang wrote:
> > the following two commands are.
> >
> > 1. tx_checksum set sw-tunnel-mode on/off
> >
> > 2. tx_checksum set hw-tunnel-mode on/off
> >
> > For command 1, If the sw-tunnel-mode is set/clear, which will
> > set/clear a testpmd flag that is used in the process of analyzing
> > incoming packet., the pseudo-codes are list below,
> >
> > If (sw-tunnel-mode)
> >
> > 	Csum fwd engine will analyze if incoming packet is a tunneling packet.
> >                 tunnel = 1;
> > else
> >             Csum fwd engine will not analyze if incoming packet is a tunneling
> packet, and treat all the incoming packets as non-tunneling packets.
> >             It is used for A.
> 
> What about "recognize-tunnel" instead of "sw-tunnel-mode"?
> Or "parse-tunnel"?

Ok,  "parse-tunnel" or "parse-tunnel-pkt" is better.
Thanks.


> To me, using "sw-" or "hw-" prefix is confusing because in any case the checksums
> can be calculated in software or hardware depending on "tx_checksum set outer-
> ip hw|sw".
> 
> Moreover, this command has an impact on receive side, but the name is still
> "tx_checksum". Maybe this is also confusing.
Ok,  how about this?

set  checksum parse-tunnel-pkt on|off  (port-id)

> > For command 2, If the hw-tunnel-mode is set/clear, which will
> > set/clear a testpmd flag that is used in the process of how to handle
> > tunneling packet, the pseudo-codes are list below,
> >
> > if (tunnel == 1) { // this is a tunneling packet
> >              If (hw-tunnel-mode)
> >                        ol_flags |= PKT_TX_UDP_TUNNEL_PKT;
> >
> > 	       Csum fwd engine set PKT_TX_UDP_TUNNEL_PKT offload flag, which
> means to tell HW treat  the transmit packet as a tunneling packet to do checksum
> offload.
> > 	       It is used for B.1
> >             Else
> >                        Csum fwd engine doesn't  set PKT_TX_UDP_TUNNEL_PKT offload
> flag, which means  tell HW to treat the packet as ordinary (non-tunnelled) packet.
> > 	      It is used for B.2
> > }
> 
> What about:
>    tx_checksum set tunnel-method normal|outer
> It would select if we use lX_len or outer_lX_len. Is it what you mean?
        
tx_checksum set tunnel-method normal|outer

Let me explain that what differences of  TX checksum mechanism between ixgbe(82599) and i40e(40G NIC) are.

For 82599, there is only one register that is used for L3 checksum offload. So for tunneling packet, hardware is unable to recognize the packet is tunneling packet and  the register cannot be worked for both outer L3 checksum offload and inner L3 checksum offload at the same time,  just for outer or inner.

For i40e(40G NIC),  there are two registers that are user for L3 TX checksum offload, so for tunneling packet, the outer and inner L3 checksum offload  can be done by hardware at the same time, but a prerequisite is that we must tell
Hardware the packet is a tunneling packet by setting a register (PKT_TX_UDP_TUNNEL_PKT offload flag is used to indicate to set this register.)

As for other NIC, I think its working mechanism should be same as the i40e if it can recognize tunneling packet.

So my idea:
tx_checksum set tunnel-method  tunnel-pkt on|off

or
tx_checksum set tunnel-pkt on|off

What do you think?

                                                                                                                                                                                                                                                                                                                                                                                                                                                               
> And this only makes sense when we use hw checksum right?
yes

> 
> >> And will it be possible to support future hardware that will be able
> >> to compute both outer l3, outer l4, l3 and l4 checksums?

Currently, if outer l4  will be supported in the future, and we can add outer-udp/tcp option into following command.
Tx_checksum set outer-ip|ip|sctp|udp|tcp.


> > Yes.
> > Currently, i40e support outer l3, outer l4, l3 and l4 checksums offload at the
> same time.
Sorry, my bad.
I40e just support outer l3, l3 and l4.

Fortville can offload the following L3 and L4 integrity checks: IPv4 header(s) checksum for "simple" and tunneled packets, Inner TCP or UDP checksum and SCTP CRC integrity. Tunneling UDP headers and GRE header are not offloaded while Fortville leaves their checksum field as is. If a checksum is required, software should provide it as well as the inner checksum value(s) that are required for the outer checksum.

> 
> >> I have another idea, please let me know if you find it clearer or not.
> >> The commands format would be:
> >>
> >> tx_checksum <pkt-type> <field1> <action1> <field2> <action2> ...
> >>
> >> [...]
> >>
> >> What do you think?
> >
> > Thanks for your proposal.
> > It is clear for me.
> >
> > But there are two questions for me.
> >
> > As I know, in current command line framework, the option in command line is
> exact match, so you probably have to add duplicated codes when you want to
> support a new packet types.
> 
> I don't think it's really a problem. The cmdline library supports string list, so can
> have the following 3 commands definitions:
> 
> 1. tx_checksum
> ip-udp|ip-tcp|ip-sctp|vxlan-ip-udp|vxlan-ip-tcp|vxlan-ip-sctp l3
> off|sw|hw l4 off|sw|hw
> 2. tx_checksum ip-other|vxlan-ip-other l3 off|sw|hw 3. tx_checksum vxlan
> outer-l3 off|sw|hw outer-l4 off|sw|hw
> 
> Maybe 1 and 2 could be splitted in non-vxlan and vxlan. But only the structure
> should be redefined to have a different help string, not the callback function.


Ok, but I think you probably need to add many string in the list :)

> > Other question:
> >
> > Currently, the following testpmd flag is for per port, not for per packet type,
> when they are set, which will affect whole port, not just for packet type or format,
> if you  add  <pkt-type> option in cmdline, which means you have to other
> changes.
> >
> > /** Offload IP checksum in csum forward engine */
> > #define TESTPMD_TX_OFFLOAD_IP_CKSUM          0x0001
> > /** Offload UDP checksum in csum forward engine */
> > #define TESTPMD_TX_OFFLOAD_UDP_CKSUM         0x0002
> > /** Offload TCP checksum in csum forward engine */
> > #define TESTPMD_TX_OFFLOAD_TCP_CKSUM         0x0004
> > /** Offload SCTP checksum in csum forward engine */
> > #define TESTPMD_TX_OFFLOAD_SCTP_CKSUM        0x0008
> > /** Offload VxLAN checksum in csum forward engine */
> > #define TESTPMD_TX_OFFLOAD_VXLAN_CKSUM       0x0010
> 
> We can add a portid in each command.

Ok, but I think your idea will make the csum fwd engine more complicated.

> > Of course, it is welcome if you can send this patch set with this idea for
> community review.
> Let's first agree on the user API :)

If you don't have more comments and questions on my current solution, I will send new patch set.
Or you can send your patch.
Anyway, our goal is the same.

> 
> Regards,
> Olivier
> 
> 

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Ananyev, Konstantin]

Hi lads,

> -----Original Message-----
> From: Liu, Jijiang
> Sent: Wednesday, January 14, 2015 3:01 AM
> To: Olivier MATZ
> Cc: dev@dpdk.org; Ananyev, Konstantin
> Subject: RE: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine
> 
> Hi Olivier,
> 
> > -----Original Message-----
> > From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> > Sent: Tuesday, January 13, 2015 5:56 PM
> > To: Liu, Jijiang
> > Cc: dev@dpdk.org; Ananyev, Konstantin
> > Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and
> > csum forwarding engine
> >
> > Hi Jijiang,
> >
> > On 01/13/2015 04:04 AM, Liu, Jijiang wrote:
> > > the following two commands are.
> > >
> > > 1. tx_checksum set sw-tunnel-mode on/off
> > >
> > > 2. tx_checksum set hw-tunnel-mode on/off
> > >
> > > For command 1, If the sw-tunnel-mode is set/clear, which will
> > > set/clear a testpmd flag that is used in the process of analyzing
> > > incoming packet., the pseudo-codes are list below,
> > >
> > > If (sw-tunnel-mode)
> > >
> > > 	Csum fwd engine will analyze if incoming packet is a tunneling packet.
> > >                 tunnel = 1;
> > > else
> > >             Csum fwd engine will not analyze if incoming packet is a tunneling
> > packet, and treat all the incoming packets as non-tunneling packets.
> > >             It is used for A.
> >
> > What about "recognize-tunnel" instead of "sw-tunnel-mode"?
> > Or "parse-tunnel"?
> 
> Ok,  "parse-tunnel" or "parse-tunnel-pkt" is better.
> Thanks.
> 
> 
> > To me, using "sw-" or "hw-" prefix is confusing because in any case the checksums
> > can be calculated in software or hardware depending on "tx_checksum set outer-
> > ip hw|sw".
> >
> > Moreover, this command has an impact on receive side, but the name is still
> > "tx_checksum". Maybe this is also confusing.
> Ok,  how about this?
> 
> set  checksum parse-tunnel-pkt on|off  (port-id)
> 
> > > For command 2, If the hw-tunnel-mode is set/clear, which will
> > > set/clear a testpmd flag that is used in the process of how to handle
> > > tunneling packet, the pseudo-codes are list below,
> > >
> > > if (tunnel == 1) { // this is a tunneling packet
> > >              If (hw-tunnel-mode)
> > >                        ol_flags |= PKT_TX_UDP_TUNNEL_PKT;
> > >
> > > 	       Csum fwd engine set PKT_TX_UDP_TUNNEL_PKT offload flag, which
> > means to tell HW treat  the transmit packet as a tunneling packet to do checksum
> > offload.
> > > 	       It is used for B.1
> > >             Else
> > >                        Csum fwd engine doesn't  set PKT_TX_UDP_TUNNEL_PKT offload
> > flag, which means  tell HW to treat the packet as ordinary (non-tunnelled) packet.
> > > 	      It is used for B.2
> > > }
> >
> > What about:
> >    tx_checksum set tunnel-method normal|outer
> > It would select if we use lX_len or outer_lX_len. Is it what you mean?
> 
> tx_checksum set tunnel-method normal|outer
> 
> Let me explain that what differences of  TX checksum mechanism between ixgbe(82599) and i40e(40G NIC) are.
> 
> For 82599, there is only one register that is used for L3 checksum offload. So for tunneling packet, hardware is unable to recognize the
> packet is tunneling packet and  the register cannot be worked for both outer L3 checksum offload and inner L3 checksum offload at the
> same time,  just for outer or inner.
> 
> For i40e(40G NIC),  there are two registers that are user for L3 TX checksum offload, so for tunneling packet, the outer and inner L3
> checksum offload  can be done by hardware at the same time, but a prerequisite is that we must tell
> Hardware the packet is a tunneling packet by setting a register (PKT_TX_UDP_TUNNEL_PKT offload flag is used to indicate to set this
> register.)
> 
> As for other NIC, I think its working mechanism should be same as the i40e if it can recognize tunneling packet.
> 
> So my idea:
> tx_checksum set tunnel-method  tunnel-pkt on|off
> 
> or
> tx_checksum set tunnel-pkt on|off
> 
> What do you think?
> 
> 
> > And this only makes sense when we use hw checksum right?
> yes
> 
> >
> > >> And will it be possible to support future hardware that will be able
> > >> to compute both outer l3, outer l4, l3 and l4 checksums?
> 
> Currently, if outer l4  will be supported in the future, and we can add outer-udp/tcp option into following command.
> Tx_checksum set outer-ip|ip|sctp|udp|tcp.
> 
> 
> > > Yes.
> > > Currently, i40e support outer l3, outer l4, l3 and l4 checksums offload at the
> > same time.
> Sorry, my bad.
> I40e just support outer l3, l3 and l4.
> 
> Fortville can offload the following L3 and L4 integrity checks: IPv4 header(s) checksum for "simple" and tunneled packets, Inner TCP or
> UDP checksum and SCTP CRC integrity. Tunneling UDP headers and GRE header are not offloaded while Fortville leaves their checksum
> field as is. If a checksum is required, software should provide it as well as the inner checksum value(s) that are required for the outer
> checksum.
> 
> >
> > >> I have another idea, please let me know if you find it clearer or not.
> > >> The commands format would be:
> > >>
> > >> tx_checksum <pkt-type> <field1> <action1> <field2> <action2> ...
> > >>
> > >> [...]
> > >>
> > >> What do you think?
> > >
> > > Thanks for your proposal.
> > > It is clear for me.
> > >
> > > But there are two questions for me.
> > >
> > > As I know, in current command line framework, the option in command line is
> > exact match, so you probably have to add duplicated codes when you want to
> > support a new packet types.
> >
> > I don't think it's really a problem. The cmdline library supports string list, so can
> > have the following 3 commands definitions:
> >
> > 1. tx_checksum
> > ip-udp|ip-tcp|ip-sctp|vxlan-ip-udp|vxlan-ip-tcp|vxlan-ip-sctp l3
> > off|sw|hw l4 off|sw|hw
> > 2. tx_checksum ip-other|vxlan-ip-other l3 off|sw|hw 3. tx_checksum vxlan
> > outer-l3 off|sw|hw outer-l4 off|sw|hw
> >
> > Maybe 1 and 2 could be splitted in non-vxlan and vxlan. But only the structure
> > should be redefined to have a different help string, not the callback function.
> 
> 
> Ok, but I think you probably need to add many string in the list :)
> 
> > > Other question:
> > >
> > > Currently, the following testpmd flag is for per port, not for per packet type,
> > when they are set, which will affect whole port, not just for packet type or format,
> > if you  add  <pkt-type> option in cmdline, which means you have to other
> > changes.
> > >
> > > /** Offload IP checksum in csum forward engine */
> > > #define TESTPMD_TX_OFFLOAD_IP_CKSUM          0x0001
> > > /** Offload UDP checksum in csum forward engine */
> > > #define TESTPMD_TX_OFFLOAD_UDP_CKSUM         0x0002
> > > /** Offload TCP checksum in csum forward engine */
> > > #define TESTPMD_TX_OFFLOAD_TCP_CKSUM         0x0004
> > > /** Offload SCTP checksum in csum forward engine */
> > > #define TESTPMD_TX_OFFLOAD_SCTP_CKSUM        0x0008
> > > /** Offload VxLAN checksum in csum forward engine */
> > > #define TESTPMD_TX_OFFLOAD_VXLAN_CKSUM       0x0010
> >
> > We can add a portid in each command.
> 
> Ok, but I think your idea will make the csum fwd engine more complicated.
> 
> > > Of course, it is welcome if you can send this patch set with this idea for
> > community review.
> > Let's first agree on the user API :)
> 
> If you don't have more comments and questions on my current solution, I will send new patch set.
> Or you can send your patch.
> Anyway, our goal is the same.

To be honest, there are so many mails around that subject, so I am already lost :)
Oliver, as I understand you are not happy with the test-pmd commands Frank is proposing.
Both syntax and semantics.
Is that correct?
If so, could you suggest something from your side?
That would allow to configure test-pmd to behave in any of 4 possible ways we discussed previously:
http://dpdk.org/ml/archives/dev/2014-December/009213.html
Thanks
Konstantin

> 
> >
> > Regards,
> > Olivier
> >
> >

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Olivier MATZ]

Hi Konstantin, Hi Jijiang,

On 01/15/2015 02:31 PM, Ananyev, Konstantin wrote:
> To be honest, there are so many mails around that subject, so I am already lost :)
> Oliver, as I understand you are not happy with the test-pmd commands Frank is proposing.
> Both syntax and semantics.
> Is that correct?
> If so, could you suggest something from your side?
> That would allow to configure test-pmd to behave in any of 4 possible ways we discussed previously:
> http://dpdk.org/ml/archives/dev/2014-December/009213.html

I first wanted to send a mail to describe the current problem with
testpmd command line and the 2 solutions (Jijiang's and mine).
But, first, I think we need to fully clarify the checksum offload
API through examples as it will help to implement testpmd and do
the documentation. They are based on Jijiang's previous mail [1].

I will submit a patchset fixing the problems described below in
the coming days. If we agree on it, I'll submit another one for testpmd.

Let's use the following packet for all the examples below:
  out_eth / out_ip / out_udp / vxlan / in_eth / in_ip / in_tcp


The following cases are supposed to work on niantic and fortville:

case 1) calculate checksum of out_ip  (was case A in [1])

   mb->l2_len = len(out_eth)
   mb->l3_len = len(out_ip)
   mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_IP_CSUM
   set out_ip checksum to 0 in the packet

   supported on hardware advertising DEV_TX_OFFLOAD_IPV4_CKSUM

case 2) calculate checksum of out_ip and out_udp

   mb->l2_len = len(out_eth)
   mb->l3_len = len(out_ip)
   mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_IP_CSUM | PKT_TX_UDP_CKSUM
   set out_ip checksum to 0 in the packet
   set out_udp checksum to pseudo header using rte_ipv4_phdr_cksum()

   supported on hardware advertising DEV_TX_OFFLOAD_IPV4_CKSUM and
   DEV_TX_OFFLOAD_UDP_CKSUM

   *Problem 1*: The comment above PKT_TX_IPV4 says "Packet is IPv4
   without requiring IP checksum offload" [2], and the help of L4
   checksum and TSO says that it is required to set the PKT_TX_IPV4
   flag [3]. This is not coherent.

   We are back on the debate about the meaning of PKT_TX_IPV4 vs
   PKT_TX_IP_CSUM from [4]. This incoherency in comments are introduced
   by patch [5]. The question is "when an application should set
   this flag? for any IP packet that does not require IP checksum?".
   This would break many applications. I think a good definition would
   be:

   Packet is IPv4. This flag must be set when using any offload
   feature (TSO, L3 or L4 checksum) to tell the NIC that the packet
   is an IPv4 packet.

   That's why I added PKT_TX_IPV4 in the examples.

case 3) calculate checksum of in_ip

   mb->l2_len = len(out_eth + out_ip + out_udp + vxlan + in_eth)
   mb->l3_len = len(in_ip)
   mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_IP_CSUM
   set in_ip checksum to 0 in the packet

   This is similar to case 1), but l2_len is different.

   supported on hardware advertising DEV_TX_OFFLOAD_IPV4_CKSUM

   Note that it can only work if outer L4 checksum is 0.

case 4) calculate checksum of in_ip and in_tcp  (was case B.2 in [1])

   mb->l2_len = len(out_eth + out_ip + out_udp + vxlan + in_eth)
   mb->l3_len = len(in_ip)
   mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_IP_CSUM | PKT_TX_TCP_CKSUM
   set in_ip checksum to 0 in the packet
   set in_tcp checksum to pseudo header using rte_ipv4_phdr_cksum()

   This is similar to case 2), but l2_len is different.

   supported on hardware advertising DEV_TX_OFFLOAD_IPV4_CKSUM and
   DEV_TX_OFFLOAD_TCP_CKSUM

   Note that it can only work if outer L4 checksum is 0.

case 5) segment inner TCP

   mb->l2_len = len(out_eth + out_ip + out_udp + vxlan + in_eth)
   mb->l3_len = len(in_ip)
   mb->l4_len = len(in_tcp)
   mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_IP_CKSUM | PKT_TX_TCP_CKSUM |
     PKT_TX_TCP_SEG;
   set in_ip checksum to 0 in the packet
   set in_tcp checksum to pseudo header without including the IP
     payload length using rte_ipv4_phdr_cksum()

   supported on hardware advertising DEV_TX_OFFLOAD_TCP_TSO.

   Note that it can only work if outer L4 checksum is 0.

   Problem 1 is also visible here.


The following cases are supposed to *work on fortville*:

case 6) calculate checksum of out_ip, in_ip, in_tcp (was case C in [1])

   mb->outer_l2_len = len(out_eth)
   mb->outer_l3_len = len(out_ip)
   mb->l2_len = len(out_udp + vxlan + in_eth)
   mb->l3_len = len(in_ip)
   mb->ol_flags |= PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IP_CKSUM  | \
     PKT_TX_IP_CKSUM |  PKT_TX_TCP_CKSUM;
   set out_ip checksum to 0 in the packet
   set in_ip checksum to 0 in the packet
   set in_tcp checksum to pseudo header using rte_ipv4_phdr_cksum()

   supported on hardware advertising DEV_TX_OFFLOAD_IPV4_CKSUM,
   DEV_TX_OFFLOAD_UDP_CKSUM and DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM

   *Problem 2*: it is not written in the API comments that out_ip
   checksum field should be set to 0 by the application. They should
   be enhanced.

   *Problem 3*: without using the word "fortville", it is difficult
   to understand the goal of the flag PKT_TX_UDP_TUNNEL_PKT. Indeed,
   once PKT_TX_OUTER_IPV4/6 is set, it looks obvious that it's a
   tunnel packet. I suggest to remove the PKT_TX_UDP_TUNNEL_PKT
   flag. In linux, the driver doesn't care about the tunnel type,
   it always set I40E_TXD_CTX_UDP_TUNNELING for all encapsulations [6].

   *Problem 4*: features flags are missing here. A flag
   DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM should be added. This is already
   addressed by one patch from Jijiang [7]


The cases should work in some *future drivers*:

case 7) calculate checksum of out_ip, out_udp, in_ip and in_tcp

   mb->outer_l2_len = len(out_eth)
   mb->outer_l3_len = len(out_ip)
   mb->l2_len = len(out_udp + vxlan + in_eth)
   mb->l3_len = len(in_ip)
   mb->ol_flags |= PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IP_CKSUM | \
     PKT_TX_OUTER_UDP_CKSUM | PKT_TX_IP_CKSUM |  PKT_TX_TCP_CKSUM;
   set out_ip checksum to 0 in the packet
   set out_udp checksum to pseudo header using rte_ipv4_phdr_cksum()
   set in_ip checksum to 0 in the packet
   set in_tcp checksum to pseudo header using rte_ipv4_phdr_cksum()

  We need to add the flag PKT_TX_OUTER_UDP_CKSUM.

case 8) TSO on inner header + out_ip checksum

   This is not supported yet, but latest patch from Jijiang [8]
   implements this feature.

   mb->outer_l2_len = len(out_eth)
   mb->outer_l3_len = len(out_ip)
   mb->l2_len = len(out_udp + vxlan + in_eth)
   mb->l3_len = len(in_ip)
   mb->l4_len = len(in_tcp)
   mb->ol_flags |= PKT_TX_OUTER_IP_CKSUM | PKT_TX_OUTER_IPV4 | \
     PKT_TX_IPV4 | PKT_TX_IP_CKSUM | PKT_TX_TCP_CKSUM | \
     PKT_TX_TCP_SEG;
   set out_ip checksum to 0 in the packet
   set in_ip checksum to 0 in the packet
   set in_tcp checksum to pseudo header without including the IP
     payload length using rte_ipv4_phdr_cksum()

   supported on hardware advertising DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM
   and DEV_TX_OFFLOAD_TCP_TSO.


I think the following cases should be *forbidden by the API*:

case 9) calculate checksum of in_ip and in_tcp  (was case B.1 in [1])

   mb->outer_l2_len = len(out_eth)
   mb->outer_l3_len = len(out_ip)
   mb->l2_len = len(out_udp + vxlan + in_eth)
   mb->l3_len = len(out_ip)
   mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_UDP_TUNNEL_PKT | \
     PKT_TX_IP_CSUM | PKT_TX_UDP_CKSUM;
   set out_ip checksum to 0 in the packet
   set out_udp checksum to pseudo header using rte_ipv4_phdr_cksum()

   If we remove the flag PKT_TX_UDP_TUNNEL_PKT, this cannot be
   supported, but there is no reason to support it as there is
   already one way to do the same.

   I think the driver should not even look at mb->outer_l2_len
   and mb->outer_l3_len if no flag PKT_TX_OUTER_* is set.

case 10) calculate a checksum using only outer_lX fields

   The outer_lX fields or PKT_TX_OUTER_* flags can only be used
   if a inner checksum is enabled. So it's not possible to do
   the following:

   mb->outer_l2_len = len(out_eth)
   mb->outer_l3_len = len(out_ip)
   mb->ol_flags |= PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IP_CSUM
   set out_ip checksum to 0 in the packet

Regards,
Olivier



[1] http://dpdk.org/ml/archives/dev/2014-December/009213.html
[2]
http://dpdk.org/browse/dpdk/tree/lib/librte_mbuf/rte_mbuf.h?id=v1.8.0#n147
[3]
http://dpdk.org/browse/dpdk/tree/lib/librte_mbuf/rte_mbuf.h?id=v1.8.0#n108
[4] http://dpdk.org/ml/archives/dev/2014-December/009352.html
[5]
http://dpdk.org/browse/dpdk/commit/lib/librte_mbuf/rte_mbuf.h?id=711ba9e23e681b97d547219de8af199ea03a33b3
[6]
http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/i40evf/i40e_txrx.c?v=3.17#L1223
[7] http://dpdk.org/dev/patchwork/patch/1907/
[8] http://dpdk.org/dev/patchwork/patch/2329/

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Ananyev, Konstantin]

Hi Olivier,

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Friday, January 16, 2015 5:28 PM
> To: Ananyev, Konstantin; Liu, Jijiang
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine
> 
> Hi Konstantin, Hi Jijiang,
> 
> On 01/15/2015 02:31 PM, Ananyev, Konstantin wrote:
> > To be honest, there are so many mails around that subject, so I am already lost :)
> > Oliver, as I understand you are not happy with the test-pmd commands Frank is proposing.
> > Both syntax and semantics.
> > Is that correct?
> > If so, could you suggest something from your side?
> > That would allow to configure test-pmd to behave in any of 4 possible ways we discussed previously:
> > http://dpdk.org/ml/archives/dev/2014-December/009213.html
> 
> I first wanted to send a mail to describe the current problem with
> testpmd command line and the 2 solutions (Jijiang's and mine).
> But, first, I think we need to fully clarify the checksum offload
> API through examples as it will help to implement testpmd and do
> the documentation. They are based on Jijiang's previous mail [1].
> 
> I will submit a patchset fixing the problems described below in
> the coming days. If we agree on it, I'll submit another one for testpmd.
> 
> Let's use the following packet for all the examples below:
>   out_eth / out_ip / out_udp / vxlan / in_eth / in_ip / in_tcp
> 
> 
> The following cases are supposed to work on niantic and fortville:
> 
> case 1) calculate checksum of out_ip  (was case A in [1])
> 
>    mb->l2_len = len(out_eth)
>    mb->l3_len = len(out_ip)
>    mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_IP_CSUM
>    set out_ip checksum to 0 in the packet
> 
>    supported on hardware advertising DEV_TX_OFFLOAD_IPV4_CKSUM
> 
> case 2) calculate checksum of out_ip and out_udp
> 
>    mb->l2_len = len(out_eth)
>    mb->l3_len = len(out_ip)
>    mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_IP_CSUM | PKT_TX_UDP_CKSUM
>    set out_ip checksum to 0 in the packet
>    set out_udp checksum to pseudo header using rte_ipv4_phdr_cksum()
> 
>    supported on hardware advertising DEV_TX_OFFLOAD_IPV4_CKSUM and
>    DEV_TX_OFFLOAD_UDP_CKSUM
> 
>    *Problem 1*: The comment above PKT_TX_IPV4 says "Packet is IPv4
>    without requiring IP checksum offload" [2], and the help of L4
>    checksum and TSO says that it is required to set the PKT_TX_IPV4
>    flag [3]. This is not coherent.

So what is the problem?
Comments in rte_mbuf.h are not coherent?

> 
>    We are back on the debate about the meaning of PKT_TX_IPV4 vs
>    PKT_TX_IP_CSUM from [4]. This incoherency in comments are introduced
>    by patch [5]. The question is "when an application should set
>    this flag? for any IP packet that does not require IP checksum?".

Yes, if it is an IPv4 packet and application required TX offload for L4 checksum or TSO,
but doesn't want HW offload ofr IPV4 checksum calculation. 

>    This would break many applications.

Which ones?
As I know, so far nothing is broken.

> I think a good definition would
>    be:
> 
>    Packet is IPv4. This flag must be set when using any offload
>    feature (TSO, L3 or L4 checksum) to tell the NIC that the packet
>    is an IPv4 packet.
> 
>    That's why I added PKT_TX_IPV4 in the examples.

I suppose we discussed it several times: both ways are possible.
>From PMD perspective - treating PKT_TX_IPV4 and PKT_TX_IP_CSUM
As mutually exclusive seems a bit more plausible.
>From the upper layer - my understanding, that it is doesn't really matter. 
I thought we had an agreement about it in 1.8, no?

> 
> case 3) calculate checksum of in_ip
> 
>    mb->l2_len = len(out_eth + out_ip + out_udp + vxlan + in_eth)
>    mb->l3_len = len(in_ip)
>    mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_IP_CSUM
>    set in_ip checksum to 0 in the packet
> 
>    This is similar to case 1), but l2_len is different.
> 
>    supported on hardware advertising DEV_TX_OFFLOAD_IPV4_CKSUM
> 
>    Note that it can only work if outer L4 checksum is 0.
> 
> case 4) calculate checksum of in_ip and in_tcp  (was case B.2 in [1])
> 
>    mb->l2_len = len(out_eth + out_ip + out_udp + vxlan + in_eth)
>    mb->l3_len = len(in_ip)
>    mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_IP_CSUM | PKT_TX_TCP_CKSUM
>    set in_ip checksum to 0 in the packet
>    set in_tcp checksum to pseudo header using rte_ipv4_phdr_cksum()
> 
>    This is similar to case 2), but l2_len is different.
> 
>    supported on hardware advertising DEV_TX_OFFLOAD_IPV4_CKSUM and
>    DEV_TX_OFFLOAD_TCP_CKSUM
> 
>    Note that it can only work if outer L4 checksum is 0.
> 
> case 5) segment inner TCP
> 
>    mb->l2_len = len(out_eth + out_ip + out_udp + vxlan + in_eth)
>    mb->l3_len = len(in_ip)
>    mb->l4_len = len(in_tcp)
>    mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_IP_CKSUM | PKT_TX_TCP_CKSUM |
>      PKT_TX_TCP_SEG;
>    set in_ip checksum to 0 in the packet
>    set in_tcp checksum to pseudo header without including the IP
>      payload length using rte_ipv4_phdr_cksum()
> 
>    supported on hardware advertising DEV_TX_OFFLOAD_TCP_TSO.
> 
>    Note that it can only work if outer L4 checksum is 0.
> 
>    Problem 1 is also visible here.
> 
> 
> The following cases are supposed to *work on fortville*:
> 
> case 6) calculate checksum of out_ip, in_ip, in_tcp (was case C in [1])
> 
>    mb->outer_l2_len = len(out_eth)
>    mb->outer_l3_len = len(out_ip)
>    mb->l2_len = len(out_udp + vxlan + in_eth)
>    mb->l3_len = len(in_ip)
>    mb->ol_flags |= PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IP_CKSUM  | \
>      PKT_TX_IP_CKSUM |  PKT_TX_TCP_CKSUM;
>    set out_ip checksum to 0 in the packet
>    set in_ip checksum to 0 in the packet
>    set in_tcp checksum to pseudo header using rte_ipv4_phdr_cksum()
> 
>    supported on hardware advertising DEV_TX_OFFLOAD_IPV4_CKSUM,
>    DEV_TX_OFFLOAD_UDP_CKSUM and DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM
> 
>    *Problem 2*: it is not written in the API comments that out_ip
>    checksum field should be set to 0 by the application. They should
>    be enhanced.

Ok

> 
>    *Problem 3*: without using the word "fortville", it is difficult
>    to understand the goal of the flag PKT_TX_UDP_TUNNEL_PKT. Indeed,
>    once PKT_TX_OUTER_IPV4/6 is set, it looks obvious that it's a
>    tunnel packet. I suggest to remove the PKT_TX_UDP_TUNNEL_PKT
>    flag. In linux, the driver doesn't care about the tunnel type,
>    it always set I40E_TXD_CTX_UDP_TUNNELING for all encapsulations [6].

It might be obvious that it is a tunnel packet from PKT_TX_OUTER_* is set,
but it is not obvious what type of tunnelling it would be.
FVL HW supports HW TX offloads for different type of tunnelling and
requires that SW provide information about tunnelling type.
>From i40e datasheet:
L4TUNT L4 Tunneling Type (Teredo / GRE header / VXLAN header) indication:
00b - No UDP / GRE tunneling (field must be set to zero if EIPT equals to zero)
01b - UDP tunneling header (any UDP tunneling, VXLAN and Geneve).
10b - GRE tunneling header
As we do plan to support other than UDP tunnelling types, I suppose we'll need to keep  
PKT_TX_UDP_TUNNEL_PKT flag.

> 
>    *Problem 4*: features flags are missing here. A flag
>    DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM should be added. This is already
>    addressed by one patch from Jijiang [7]

Ok, yes I think Frank already submit a patch for that.

> 
> 
> The cases should work in some *future drivers*:
> 
> case 7) calculate checksum of out_ip, out_udp, in_ip and in_tcp
> 
>    mb->outer_l2_len = len(out_eth)
>    mb->outer_l3_len = len(out_ip)
>    mb->l2_len = len(out_udp + vxlan + in_eth)
>    mb->l3_len = len(in_ip)
>    mb->ol_flags |= PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IP_CKSUM | \
>      PKT_TX_OUTER_UDP_CKSUM | PKT_TX_IP_CKSUM |  PKT_TX_TCP_CKSUM;
>    set out_ip checksum to 0 in the packet
>    set out_udp checksum to pseudo header using rte_ipv4_phdr_cksum()
>    set in_ip checksum to 0 in the packet
>    set in_tcp checksum to pseudo header using rte_ipv4_phdr_cksum()
> 
>   We need to add the flag PKT_TX_OUTER_UDP_CKSUM.

We can, though right now we don't have a HW that is able to do that.
Why need to do it now?

> 
> case 8) TSO on inner header + out_ip checksum
> 
>    This is not supported yet, but latest patch from Jijiang [8]
>    implements this feature.
> 
>    mb->outer_l2_len = len(out_eth)
>    mb->outer_l3_len = len(out_ip)
>    mb->l2_len = len(out_udp + vxlan + in_eth)
>    mb->l3_len = len(in_ip)
>    mb->l4_len = len(in_tcp)
>    mb->ol_flags |= PKT_TX_OUTER_IP_CKSUM | PKT_TX_OUTER_IPV4 | \
>      PKT_TX_IPV4 | PKT_TX_IP_CKSUM | PKT_TX_TCP_CKSUM | \
>      PKT_TX_TCP_SEG;
>    set out_ip checksum to 0 in the packet
>    set in_ip checksum to 0 in the packet
>    set in_tcp checksum to pseudo header without including the IP
>      payload length using rte_ipv4_phdr_cksum()
> 
>    supported on hardware advertising DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM
>    and DEV_TX_OFFLOAD_TCP_TSO.
> 
> 
> I think the following cases should be *forbidden by the API*:
> 
> case 9) calculate checksum of in_ip and in_tcp  (was case B.1 in [1])
> 
>    mb->outer_l2_len = len(out_eth)
>    mb->outer_l3_len = len(out_ip)
>    mb->l2_len = len(out_udp + vxlan + in_eth)
>    mb->l3_len = len(out_ip)
>    mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_UDP_TUNNEL_PKT | \
>      PKT_TX_IP_CSUM | PKT_TX_UDP_CKSUM;
>    set out_ip checksum to 0 in the packet
>    set out_udp checksum to pseudo header using rte_ipv4_phdr_cksum()
> 
>    If we remove the flag PKT_TX_UDP_TUNNEL_PKT, this cannot be
>    supported, but there is no reason to support it as there is
>    already one way to do the same.
> 
>    I think the driver should not even look at mb->outer_l2_len
>    and mb->outer_l3_len if no flag PKT_TX_OUTER_* is set.

Why it should be forbidden?
I admit it might be a bit slower than case 4),
but I think absolutely legal way to setup HW offloads for inner L3/L4.
As I said we need a PKT_TX_UDP_TUNNEL_PKT anyway, so I suppose
PKT_TX_*_TUNNEL_PKT should be an indication is it a tunnel packet or not.
PKT_TX_OUTER_* flags indicate does outer cksum offload is required or not. 

> 
> case 10) calculate a checksum using only outer_lX fields
> 
>    The outer_lX fields or PKT_TX_OUTER_* flags can only be used
>    if a inner checksum is enabled. So it's not possible to do
>    the following:
> 
>    mb->outer_l2_len = len(out_eth)
>    mb->outer_l3_len = len(out_ip)
>    mb->ol_flags |= PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IP_CSUM
>    set out_ip checksum to 0 in the packet

Ok, I think no one plans to use it anyway.

Konstantin

> 
> Regards,
> Olivier
> 
> 
> 
> [1] http://dpdk.org/ml/archives/dev/2014-December/009213.html
> [2]
> http://dpdk.org/browse/dpdk/tree/lib/librte_mbuf/rte_mbuf.h?id=v1.8.0#n147
> [3]
> http://dpdk.org/browse/dpdk/tree/lib/librte_mbuf/rte_mbuf.h?id=v1.8.0#n108
> [4] http://dpdk.org/ml/archives/dev/2014-December/009352.html
> [5]
> http://dpdk.org/browse/dpdk/commit/lib/librte_mbuf/rte_mbuf.h?id=711ba9e23e681b97d547219de8af199ea03a33b3
> [6]
> http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/i40evf/i40e_txrx.c?v=3.17#L1223
> [7] http://dpdk.org/dev/patchwork/patch/1907/
> [8] http://dpdk.org/dev/patchwork/patch/2329/

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Olivier MATZ]

Hi Konstantin,

On 01/19/2015 02:04 PM, Ananyev, Konstantin wrote:
>> case 2) calculate checksum of out_ip and out_udp
>>
>>    mb->l2_len = len(out_eth)
>>    mb->l3_len = len(out_ip)
>>    mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_IP_CSUM | PKT_TX_UDP_CKSUM
>>    set out_ip checksum to 0 in the packet
>>    set out_udp checksum to pseudo header using rte_ipv4_phdr_cksum()
>>
>>    supported on hardware advertising DEV_TX_OFFLOAD_IPV4_CKSUM and
>>    DEV_TX_OFFLOAD_UDP_CKSUM
>>
>>    *Problem 1*: The comment above PKT_TX_IPV4 says "Packet is IPv4
>>    without requiring IP checksum offload" [2], and the help of L4
>>    checksum and TSO says that it is required to set the PKT_TX_IPV4
>>    flag [3]. This is not coherent.
> 
> So what is the problem?
> Comments in rte_mbuf.h are not coherent?

No there're not coherent

>>    We are back on the debate about the meaning of PKT_TX_IPV4 vs
>>    PKT_TX_IP_CSUM from [4]. This incoherency in comments are introduced
>>    by patch [5]. The question is "when an application should set
>>    this flag? for any IP packet that does not require IP checksum?".
> 
> Yes, if it is an IPv4 packet and application required TX offload for L4 checksum or TSO,
> but doesn't want HW offload ofr IPV4 checksum calculation. 
> 
>>    This would break many applications.
> 
> Which ones?
> As I know, so far nothing is broken.

The problem today is that it's not obvious for a developper to
know when an application should set the PKT_TX_IPV4 flag. From the
comments, we could think that an application has to set it for any
transmitted IP packet, even for packets that do not require tx
offload. Asking to do this in the API would break many applications.

The comment should at least say that this flag is *only* required
when asking for L4 checksum. As TSO implies IP checksum, it means the
PKT_TX_IPV4 should not be set, but PKT_TX_IP_CSUM instead.

>> I think a good definition would
>>    be:
>>
>>    Packet is IPv4. This flag must be set when using any offload
>>    feature (TSO, L3 or L4 checksum) to tell the NIC that the packet
>>    is an IPv4 packet.
>>
>>    That's why I added PKT_TX_IPV4 in the examples.
> 
> I suppose we discussed it several times: both ways are possible.
> From PMD perspective - treating PKT_TX_IPV4 and PKT_TX_IP_CSUM
> As mutually exclusive seems a bit more plausible.
> From the upper layer - my understanding, that it is doesn't really matter. 
> I thought we had an agreement about it in 1.8, no?

Indeed, this was already discussed, but there was a lot of pressure
for 1.8.0 to push something, even not perfect. The fog around comments
shows that the API was not very clearly defined for 1.8.0. If you read
the comments of the API, it is impossible to understand when the
PKT_TX_IPV4 or PKT_TX_IP_CSUM flags must be set. I would even say
more: the only place where the comments bring a valuable information
(L4 checksum and TSO) describe the case where PKT_TX_IPV4 and
PKT_TX_IP_CSUM are not exclusive...

So I will fix that in my coming patch series. Just for information,
I'm pretty sure that having PKT_TX_IPV4 and PKT_TX_IP_CSUM as not
exclusive flag would not require any change anywhere in the PMDs (even
in i40e). On the contrary, making them exclusive would require to
change the ixgbe TSO code because we check PKT_TX_IPV4.

>>    *Problem 3*: without using the word "fortville", it is difficult
>>    to understand the goal of the flag PKT_TX_UDP_TUNNEL_PKT. Indeed,
>>    once PKT_TX_OUTER_IPV4/6 is set, it looks obvious that it's a
>>    tunnel packet. I suggest to remove the PKT_TX_UDP_TUNNEL_PKT
>>    flag. In linux, the driver doesn't care about the tunnel type,
>>    it always set I40E_TXD_CTX_UDP_TUNNELING for all encapsulations [6].
> 
> It might be obvious that it is a tunnel packet from PKT_TX_OUTER_* is set,
> but it is not obvious what type of tunnelling it would be.
> FVL HW supports HW TX offloads for different type of tunnelling and
> requires that SW provide information about tunnelling type.
> From i40e datasheet:
> L4TUNT L4 Tunneling Type (Teredo / GRE header / VXLAN header) indication:
> 00b - No UDP / GRE tunneling (field must be set to zero if EIPT equals to zero)
> 01b - UDP tunneling header (any UDP tunneling, VXLAN and Geneve).
> 10b - GRE tunneling header
> As we do plan to support other than UDP tunnelling types, I suppose we'll need to keep  
> PKT_TX_UDP_TUNNEL_PKT flag.

As I've said: in linux, the driver doesn't care about the tunnel type,
it always set I40E_TXD_CTX_UDP_TUNNELING for all encapsulations.
However I suppose that linux driver is able to process the hw outer
checksum even for other encapsulations (gre, ipip). And, does it mean
that ipip tunnels are not supported by i40e? I can't believe it.
If it's the case... how an application on top of DPDK can know which
tunnel types are supported by the underlying port?

>From what I've read, what the datasheet does not explain is:
"what is done differently for this packet between setting the register
to GRE (10b) or UDP (01b)?"

>> case 7) calculate checksum of out_ip, out_udp, in_ip and in_tcp
>>
>>    mb->outer_l2_len = len(out_eth)
>>    mb->outer_l3_len = len(out_ip)
>>    mb->l2_len = len(out_udp + vxlan + in_eth)
>>    mb->l3_len = len(in_ip)
>>    mb->ol_flags |= PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IP_CKSUM | \
>>      PKT_TX_OUTER_UDP_CKSUM | PKT_TX_IP_CKSUM |  PKT_TX_TCP_CKSUM;
>>    set out_ip checksum to 0 in the packet
>>    set out_udp checksum to pseudo header using rte_ipv4_phdr_cksum()
>>    set in_ip checksum to 0 in the packet
>>    set in_tcp checksum to pseudo header using rte_ipv4_phdr_cksum()
>>
>>   We need to add the flag PKT_TX_OUTER_UDP_CKSUM.
> 
> We can, though right now we don't have a HW that is able to do that.
> Why need to do it now?

No, I agree we should not add it now. I just want to be sure we have
a consensus that it will work like this the day we'll have such drivers.

>> I think the following cases should be *forbidden by the API*:
>>
>> case 9) calculate checksum of in_ip and in_tcp  (was case B.1 in [1])
>>
>>    mb->outer_l2_len = len(out_eth)
>>    mb->outer_l3_len = len(out_ip)
>>    mb->l2_len = len(out_udp + vxlan + in_eth)
>>    mb->l3_len = len(out_ip)
>>    mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_UDP_TUNNEL_PKT | \
>>      PKT_TX_IP_CSUM | PKT_TX_UDP_CKSUM;
>>    set out_ip checksum to 0 in the packet
>>    set out_udp checksum to pseudo header using rte_ipv4_phdr_cksum()
>>
>>    If we remove the flag PKT_TX_UDP_TUNNEL_PKT, this cannot be
>>    supported, but there is no reason to support it as there is
>>    already one way to do the same.
>>
>>    I think the driver should not even look at mb->outer_l2_len
>>    and mb->outer_l3_len if no flag PKT_TX_OUTER_* is set.
> 
> Why it should be forbidden?
> I admit it might be a bit slower than case 4),
> but I think absolutely legal way to setup HW offloads for inner L3/L4.
> As I said we need a PKT_TX_UDP_TUNNEL_PKT anyway, so I suppose
> PKT_TX_*_TUNNEL_PKT should be an indication is it a tunnel packet or not.
> PKT_TX_OUTER_* flags indicate does outer cksum offload is required or not. 

I don't understand. The result in terms of hardware is exactly the
same than case 4). Why should we have 2 different ways for doing the
same thing? This is really confusing for an API. Moreover, you said
it: it is slower that case 4).

It also seems easier to understand from an API point of view: the
PMD uses mb->outer_lX_len if and only if a PKT_TX_OUTER_* flag
is present.

>> case 10) calculate a checksum using only outer_lX fields
>>
>>    The outer_lX fields or PKT_TX_OUTER_* flags can only be used
>>    if a inner checksum is enabled. So it's not possible to do
>>    the following:
>>
>>    mb->outer_l2_len = len(out_eth)
>>    mb->outer_l3_len = len(out_ip)
>>    mb->ol_flags |= PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IP_CSUM
>>    set out_ip checksum to 0 in the packet
> 
> Ok, I think no one plans to use it anyway.
> 
> Konstantin

Thanks Konstantin for taking the time to reply and progress on this.

Regards,
Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Ananyev, Konstantin]

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Monday, January 19, 2015 2:39 PM
> To: Ananyev, Konstantin; Liu, Jijiang
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine
> 
> Hi Konstantin,
> 
> On 01/19/2015 02:04 PM, Ananyev, Konstantin wrote:
> >> case 2) calculate checksum of out_ip and out_udp
> >>
> >>    mb->l2_len = len(out_eth)
> >>    mb->l3_len = len(out_ip)
> >>    mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_IP_CSUM | PKT_TX_UDP_CKSUM
> >>    set out_ip checksum to 0 in the packet
> >>    set out_udp checksum to pseudo header using rte_ipv4_phdr_cksum()
> >>
> >>    supported on hardware advertising DEV_TX_OFFLOAD_IPV4_CKSUM and
> >>    DEV_TX_OFFLOAD_UDP_CKSUM
> >>
> >>    *Problem 1*: The comment above PKT_TX_IPV4 says "Packet is IPv4
> >>    without requiring IP checksum offload" [2], and the help of L4
> >>    checksum and TSO says that it is required to set the PKT_TX_IPV4
> >>    flag [3]. This is not coherent.
> >
> > So what is the problem?
> > Comments in rte_mbuf.h are not coherent?
> 
> No there're not coherent

Ok, if the problem is just comments - let's fix it.

> 
> >>    We are back on the debate about the meaning of PKT_TX_IPV4 vs
> >>    PKT_TX_IP_CSUM from [4]. This incoherency in comments are introduced
> >>    by patch [5]. The question is "when an application should set
> >>    this flag? for any IP packet that does not require IP checksum?".
> >
> > Yes, if it is an IPv4 packet and application required TX offload for L4 checksum or TSO,
> > but doesn't want HW offload ofr IPV4 checksum calculation.
> >
> >>    This would break many applications.
> >
> > Which ones?
> > As I know, so far nothing is broken.
> 
> The problem today is that it's not obvious for a developper to
> know when an application should set the PKT_TX_IPV4 flag. From the
> comments, we could think that an application has to set it for any
> transmitted IP packet, even for packets that do not require tx
> offload. Asking to do this in the API would break many applications.
> 
> The comment should at least say that this flag is *only* required
> when asking for L4 checksum. As TSO implies IP checksum, it means the
> PKT_TX_IPV4 should not be set, but PKT_TX_IP_CSUM instead.

Ok, so the problem is in comments again?
If so, sure let's update them to make things clear.

> 
> >> I think a good definition would
> >>    be:
> >>
> >>    Packet is IPv4. This flag must be set when using any offload
> >>    feature (TSO, L3 or L4 checksum) to tell the NIC that the packet
> >>    is an IPv4 packet.
> >>
> >>    That's why I added PKT_TX_IPV4 in the examples.
> >
> > I suppose we discussed it several times: both ways are possible.
> > From PMD perspective - treating PKT_TX_IPV4 and PKT_TX_IP_CSUM
> > As mutually exclusive seems a bit more plausible.
> > From the upper layer - my understanding, that it is doesn't really matter.
> > I thought we had an agreement about it in 1.8, no?
> 
> Indeed, this was already discussed, but there was a lot of pressure
> for 1.8.0 to push something, even not perfect. The fog around comments
> shows that the API was not very clearly defined for 1.8.0. If you read
> the comments of the API, it is impossible to understand when the
> PKT_TX_IPV4 or PKT_TX_IP_CSUM flags must be set. I would even say
> more: the only place where the comments bring a valuable information
> (L4 checksum and TSO) describe the case where PKT_TX_IPV4 and
> PKT_TX_IP_CSUM are not exclusive...
> 
> So I will fix that in my coming patch series. Just for information,
> I'm pretty sure that having PKT_TX_IPV4 and PKT_TX_IP_CSUM as not
> exclusive flag would not require any change anywhere in the PMDs (even
> in i40e).

Right now - no.
Though as I said from PMD perspective having them exclusive is a bit preferable.
Again, I don't see any big difference from upper layer code.

> On the contrary, making them exclusive would require to
> change the ixgbe TSO code because we check PKT_TX_IPV4.

Hmm, so you are saying there is a bug somewhere  in ixbe_rxtx.c?
What particular place you are talking about?

> 
> >>    *Problem 3*: without using the word "fortville", it is difficult
> >>    to understand the goal of the flag PKT_TX_UDP_TUNNEL_PKT. Indeed,
> >>    once PKT_TX_OUTER_IPV4/6 is set, it looks obvious that it's a
> >>    tunnel packet. I suggest to remove the PKT_TX_UDP_TUNNEL_PKT
> >>    flag. In linux, the driver doesn't care about the tunnel type,
> >>    it always set I40E_TXD_CTX_UDP_TUNNELING for all encapsulations [6].
> >
> > It might be obvious that it is a tunnel packet from PKT_TX_OUTER_* is set,
> > but it is not obvious what type of tunnelling it would be.
> > FVL HW supports HW TX offloads for different type of tunnelling and
> > requires that SW provide information about tunnelling type.
> > From i40e datasheet:
> > L4TUNT L4 Tunneling Type (Teredo / GRE header / VXLAN header) indication:
> > 00b - No UDP / GRE tunneling (field must be set to zero if EIPT equals to zero)
> > 01b - UDP tunneling header (any UDP tunneling, VXLAN and Geneve).
> > 10b - GRE tunneling header
> > As we do plan to support other than UDP tunnelling types, I suppose we'll need to keep
> > PKT_TX_UDP_TUNNEL_PKT flag.
> 
> As I've said: in linux, the driver doesn't care about the tunnel type,
> it always set I40E_TXD_CTX_UDP_TUNNELING for all encapsulations.

Ok, and why it should be our problem?
We have a lot of things done in a different manner then linux/freebsd kernel drivers,
Why now it became a problem?

> However I suppose that linux driver is able to process the hw outer
> checksum even for other encapsulations (gre, ipip). 

No idea, never tried.

>And, does it mean
> that ipip tunnels are not supported by i40e? I can't believe it.

IPIP tunnels are supported.
>From the FVL spec, for IPIP L4TUNT and L4TUN should be set to 0.
You can read all that yourself:
http://www.intel.com/content/www/us/en/embedded/products/networking/xl710-10-40-controller-datasheet.html
Sections 8.4.2.2.1 and 8.4.4
No need to do a reverse engineering from linux i40e source code. 

> If it's the case... how an application on top of DPDK can know which
> tunnel types are supported by the underlying port?
> From what I've read, what the datasheet does not explain is:
> "what is done differently for this packet between setting the register
> to GRE (10b) or UDP (01b)?"

That's a good question. 
I don't know why HW requires that information, just follow the spec here.

> 
> >> case 7) calculate checksum of out_ip, out_udp, in_ip and in_tcp
> >>
> >>    mb->outer_l2_len = len(out_eth)
> >>    mb->outer_l3_len = len(out_ip)
> >>    mb->l2_len = len(out_udp + vxlan + in_eth)
> >>    mb->l3_len = len(in_ip)
> >>    mb->ol_flags |= PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IP_CKSUM | \
> >>      PKT_TX_OUTER_UDP_CKSUM | PKT_TX_IP_CKSUM |  PKT_TX_TCP_CKSUM;
> >>    set out_ip checksum to 0 in the packet
> >>    set out_udp checksum to pseudo header using rte_ipv4_phdr_cksum()
> >>    set in_ip checksum to 0 in the packet
> >>    set in_tcp checksum to pseudo header using rte_ipv4_phdr_cksum()
> >>
> >>   We need to add the flag PKT_TX_OUTER_UDP_CKSUM.
> >
> > We can, though right now we don't have a HW that is able to do that.
> > Why need to do it now?
> 
> No, I agree we should not add it now. I just want to be sure we have
> a consensus that it will work like this the day we'll have such drivers.

Ok.

> 
> >> I think the following cases should be *forbidden by the API*:
> >>
> >> case 9) calculate checksum of in_ip and in_tcp  (was case B.1 in [1])
> >>
> >>    mb->outer_l2_len = len(out_eth)
> >>    mb->outer_l3_len = len(out_ip)
> >>    mb->l2_len = len(out_udp + vxlan + in_eth)
> >>    mb->l3_len = len(out_ip)
> >>    mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_UDP_TUNNEL_PKT | \
> >>      PKT_TX_IP_CSUM | PKT_TX_UDP_CKSUM;
> >>    set out_ip checksum to 0 in the packet
> >>    set out_udp checksum to pseudo header using rte_ipv4_phdr_cksum()
> >>
> >>    If we remove the flag PKT_TX_UDP_TUNNEL_PKT, this cannot be
> >>    supported, but there is no reason to support it as there is
> >>    already one way to do the same.
> >>
> >>    I think the driver should not even look at mb->outer_l2_len
> >>    and mb->outer_l3_len if no flag PKT_TX_OUTER_* is set.
> >
> > Why it should be forbidden?
> > I admit it might be a bit slower than case 4),
> > but I think absolutely legal way to setup HW offloads for inner L3/L4.
> > As I said we need a PKT_TX_UDP_TUNNEL_PKT anyway, so I suppose
> > PKT_TX_*_TUNNEL_PKT should be an indication is it a tunnel packet or not.
> > PKT_TX_OUTER_* flags indicate does outer cksum offload is required or not.
> 
> I don't understand. The result in terms of hardware is exactly the
> same than case 4). Why should we have 2 different ways for doing the
> same thing?

If HW supports that capability, why should we forbid it? 
Let user to choose himself what way to use.
FVL spec lists it as a valid approach.   
As one of possible use-cases:  HW VLAN tags insertion for both inner and outer packets.
FVL can do that, though as I know our PMD doesn't implement it yet.
For that, we'll need to specify at least:
outer_l2_len, outer_l3_len, l2_len.
While PKT_TX_OUTER_* might stay cleared.

> This is really confusing for an API. Moreover, you said
> it: it is slower that case 4).

I don't know would be slower then 4) or not for sure.
That's my guess, based on the fact that for 9) we need to fill 2 descriptors, while fro 4) - only 1.
Though I didn't measure the difference.
That's actually one more reason why to allow and support it -
so people can make sure that on FVL both ways work as expected and measure the difference.

Konstantin

> 
> It also seems easier to understand from an API point of view: the
> PMD uses mb->outer_lX_len if and only if a PKT_TX_OUTER_* flag
> is present.
> 
> >> case 10) calculate a checksum using only outer_lX fields
> >>
> >>    The outer_lX fields or PKT_TX_OUTER_* flags can only be used
> >>    if a inner checksum is enabled. So it's not possible to do
> >>    the following:
> >>
> >>    mb->outer_l2_len = len(out_eth)
> >>    mb->outer_l3_len = len(out_ip)
> >>    mb->ol_flags |= PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IP_CSUM
> >>    set out_ip checksum to 0 in the packet
> >
> > Ok, I think no one plans to use it anyway.
> >
> > Konstantin
> 
> Thanks Konstantin for taking the time to reply and progress on this.
> 
> Regards,
> Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Olivier MATZ]

Hi,

On 01/20/2015 02:12 AM, Ananyev, Konstantin wrote:
>>>> I think a good definition would
>>>>     be:
>>>>
>>>>     Packet is IPv4. This flag must be set when using any offload
>>>>     feature (TSO, L3 or L4 checksum) to tell the NIC that the packet
>>>>     is an IPv4 packet.
>>>>
>>>>     That's why I added PKT_TX_IPV4 in the examples.
>>>
>>> I suppose we discussed it several times: both ways are possible.
>>>  From PMD perspective - treating PKT_TX_IPV4 and PKT_TX_IP_CSUM
>>> As mutually exclusive seems a bit more plausible.
>>>  From the upper layer - my understanding, that it is doesn't really matter.
>>> I thought we had an agreement about it in 1.8, no?
>>
>> Indeed, this was already discussed, but there was a lot of pressure
>> for 1.8.0 to push something, even not perfect. The fog around comments
>> shows that the API was not very clearly defined for 1.8.0. If you read
>> the comments of the API, it is impossible to understand when the
>> PKT_TX_IPV4 or PKT_TX_IP_CSUM flags must be set. I would even say
>> more: the only place where the comments bring a valuable information
>> (L4 checksum and TSO) describe the case where PKT_TX_IPV4 and
>> PKT_TX_IP_CSUM are not exclusive...
>>
>> So I will fix that in my coming patch series. Just for information,
>> I'm pretty sure that having PKT_TX_IPV4 and PKT_TX_IP_CSUM as not
>> exclusive flag would not require any change anywhere in the PMDs (even
>> in i40e).
>
> Right now - no.
> Though as I said from PMD perspective having them exclusive is a bit preferable.
> Again, I don't see any big difference from upper layer code.

Sure, it does not make a big difference in terms of code. But
in terms of API, the naming of the flag is coherent to what it is
used for. And it's easier to find a simple definition, like:

  * Packet is IPv4. This flag must be set when using any offload feature
  * (TSO, L3 or L4 checksum) to tell the NIC that the packet is an IPv4
  * packet.

>> On the contrary, making them exclusive would require to
>> change the ixgbe TSO code because we check.
>
> Hmm, so you are saying there is a bug somewhere  in ixbe_rxtx.c?
> What particular place you are talking about?

Sorry, I spoke too fast. In TSO code, we check PKT_TX_IP_CKSUM (and not
PKT_TX_IPV4 as I thought), so it would work for both methods without
patching the code.

In this case, it means that both approach would not require to
modify the code.

>>>>     *Problem 3*: without using the word "fortville", it is difficult
>>>>     to understand the goal of the flag PKT_TX_UDP_TUNNEL_PKT. Indeed,
>>>>     once PKT_TX_OUTER_IPV4/6 is set, it looks obvious that it's a
>>>>     tunnel packet. I suggest to remove the PKT_TX_UDP_TUNNEL_PKT
>>>>     flag. In linux, the driver doesn't care about the tunnel type,
>>>>     it always set I40E_TXD_CTX_UDP_TUNNELING for all encapsulations [6].
>>>
>>> It might be obvious that it is a tunnel packet from PKT_TX_OUTER_* is set,
>>> but it is not obvious what type of tunnelling it would be.
>>> FVL HW supports HW TX offloads for different type of tunnelling and
>>> requires that SW provide information about tunnelling type.
>>>  From i40e datasheet:
>>> L4TUNT L4 Tunneling Type (Teredo / GRE header / VXLAN header) indication:
>>> 00b - No UDP / GRE tunneling (field must be set to zero if EIPT equals to zero)
>>> 01b - UDP tunneling header (any UDP tunneling, VXLAN and Geneve).
>>> 10b - GRE tunneling header
>>> As we do plan to support other than UDP tunnelling types, I suppose we'll need to keep
>>> PKT_TX_UDP_TUNNEL_PKT flag.
>>
>> As I've said: in linux, the driver doesn't care about the tunnel type,
>> it always set I40E_TXD_CTX_UDP_TUNNELING for all encapsulations.
>
> Ok, and why it should be our problem?
> We have a lot of things done in a different manner then linux/freebsd kernel drivers,
> Why now it became a problem?

If linux doesn't need an equivalent flag for doing the same thing,
it probably means we don't need it either.

In a performance-oriented software like dpdk, having a flag that we
don't know what the hardware does with, that is not needed in other
drivers of the same harware, that makes the API harder to understand
could be a problem.

Another argument: if we can remove this flag, it would make the
testpmd commands reworkd proposed by Jijiang much more easy to
understand: only a new "csum parse-tunnel on|off" would be required,
and it can be explained in a few words.

I'll try to do some tests on a fortville NIC if I can find one. I'm
curious to see if we can transmit any encapsulation packet (ip in ip,
ip in gre, eth in gre, eth in vxlan, or even a proprietary tunnel).

We should avoid the need to specify the tunnel type in the OUTER
checksum API if we can, else it would limit us to specific
supported protocols.

>>>> I think the following cases should be *forbidden by the API*:
>>>>
>>>> case 9) calculate checksum of in_ip and in_tcp  (was case B.1 in [1])
>>>>
>>>>     mb->outer_l2_len = len(out_eth)
>>>>     mb->outer_l3_len = len(out_ip)
>>>>     mb->l2_len = len(out_udp + vxlan + in_eth)
>>>>     mb->l3_len = len(out_ip)
>>>>     mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_UDP_TUNNEL_PKT | \
>>>>       PKT_TX_IP_CSUM | PKT_TX_UDP_CKSUM;
>>>>     set out_ip checksum to 0 in the packet
>>>>     set out_udp checksum to pseudo header using rte_ipv4_phdr_cksum()
>>>>
>>>>     If we remove the flag PKT_TX_UDP_TUNNEL_PKT, this cannot be
>>>>     supported, but there is no reason to support it as there is
>>>>     already one way to do the same.
>>>>
>>>>     I think the driver should not even look at mb->outer_l2_len
>>>>     and mb->outer_l3_len if no flag PKT_TX_OUTER_* is set.
>>>
>>> Why it should be forbidden?
>>> I admit it might be a bit slower than case 4),
>>> but I think absolutely legal way to setup HW offloads for inner L3/L4.
>>> As I said we need a PKT_TX_UDP_TUNNEL_PKT anyway, so I suppose
>>> PKT_TX_*_TUNNEL_PKT should be an indication is it a tunnel packet or not.
>>> PKT_TX_OUTER_* flags indicate does outer cksum offload is required or not.
>>
>> I don't understand. The result in terms of hardware is exactly the
>> same than case 4). Why should we have 2 different ways for doing the
>> same thing?
>
> If HW supports that capability, why should we forbid it?
> Let user to choose himself what way to use.
> FVL spec lists it as a valid approach.

It is not a hardware feature. Case 4) and case 9) would fill the
hardware registers exactly the same. To me, it's just an API question.

> As one of possible use-cases:  HW VLAN tags insertion for both inner and outer packets.
> FVL can do that, though as I know our PMD doesn't implement it yet.
> For that, we'll need to specify at least:
> outer_l2_len, outer_l3_len, l2_len.
> While PKT_TX_OUTER_* might stay cleared.

If a VLAN flag has to be inserted in outer header, a new flag
PKT_TX_OUTER_INSERT_VLAN would be added. So my specification
would still be correct:

   The driver should look at mb->outer_lX_len only if a
   PKT_TX_OUTER_* flag is present.

>> This is really confusing for an API. Moreover, you said
>> it: it is slower that case 4).
>
> I don't know would be slower then 4) or not for sure.
> That's my guess, based on the fact that for 9) we need to fill 2 descriptors, while fro 4) - only 1.
> Though I didn't measure the difference.
> That's actually one more reason why to allow and support it -
> so people can make sure that on FVL both ways work as expected and measure the difference.
>
> Konstantin


Regards,
Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Thomas Monjalon]

2015-01-20 13:39, Olivier MATZ:
> On 01/20/2015 02:12 AM, Ananyev, Konstantin wrote:
> >> So I will fix that in my coming patch series. Just for information,
> >> I'm pretty sure that having PKT_TX_IPV4 and PKT_TX_IP_CSUM as not
> >> exclusive flag would not require any change anywhere in the PMDs (even
> >> in i40e).
> >
> > Right now - no.
> > Though as I said from PMD perspective having them exclusive is a bit preferable.
> > Again, I don't see any big difference from upper layer code.
> 
> Sure, it does not make a big difference in terms of code. But
> in terms of API, the naming of the flag is coherent to what it is
> used for. And it's easier to find a simple definition, like:
> 
>   * Packet is IPv4. This flag must be set when using any offload feature
>   * (TSO, L3 or L4 checksum) to tell the NIC that the packet is an IPv4
>   * packet.

+1
It's clearer to set PKT_TX_IPV4 in all offload cases of IPv4 packets,
and add PKT_TX_IP_CSUM when checksum offload is required.

Simply simpler ;)

-- 
Thomas

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Stephen Hemminger]

On Tue, 20 Jan 2015 16:18:01 +0100
Thomas Monjalon <thomas.monjalon@6wind.com> wrote:

> 2015-01-20 13:39, Olivier MATZ:
> > On 01/20/2015 02:12 AM, Ananyev, Konstantin wrote:
> > >> So I will fix that in my coming patch series. Just for information,
> > >> I'm pretty sure that having PKT_TX_IPV4 and PKT_TX_IP_CSUM as not
> > >> exclusive flag would not require any change anywhere in the PMDs (even
> > >> in i40e).
> > >
> > > Right now - no.
> > > Though as I said from PMD perspective having them exclusive is a bit preferable.
> > > Again, I don't see any big difference from upper layer code.
> > 
> > Sure, it does not make a big difference in terms of code. But
> > in terms of API, the naming of the flag is coherent to what it is
> > used for. And it's easier to find a simple definition, like:
> > 
> >   * Packet is IPv4. This flag must be set when using any offload feature
> >   * (TSO, L3 or L4 checksum) to tell the NIC that the packet is an IPv4
> >   * packet.
> 
> +1
> It's clearer to set PKT_TX_IPV4 in all offload cases of IPv4 packets,
> and add PKT_TX_IP_CSUM when checksum offload is required.
> 
> Simply simpler ;)
> 

Sure. Although in my experience IP checksum is just as cheap done in
software since the header fits in cache.

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Ananyev, Konstantin]

Hi Olivier,

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, January 20, 2015 12:39 PM
> To: Ananyev, Konstantin; Liu, Jijiang
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine
> 
> Hi,
> 
> On 01/20/2015 02:12 AM, Ananyev, Konstantin wrote:
> >>>> I think a good definition would
> >>>>     be:
> >>>>
> >>>>     Packet is IPv4. This flag must be set when using any offload
> >>>>     feature (TSO, L3 or L4 checksum) to tell the NIC that the packet
> >>>>     is an IPv4 packet.
> >>>>
> >>>>     That's why I added PKT_TX_IPV4 in the examples.
> >>>
> >>> I suppose we discussed it several times: both ways are possible.
> >>>  From PMD perspective - treating PKT_TX_IPV4 and PKT_TX_IP_CSUM
> >>> As mutually exclusive seems a bit more plausible.
> >>>  From the upper layer - my understanding, that it is doesn't really matter.
> >>> I thought we had an agreement about it in 1.8, no?
> >>
> >> Indeed, this was already discussed, but there was a lot of pressure
> >> for 1.8.0 to push something, even not perfect. The fog around comments
> >> shows that the API was not very clearly defined for 1.8.0. If you read
> >> the comments of the API, it is impossible to understand when the
> >> PKT_TX_IPV4 or PKT_TX_IP_CSUM flags must be set. I would even say
> >> more: the only place where the comments bring a valuable information
> >> (L4 checksum and TSO) describe the case where PKT_TX_IPV4 and
> >> PKT_TX_IP_CSUM are not exclusive...
> >>
> >> So I will fix that in my coming patch series. Just for information,
> >> I'm pretty sure that having PKT_TX_IPV4 and PKT_TX_IP_CSUM as not
> >> exclusive flag would not require any change anywhere in the PMDs (even
> >> in i40e).
> >
> > Right now - no.
> > Though as I said from PMD perspective having them exclusive is a bit preferable.
> > Again, I don't see any big difference from upper layer code.
> 
> Sure, it does not make a big difference in terms of code. But
> in terms of API, the naming of the flag is coherent to what it is
> used for. And it's easier to find a simple definition, like:
> 
>   * Packet is IPv4. This flag must be set when using any offload feature
>   * (TSO, L3 or L4 checksum) to tell the NIC that the packet is an IPv4
>   * packet.

Ok, and what's wrong with:
"Packet is IPv4. This flag must be set when using any offload feature
(TSO, L3 or L4 checksum) to tell the NIC that the packet is an IPv4
packet and no HW offload for IPv4 header checksum calculation is required"
?

> 
> >> On the contrary, making them exclusive would require to
> >> change the ixgbe TSO code because we check.
> >
> > Hmm, so you are saying there is a bug somewhere  in ixbe_rxtx.c?
> > What particular place you are talking about?
> 
> Sorry, I spoke too fast. In TSO code, we check PKT_TX_IP_CKSUM (and not
> PKT_TX_IPV4 as I thought), so it would work for both methods without
> patching the code.
> 
> In this case, it means that both approach would not require to
> modify the code.

Ok.

> 
> >>>>     *Problem 3*: without using the word "fortville", it is difficult
> >>>>     to understand the goal of the flag PKT_TX_UDP_TUNNEL_PKT. Indeed,
> >>>>     once PKT_TX_OUTER_IPV4/6 is set, it looks obvious that it's a
> >>>>     tunnel packet. I suggest to remove the PKT_TX_UDP_TUNNEL_PKT
> >>>>     flag. In linux, the driver doesn't care about the tunnel type,
> >>>>     it always set I40E_TXD_CTX_UDP_TUNNELING for all encapsulations [6].
> >>>
> >>> It might be obvious that it is a tunnel packet from PKT_TX_OUTER_* is set,
> >>> but it is not obvious what type of tunnelling it would be.
> >>> FVL HW supports HW TX offloads for different type of tunnelling and
> >>> requires that SW provide information about tunnelling type.
> >>>  From i40e datasheet:
> >>> L4TUNT L4 Tunneling Type (Teredo / GRE header / VXLAN header) indication:
> >>> 00b - No UDP / GRE tunneling (field must be set to zero if EIPT equals to zero)
> >>> 01b - UDP tunneling header (any UDP tunneling, VXLAN and Geneve).
> >>> 10b - GRE tunneling header
> >>> As we do plan to support other than UDP tunnelling types, I suppose we'll need to keep
> >>> PKT_TX_UDP_TUNNEL_PKT flag.
> >>
> >> As I've said: in linux, the driver doesn't care about the tunnel type,
> >> it always set I40E_TXD_CTX_UDP_TUNNELING for all encapsulations.
> >
> > Ok, and why it should be our problem?
> > We have a lot of things done in a different manner then linux/freebsd kernel drivers,
> > Why now it became a problem?
> 
> If linux doesn't need an equivalent flag for doing the same thing,
> it probably means we don't need it either.

Probably yes .... Or probably not.
Why do we need to guess what was the intention of guys who wrote that part of linux driver?
BTW, the macro for GRE is here:
find lib/librte_pmd_i40e/i40e -type f | xargs grep TUN | grep TXD
lib/librte_pmd_i40e/i40e/i40e_type.h:#define I40E_TXD_CTX_UDP_TUNNELING (0x1ULL << I40E_TXD_CTX_QW0_NATT_SHIFT)
lib/librte_pmd_i40e/i40e/i40e_type.h:#define I40E_TXD_CTX_GRE_TUNNELING (0x2ULL << I40E_TXD_CTX_QW0_NATT_SHIFT)

Though it not used (yet?) by some reason. 

> 
> In a performance-oriented software like dpdk, having a flag that we
> don't know what the hardware does with, that is not needed in other
> drivers of the same harware, that makes the API harder to understand
> could be a problem.

Here is a HW spec, that says what values have to be setup for L4TUNT.
Yes, I am not sure why they need to distinguish between VXLAN/GRE tunnelling.
Though, I suppose that wouldn't eliminate the requirement.
But for same, there is no good explanation why FVL HW need to know that it is IPv4 or IPv6 packet,
in the case when only L4 checksum offload is required (IIPT field).
Niantic, as I remember, is able to work ok without that requirement.   
Though, we still have to set it up.
 
> Another argument: if we can remove this flag, it would make the
> testpmd commands reworkd proposed by Jijiang much more easy to
> understand: only a new "csum parse-tunnel on|off" would be required,
> and it can be explained in a few words.

Well, from my point - testpmd commands that Jijiang proposed are perfectly clear and understandable. 
Another thing, as I remember, our primary concern should be public API, no testpmd.

> 
> I'll try to do some tests on a fortville NIC if I can find one. I'm
> curious to see if we can transmit any encapsulation packet (ip in ip,
> ip in gre, eth in gre, eth in vxlan, or even a proprietary tunnel).

Ok cool.

> 
> We should avoid the need to specify the tunnel type in the OUTER
> checksum API if we can, else it would limit us to specific
> supported protocols.

>From the FVL spec it is required by HW, it is not what we introducing on our own.
Spec stays explicitly that L4TUNT (L4 tunneling type) has to be setup for tunnelling packets.
Again from the spec, there are 3 different values it can take.
If you have an idea how to pass that information to  PMD without using flags, sure we can consider it.

> 
> >>>> I think the following cases should be *forbidden by the API*:
> >>>>
> >>>> case 9) calculate checksum of in_ip and in_tcp  (was case B.1 in [1])
> >>>>
> >>>>     mb->outer_l2_len = len(out_eth)
> >>>>     mb->outer_l3_len = len(out_ip)
> >>>>     mb->l2_len = len(out_udp + vxlan + in_eth)
> >>>>     mb->l3_len = len(out_ip)
> >>>>     mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_UDP_TUNNEL_PKT | \
> >>>>       PKT_TX_IP_CSUM | PKT_TX_UDP_CKSUM;
> >>>>     set out_ip checksum to 0 in the packet
> >>>>     set out_udp checksum to pseudo header using rte_ipv4_phdr_cksum()
> >>>>
> >>>>     If we remove the flag PKT_TX_UDP_TUNNEL_PKT, this cannot be
> >>>>     supported, but there is no reason to support it as there is
> >>>>     already one way to do the same.
> >>>>
> >>>>     I think the driver should not even look at mb->outer_l2_len
> >>>>     and mb->outer_l3_len if no flag PKT_TX_OUTER_* is set.
> >>>
> >>> Why it should be forbidden?
> >>> I admit it might be a bit slower than case 4),
> >>> but I think absolutely legal way to setup HW offloads for inner L3/L4.
> >>> As I said we need a PKT_TX_UDP_TUNNEL_PKT anyway, so I suppose
> >>> PKT_TX_*_TUNNEL_PKT should be an indication is it a tunnel packet or not.
> >>> PKT_TX_OUTER_* flags indicate does outer cksum offload is required or not.
> >>
> >> I don't understand. The result in terms of hardware is exactly the
> >> same than case 4). Why should we have 2 different ways for doing the
> >> same thing?
> >
> > If HW supports that capability, why should we forbid it?
> > Let user to choose himself what way to use.
> > FVL spec lists it as a valid approach.
> 
> It is not a hardware feature.

It is.

> Case 4) and case 9) would fill the hardware registers exactly the same.

No, they wouldn't.
Please read corresponding section of FVL spec and i40e_rxtx.c
For case 4) we only need to setup TDD (TX data descriptor) with the following values:
IIPT, IPLEN, L4T, L4LEN 
For case 9) we need to setup both TDD and TCD (TX context descriptor) with the following values:
TDD: IIPT, IPLEN, L4T, L4LEN
TCD: EIPT, EIPLEN,  L4TUNT, L4TUNLEN 

> To me, it's just an API question.

No, it is not.

I still don't understand why you are so eager to 'forbid' it.
Yes we support it for FVL, but no one forces you to use it. 

> 
> > As one of possible use-cases:  HW VLAN tags insertion for both inner and outer packets.
> > FVL can do that, though as I know our PMD doesn't implement it yet.
> > For that, we'll need to specify at least:
> > outer_l2_len, outer_l3_len, l2_len.
> > While PKT_TX_OUTER_* might stay cleared.
> 
> If a VLAN flag has to be inserted in outer header, a new flag
> PKT_TX_OUTER_INSERT_VLAN would be added. So my specification
> would still be correct:
> 
>    The driver should look at mb->outer_lX_len only if a
>    PKT_TX_OUTER_* flag is present.
> 

Introducing PKT_TX_OUTER_INSERT_VLAN is ok.
Though I still think we'll need TX_*_TUNNEL flags and no need to 'forbid' case 9).
BTW, as I can see linux i40e driver for tunnelling packets uses case 9), not case 4), right?
Konstantin

> >> This is really confusing for an API. Moreover, you said
> >> it: it is slower that case 4).
> >
> > I don't know would be slower then 4) or not for sure.
> > That's my guess, based on the fact that for 9) we need to fill 2 descriptors, while fro 4) - only 1.
> > Though I didn't measure the difference.
> > That's actually one more reason why to allow and support it -
> > so people can make sure that on FVL both ways work as expected and measure the difference.
> >
> > Konstantin
> 
> 
> Regards,
> Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Olivier MATZ]

Hi Konstantin,

On 01/20/2015 06:23 PM, Ananyev, Konstantin wrote:
>> Sure, it does not make a big difference in terms of code. But
>> in terms of API, the naming of the flag is coherent to what it is
>> used for. And it's easier to find a simple definition, like:
>>
>>    * Packet is IPv4. This flag must be set when using any offload feature
>>    * (TSO, L3 or L4 checksum) to tell the NIC that the packet is an IPv4
>>    * packet.
>
> Ok, and what's wrong with:
> "Packet is IPv4. This flag must be set when using any offload feature
> (TSO, L3 or L4 checksum) to tell the NIC that the packet is an IPv4
> packet and no HW offload for IPv4 header checksum calculation is required"
> ?

I honestly find the first one simpler.

Again, I understand both are possible, but I think choosing the
most trivial one is the right way for an API.


>>> Ok, and why it should be our problem?
>>> We have a lot of things done in a different manner then linux/freebsd kernel drivers,
>>> Why now it became a problem?
>>
>> If linux doesn't need an equivalent flag for doing the same thing,
>> it probably means we don't need it either.
>
> Probably yes .... Or probably not.
> Why do we need to guess what was the intention of guys who wrote that part of linux driver?

Because the dpdk looks very similar to that part of linux driver.

> BTW, the macro for GRE is here:
> find lib/librte_pmd_i40e/i40e -type f | xargs grep TUN | grep TXD
> lib/librte_pmd_i40e/i40e/i40e_type.h:#define I40E_TXD_CTX_UDP_TUNNELING (0x1ULL << I40E_TXD_CTX_QW0_NATT_SHIFT)
> lib/librte_pmd_i40e/i40e/i40e_type.h:#define I40E_TXD_CTX_GRE_TUNNELING (0x2ULL << I40E_TXD_CTX_QW0_NATT_SHIFT)
>
> Though it not used (yet?) by some reason.
>
>>
>> In a performance-oriented software like dpdk, having a flag that we
>> don't know what the hardware does with, that is not needed in other
>> drivers of the same harware, that makes the API harder to understand
>> could be a problem.
>
> Here is a HW spec, that says what values have to be setup for L4TUNT.
> Yes, I am not sure why they need to distinguish between VXLAN/GRE tunnelling.
> Though, I suppose that wouldn't eliminate the requirement.
> But for same, there is no good explanation why FVL HW need to know that it is IPv4 or IPv6 packet,
> in the case when only L4 checksum offload is required (IIPT field).
> Niantic, as I remember, is able to work ok without that requirement.
> Though, we still have to set it up.
>
>> Another argument: if we can remove this flag, it would make the
>> testpmd commands reworkd proposed by Jijiang much more easy to
>> understand: only a new "csum parse-tunnel on|off" would be required,
>> and it can be explained in a few words.
>
> Well, from my point - testpmd commands that Jijiang proposed are perfectly clear and understandable.
> Another thing, as I remember, our primary concern should be public API, no testpmd.

OK let's talk about testpmd later.

>> We should avoid the need to specify the tunnel type in the OUTER
>> checksum API if we can, else it would limit us to specific
>> supported protocols.
>
>  From the FVL spec it is required by HW, it is not what we introducing on our own.
> Spec stays explicitly that L4TUNT (L4 tunneling type) has to be setup for tunnelling packets.
> Again from the spec, there are 3 different values it can take.
> If you have an idea how to pass that information to  PMD without using flags, sure we can consider it.
>
>>
>>>>>> I think the following cases should be *forbidden by the API*:
>>>>>>
>>>>>> case 9) calculate checksum of in_ip and in_tcp  (was case B.1 in [1])
>>>>>>
>>>>>>      mb->outer_l2_len = len(out_eth)
>>>>>>      mb->outer_l3_len = len(out_ip)
>>>>>>      mb->l2_len = len(out_udp + vxlan + in_eth)
>>>>>>      mb->l3_len = len(out_ip)
>>>>>>      mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_UDP_TUNNEL_PKT | \
>>>>>>        PKT_TX_IP_CSUM | PKT_TX_UDP_CKSUM;
>>>>>>      set out_ip checksum to 0 in the packet
>>>>>>      set out_udp checksum to pseudo header using rte_ipv4_phdr_cksum()
>>>>>>
>>>>>>      If we remove the flag PKT_TX_UDP_TUNNEL_PKT, this cannot be
>>>>>>      supported, but there is no reason to support it as there is
>>>>>>      already one way to do the same.
>>>>>>
>>>>>>      I think the driver should not even look at mb->outer_l2_len
>>>>>>      and mb->outer_l3_len if no flag PKT_TX_OUTER_* is set.
>>>>>
>>>>> Why it should be forbidden?
>>>>> I admit it might be a bit slower than case 4),
>>>>> but I think absolutely legal way to setup HW offloads for inner L3/L4.
>>>>> As I said we need a PKT_TX_UDP_TUNNEL_PKT anyway, so I suppose
>>>>> PKT_TX_*_TUNNEL_PKT should be an indication is it a tunnel packet or not.
>>>>> PKT_TX_OUTER_* flags indicate does outer cksum offload is required or not.
>>>>
>>>> I don't understand. The result in terms of hardware is exactly the
>>>> same than case 4). Why should we have 2 different ways for doing the
>>>> same thing?
>>>
>>> If HW supports that capability, why should we forbid it?
>>> Let user to choose himself what way to use.
>>> FVL spec lists it as a valid approach.
>>
>> It is not a hardware feature.
>
> It is.
>
>> Case 4) and case 9) would fill the hardware registers exactly the same.
>
> No, they wouldn't.
> Please read corresponding section of FVL spec and i40e_rxtx.c
> For case 4) we only need to setup TDD (TX data descriptor) with the following values:
> IIPT, IPLEN, L4T, L4LEN
> For case 9) we need to setup both TDD and TCD (TX context descriptor) with the following values:
> TDD: IIPT, IPLEN, L4T, L4LEN
> TCD: EIPT, EIPLEN,  L4TUNT, L4TUNLEN
>
>> To me, it's just an API question.
>
> No, it is not.
>
> I still don't understand why you are so eager to 'forbid' it.
> Yes we support it for FVL, but no one forces you to use it.

Well, how would you describe this 2 ways of doing the same thing
in the offload API? Would you talk about the i40e registers? It's not
because i40e has 2 ways to do the same operation that the DPDK should
do the same.

How will you explain to a user how to choose between these 2 cases?

Having to support these 2 different cases for the same thing will
complexify all future drivers that will not work the same way than i40e.

>>> As one of possible use-cases:  HW VLAN tags insertion for both inner and outer packets.
>>> FVL can do that, though as I know our PMD doesn't implement it yet.
>>> For that, we'll need to specify at least:
>>> outer_l2_len, outer_l3_len, l2_len.
>>> While PKT_TX_OUTER_* might stay cleared.
>>
>> If a VLAN flag has to be inserted in outer header, a new flag
>> PKT_TX_OUTER_INSERT_VLAN would be added. So my specification
>> would still be correct:
>>
>>     The driver should look at mb->outer_lX_len only if a
>>     PKT_TX_OUTER_* flag is present.
>>
>
> Introducing PKT_TX_OUTER_INSERT_VLAN is ok.
> Though I still think we'll need TX_*_TUNNEL flags and no need to 'forbid' case 9).
> BTW, as I can see linux i40e driver for tunnelling packets uses case 9), not case 4), right?

I need to check this.

Regards,
Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Liu, Jijiang]

Hi,

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Wednesday, January 21, 2015 2:16 AM
> To: Ananyev, Konstantin; Liu, Jijiang
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and
> csum forwarding engine
> 
> 
> 
> >>> Ok, and why it should be our problem?
> >>> We have a lot of things done in a different manner then
> >>> linux/freebsd kernel drivers, Why now it became a problem?
> >>
> >> If linux doesn't need an equivalent flag for doing the same thing, it
> >> probably means we don't need it either.
> >
> > Probably yes .... Or probably not.
> > Why do we need to guess what was the intention of guys who wrote that
> part of linux driver?
> 
> Because the dpdk looks very similar to that part of linux driver.

A  guy from Intel  who have already confirmed that the NVGRE is not supported yet in Linux kernel.
  
He said "So far as I know it is not yet supported and I have no information on when it will be."

> > BTW, the macro for GRE is here:
> > find lib/librte_pmd_i40e/i40e -type f | xargs grep TUN | grep TXD
> > lib/librte_pmd_i40e/i40e/i40e_type.h:#define
> > I40E_TXD_CTX_UDP_TUNNELING (0x1ULL <<
> I40E_TXD_CTX_QW0_NATT_SHIFT)
> > lib/librte_pmd_i40e/i40e/i40e_type.h:#define
> > I40E_TXD_CTX_GRE_TUNNELING (0x2ULL <<
> I40E_TXD_CTX_QW0_NATT_SHIFT)
> >
> > Though it not used (yet?) by some reason.
> >
> >>
> 
> Regards,
> Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Olivier MATZ]

Hi,

On 01/21/2015 04:12 AM, Liu, Jijiang wrote:
>>>>> Ok, and why it should be our problem?
>>>>> We have a lot of things done in a different manner then
>>>>> linux/freebsd kernel drivers, Why now it became a problem?
>>>>
>>>> If linux doesn't need an equivalent flag for doing the same thing, it
>>>> probably means we don't need it either.
>>>
>>> Probably yes .... Or probably not.
>>> Why do we need to guess what was the intention of guys who wrote that
>> part of linux driver?
>>
>> Because the dpdk looks very similar to that part of linux driver.
>
> A  guy from Intel  who have already confirmed that the NVGRE is not supported yet in Linux kernel.
>
> He said "So far as I know it is not yet supported and I have no information on when it will be."

I added the support of Ether over GRE, IP over GRE and IP over IP
tunnels in csumonly to do the test. I ask the csum forward engine
to calculate inner IP+TCP checksums, and outer IP (case 6 in [1]).
Here are the results:

1/ When I use I40E_TXD_CTX_UDP_TUNNELING:
- vxlan: all checksums ok
- eth over gre: all checksums ok
- ip over gre: not transmitted by hw
- ip over ip: all checksums wrong (set to 0 by hw)

2/ When I use I40E_TXD_CTX_GRE_TUNNELING:
- vxlan: checksums ok
- eth over gre: all checksums ok
- ip over gre: all checksums ok
- ip over ip: all checksums wrong (set to 0 by hw)

3/ When I use 00b:
- vxlan: all checksums ok
- eth over gre: all checksums ok
- ip over gre: all checksums ok
- ip over ip: checksums wrong (set to 0 by hw)

All the ip over ip tests do not work yet for an unknown reason.
There is maybe something wrong in my app or in the driver
(although the registers looks consistent with the datasheet).

I think we could use 3/ for all tunnels, because the ipip case
is supposed to work according to the datasheet, and all other cases
work too.

It would allow to remove the UDP_TUNNELING flag from mbuf API.

I will send a RFC patch that provides the API change and this new
feature in csum forward engine, with full tests on ixgbe and i40e
and explanations for all changes.

Regards,
Olivier

[1] http://dpdk.org/ml/archives/dev/2015-January/011127.html

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Ananyev, Konstantin]

Hi Olivier,

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Wednesday, January 21, 2015 3:25 PM
> To: Liu, Jijiang; Ananyev, Konstantin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine
> 
> Hi,
> 
> On 01/21/2015 04:12 AM, Liu, Jijiang wrote:
> >>>>> Ok, and why it should be our problem?
> >>>>> We have a lot of things done in a different manner then
> >>>>> linux/freebsd kernel drivers, Why now it became a problem?
> >>>>
> >>>> If linux doesn't need an equivalent flag for doing the same thing, it
> >>>> probably means we don't need it either.
> >>>
> >>> Probably yes .... Or probably not.
> >>> Why do we need to guess what was the intention of guys who wrote that
> >> part of linux driver?
> >>
> >> Because the dpdk looks very similar to that part of linux driver.
> >
> > A  guy from Intel  who have already confirmed that the NVGRE is not supported yet in Linux kernel.
> >
> > He said "So far as I know it is not yet supported and I have no information on when it will be."
> 
> I added the support of Ether over GRE, IP over GRE and IP over IP
> tunnels in csumonly to do the test. I ask the csum forward engine
> to calculate inner IP+TCP checksums, and outer IP (case 6 in [1]).
> Here are the results:
> 
> 1/ When I use I40E_TXD_CTX_UDP_TUNNELING:
> - vxlan: all checksums ok
> - eth over gre: all checksums ok
> - ip over gre: not transmitted by hw
> - ip over ip: all checksums wrong (set to 0 by hw)
> 
> 2/ When I use I40E_TXD_CTX_GRE_TUNNELING:
> - vxlan: checksums ok
> - eth over gre: all checksums ok
> - ip over gre: all checksums ok
> - ip over ip: all checksums wrong (set to 0 by hw)
> 
> 3/ When I use 00b:
> - vxlan: all checksums ok
> - eth over gre: all checksums ok
> - ip over gre: all checksums ok
> - ip over ip: checksums wrong (set to 0 by hw)

Wow, so there is absolutely no difference in results for L4TUNT=2(GRE) and L4TUNT=0, right?
And IP over IP doesn't work at all?
I suppose you set L4TUNLEN as described in spec for each case, right?
That looks really weird to me and as I can see completely contradicts with what spec.
I suppose we'll need to reproduce all that tests on our HW too.
Could you send to us a patch with your changes, so we can try same thing?
Or just a dump of TDD and TCD values for each case. 
Konstantin

> 
> All the ip over ip tests do not work yet for an unknown reason.
> There is maybe something wrong in my app or in the driver
> (although the registers looks consistent with the datasheet).
> 
> I think we could use 3/ for all tunnels, because the ipip case
> is supposed to work according to the datasheet, and all other cases
> work too.
> 
> It would allow to remove the UDP_TUNNELING flag from mbuf API.
> 
> I will send a RFC patch that provides the API change and this new
> feature in csum forward engine, with full tests on ixgbe and i40e
> and explanations for all changes.
> 
> Regards,
> Olivier
> 
> [1] http://dpdk.org/ml/archives/dev/2015-January/011127.html

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Olivier MATZ]

Hi Konstantin,

On 01/21/2015 05:28 PM, Ananyev, Konstantin wrote:
>> I added the support of Ether over GRE, IP over GRE and IP over IP
>> tunnels in csumonly to do the test. I ask the csum forward engine
>> to calculate inner IP+TCP checksums, and outer IP (case 6 in [1]).
>> Here are the results:
>>
>> 1/ When I use I40E_TXD_CTX_UDP_TUNNELING:
>> - vxlan: all checksums ok
>> - eth over gre: all checksums ok
>> - ip over gre: not transmitted by hw
>> - ip over ip: all checksums wrong (set to 0 by hw)
>>
>> 2/ When I use I40E_TXD_CTX_GRE_TUNNELING:
>> - vxlan: checksums ok
>> - eth over gre: all checksums ok
>> - ip over gre: all checksums ok
>> - ip over ip: all checksums wrong (set to 0 by hw)
>>
>> 3/ When I use 00b:
>> - vxlan: all checksums ok
>> - eth over gre: all checksums ok
>> - ip over gre: all checksums ok
>> - ip over ip: checksums wrong (set to 0 by hw)
>
> Wow, so there is absolutely no difference in results for L4TUNT=2(GRE) and L4TUNT=0, right?
> And IP over IP doesn't work at all?

Right. I probably missed something in i40e driver. The application seems
to fill the mbuf properly.

> I suppose you set L4TUNLEN as described in spec for each case, right?

I think so.

> That looks really weird to me and as I can see completely contradicts with what spec.
> I suppose we'll need to reproduce all that tests on our HW too.
> Could you send to us a patch with your changes, so we can try same thing?
> Or just a dump of TDD and TCD values for each case.

Sure, I'm going to send all my code and tests in a RFC patchset in
a few minutes. By the way, I'm off tomorrow, I won't be able to
answer.

Regards,
Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Ananyev, Konstantin]

Hi lads,

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Wednesday, January 21, 2015 3:25 PM
> To: Liu, Jijiang; Ananyev, Konstantin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine
> 
> Hi,
> 
> On 01/21/2015 04:12 AM, Liu, Jijiang wrote:
> >>>>> Ok, and why it should be our problem?
> >>>>> We have a lot of things done in a different manner then
> >>>>> linux/freebsd kernel drivers, Why now it became a problem?
> >>>>
> >>>> If linux doesn't need an equivalent flag for doing the same thing, it
> >>>> probably means we don't need it either.
> >>>
> >>> Probably yes .... Or probably not.
> >>> Why do we need to guess what was the intention of guys who wrote that
> >> part of linux driver?
> >>
> >> Because the dpdk looks very similar to that part of linux driver.
> >
> > A  guy from Intel  who have already confirmed that the NVGRE is not supported yet in Linux kernel.
> >
> > He said "So far as I know it is not yet supported and I have no information on when it will be."
> 
> I added the support of Ether over GRE, IP over GRE and IP over IP
> tunnels in csumonly to do the test. I ask the csum forward engine
> to calculate inner IP+TCP checksums, and outer IP (case 6 in [1]).
> Here are the results:
> 
> 1/ When I use I40E_TXD_CTX_UDP_TUNNELING:
> - vxlan: all checksums ok
> - eth over gre: all checksums ok
> - ip over gre: not transmitted by hw
> - ip over ip: all checksums wrong (set to 0 by hw)
> 
> 2/ When I use I40E_TXD_CTX_GRE_TUNNELING:
> - vxlan: checksums ok
> - eth over gre: all checksums ok
> - ip over gre: all checksums ok
> - ip over ip: all checksums wrong (set to 0 by hw)
> 
> 3/ When I use 00b:
> - vxlan: all checksums ok
> - eth over gre: all checksums ok
> - ip over gre: all checksums ok
> - ip over ip: checksums wrong (set to 0 by hw)
> 
> All the ip over ip tests do not work yet for an unknown reason.
> There is maybe something wrong in my app or in the driver
> (although the registers looks consistent with the datasheet).
> 
> I think we could use 3/ for all tunnels, because the ipip case
> is supposed to work according to the datasheet, and all other cases
> work too.
> 
> It would allow to remove the UDP_TUNNELING flag from mbuf API.
> 
> I will send a RFC patch that provides the API change and this new
> feature in csum forward engine, with full tests on ixgbe and i40e
> and explanations for all changes.
> 
> Regards,
> Olivier
> 
> [1] http://dpdk.org/ml/archives/dev/2015-January/011127.html

I tried to repeat Olivier test-cases on my box.
Though, I didn't use test-pmd cusmonly and  i40ePMD logic, but filled TCD and TDD mostly from hardcoded values.
That's  what I got:

4 input packets:
a) ETHER/IPv4/UDP/VXLAN/ETHER/IPV4/TCP
b) ETHER/IPv4/GRE/ETHER/IPV4/TCP
c) ETHER/IPv4/GRE/IPV4/TCP
d) ETHER/IPv4/IPV4/TCP

1/ L4TUNT==1(I40E_TXD_CTX_UDP_TUNNELING):
a),b): all checksums ok
c),d): not transmitted by HW.

2/ L4TUNT==2(I40E_TXD_CTX_GRE_TUNNELING):
a) b),c): all checksums ok
d): not transmitted by HW.

3/ L4TUNT==0(UNKNOWN):
a),b),c),d): all checksums ok

So yes, it seems that L4TUNT==0 works perfectly ok for all cases, as long as L4TUNLEN and other TCD values are setup properly.
Which makes me think, that  probably we can do what you suggested: just use L4TUNT=0 for all cases. 
Though as Jijiang said, we waiting for confirmation from FVL guys, that there are no hidden implications with that approach.

Another thing - IPIP seems to work ok by HW.
There is something wrong on our (PMD/test-pmd) side.
I think at least we have to remove the following check:
if (!l2_len) {
                PMD_DRV_LOG(DEBUG, "L2 length set to 0");
                return;
        }
in i40e_txd_enable_checksum().

Konstantin

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Liu, Jijiang]

Hi,

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Monday, January 26, 2015 12:14 PM
> To: Olivier MATZ; Liu, Jijiang
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and
> csum forwarding engine
> 
> Hi lads,
> 
> > -----Original Message-----
> > From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> > Sent: Wednesday, January 21, 2015 3:25 PM
> > To: Liu, Jijiang; Ananyev, Konstantin
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and
> > csum forwarding engine
> >
> > Hi,
> >
> > On 01/21/2015 04:12 AM, Liu, Jijiang wrote:
> > >>>>> Ok, and why it should be our problem?
> > >>>>> We have a lot of things done in a different manner then
> > >>>>> linux/freebsd kernel drivers, Why now it became a problem?
> > >>>>
> > >>>> If linux doesn't need an equivalent flag for doing the same
> > >>>> thing, it probably means we don't need it either.
> > >>>
> > >>> Probably yes .... Or probably not.
> > >>> Why do we need to guess what was the intention of guys who wrote
> > >>> that
> > >> part of linux driver?
> > >>
> > >> Because the dpdk looks very similar to that part of linux driver.
> > >
> > > A  guy from Intel  who have already confirmed that the NVGRE is not
> supported yet in Linux kernel.
> > >
> > > He said "So far as I know it is not yet supported and I have no information on
> when it will be."
> >
> > I added the support of Ether over GRE, IP over GRE and IP over IP
> > tunnels in csumonly to do the test. I ask the csum forward engine to
> > calculate inner IP+TCP checksums, and outer IP (case 6 in [1]).
> > Here are the results:
> >
> > 1/ When I use I40E_TXD_CTX_UDP_TUNNELING:
> > - vxlan: all checksums ok
> > - eth over gre: all checksums ok
> > - ip over gre: not transmitted by hw
> > - ip over ip: all checksums wrong (set to 0 by hw)
> >
> > 2/ When I use I40E_TXD_CTX_GRE_TUNNELING:
> > - vxlan: checksums ok
> > - eth over gre: all checksums ok
> > - ip over gre: all checksums ok
> > - ip over ip: all checksums wrong (set to 0 by hw)
> >
> > 3/ When I use 00b:
> > - vxlan: all checksums ok
> > - eth over gre: all checksums ok
> > - ip over gre: all checksums ok
> > - ip over ip: checksums wrong (set to 0 by hw)
> >
> > All the ip over ip tests do not work yet for an unknown reason.
> > There is maybe something wrong in my app or in the driver (although
> > the registers looks consistent with the datasheet).
> >
> > I think we could use 3/ for all tunnels, because the ipip case is
> > supposed to work according to the datasheet, and all other cases work
> > too.
> >
> > It would allow to remove the UDP_TUNNELING flag from mbuf API.
> >
> > I will send a RFC patch that provides the API change and this new
> > feature in csum forward engine, with full tests on ixgbe and i40e and
> > explanations for all changes.
> >
> > Regards,
> > Olivier
> >
> > [1] http://dpdk.org/ml/archives/dev/2015-January/011127.html
> 
> I tried to repeat Olivier test-cases on my box.
> Though, I didn't use test-pmd cusmonly and  i40ePMD logic, but filled TCD and
> TDD mostly from hardcoded values.
> That's  what I got:
> 
> 4 input packets:
> a) ETHER/IPv4/UDP/VXLAN/ETHER/IPV4/TCP
> b) ETHER/IPv4/GRE/ETHER/IPV4/TCP
> c) ETHER/IPv4/GRE/IPV4/TCP
> d) ETHER/IPv4/IPV4/TCP
> 
> 1/ L4TUNT==1(I40E_TXD_CTX_UDP_TUNNELING):
> a),b): all checksums ok
> c),d): not transmitted by HW.
> 
> 2/ L4TUNT==2(I40E_TXD_CTX_GRE_TUNNELING):
> a) b),c): all checksums ok
> d): not transmitted by HW.
> 
> 3/ L4TUNT==0(UNKNOWN):
> a),b),c),d): all checksums ok
> 
> So yes, it seems that L4TUNT==0 works perfectly ok for all cases, as long as
> L4TUNLEN and other TCD values are setup properly.
> Which makes me think, that  probably we can do what you suggested: just use
> L4TUNT=0 for all cases.
> Though as Jijiang said, we waiting for confirmation from FVL guys, that there are
> no hidden implications with that approach.

Yes, the L4TUNT=0 is ok  for all cases.

But we still need to get confirmation from FVL guys, probably there are some issues in HW/FW.
I and Helin will confirm this with FVL guys ASAP.


> Another thing - IPIP seems to work ok by HW.
> There is something wrong on our (PMD/test-pmd) side.
> I think at least we have to remove the following check:
> if (!l2_len) {
>                 PMD_DRV_LOG(DEBUG, "L2 length set to 0");
>                 return;
>         }
> in i40e_txd_enable_checksum().

Yes, for IPIP, the check should be removed.

> Konstantin

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Olivier MATZ]

Hi,

On 01/26/2015 07:02 AM, Liu, Jijiang wrote:
>> I tried to repeat Olivier test-cases on my box.
>> Though, I didn't use test-pmd cusmonly and  i40ePMD logic, but filled TCD and
>> TDD mostly from hardcoded values.
>> That's  what I got:
>>
>> 4 input packets:
>> a) ETHER/IPv4/UDP/VXLAN/ETHER/IPV4/TCP
>> b) ETHER/IPv4/GRE/ETHER/IPV4/TCP
>> c) ETHER/IPv4/GRE/IPV4/TCP
>> d) ETHER/IPv4/IPV4/TCP
>>
>> 1/ L4TUNT==1(I40E_TXD_CTX_UDP_TUNNELING):
>> a),b): all checksums ok
>> c),d): not transmitted by HW.
>>
>> 2/ L4TUNT==2(I40E_TXD_CTX_GRE_TUNNELING):
>> a) b),c): all checksums ok
>> d): not transmitted by HW.
>>
>> 3/ L4TUNT==0(UNKNOWN):
>> a),b),c),d): all checksums ok
>>
>> So yes, it seems that L4TUNT==0 works perfectly ok for all cases, as long as
>> L4TUNLEN and other TCD values are setup properly.
>> Which makes me think, that  probably we can do what you suggested: just use
>> L4TUNT=0 for all cases.
>> Though as Jijiang said, we waiting for confirmation from FVL guys, that there are
>> no hidden implications with that approach.
> 
> Yes, the L4TUNT=0 is ok  for all cases.

Great! Thanks for testing on your side too.

> But we still need to get confirmation from FVL guys, probably there are some issues in HW/FW.
> I and Helin will confirm this with FVL guys ASAP.

OK, thank you.

>> Another thing - IPIP seems to work ok by HW.
>> There is something wrong on our (PMD/test-pmd) side.
>> I think at least we have to remove the following check:
>> if (!l2_len) {
>>                 PMD_DRV_LOG(DEBUG, "L2 length set to 0");
>>                 return;
>>         }
>> in i40e_txd_enable_checksum().
> 
> Yes, for IPIP, the check should be removed.

Yes, I think these lines should be removed for 2 reasons:
- it may be the cause of ipip tunnel not working
- we shouldn't do these kind of tests in dataplane. I think we have to
  suppose that the data passed to the PMD is valid.

I'll redo the test with ipip tomorrow with this fix and let you
know the result. If it works, I'll add this in the next version
of the patch.

Regards,
Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Ananyev, Konstantin]

Hi Olivier,

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Monday, January 26, 2015 2:07 PM
> To: Liu, Jijiang; Ananyev, Konstantin; Zhang, Helin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine
> 
> Hi,
> 
> On 01/26/2015 07:02 AM, Liu, Jijiang wrote:
> >> I tried to repeat Olivier test-cases on my box.
> >> Though, I didn't use test-pmd cusmonly and  i40ePMD logic, but filled TCD and
> >> TDD mostly from hardcoded values.
> >> That's  what I got:
> >>
> >> 4 input packets:
> >> a) ETHER/IPv4/UDP/VXLAN/ETHER/IPV4/TCP
> >> b) ETHER/IPv4/GRE/ETHER/IPV4/TCP
> >> c) ETHER/IPv4/GRE/IPV4/TCP
> >> d) ETHER/IPv4/IPV4/TCP
> >>
> >> 1/ L4TUNT==1(I40E_TXD_CTX_UDP_TUNNELING):
> >> a),b): all checksums ok
> >> c),d): not transmitted by HW.
> >>
> >> 2/ L4TUNT==2(I40E_TXD_CTX_GRE_TUNNELING):
> >> a) b),c): all checksums ok
> >> d): not transmitted by HW.
> >>
> >> 3/ L4TUNT==0(UNKNOWN):
> >> a),b),c),d): all checksums ok
> >>
> >> So yes, it seems that L4TUNT==0 works perfectly ok for all cases, as long as
> >> L4TUNLEN and other TCD values are setup properly.
> >> Which makes me think, that  probably we can do what you suggested: just use
> >> L4TUNT=0 for all cases.
> >> Though as Jijiang said, we waiting for confirmation from FVL guys, that there are
> >> no hidden implications with that approach.
> >
> > Yes, the L4TUNT=0 is ok  for all cases.
> 
> Great! Thanks for testing on your side too.
> 
> > But we still need to get confirmation from FVL guys, probably there are some issues in HW/FW.
> > I and Helin will confirm this with FVL guys ASAP.
> 
> OK, thank you.
> 
> >> Another thing - IPIP seems to work ok by HW.
> >> There is something wrong on our (PMD/test-pmd) side.
> >> I think at least we have to remove the following check:
> >> if (!l2_len) {
> >>                 PMD_DRV_LOG(DEBUG, "L2 length set to 0");
> >>                 return;
> >>         }
> >> in i40e_txd_enable_checksum().
> >
> > Yes, for IPIP, the check should be removed.
> 
> Yes, I think these lines should be removed for 2 reasons:
> - it may be the cause of ipip tunnel not working
> - we shouldn't do these kind of tests in dataplane. I think we have to
>   suppose that the data passed to the PMD is valid.
> 
> I'll redo the test with ipip tomorrow with this fix and let you
> know the result. If it works, I'll add this in the next version
> of the patch.

While you are on this, can I suggest you'll add debug logging for TCD and TDD we are writing to the TX ring?
Something like that:

+                       PMD_TX_LOG(DEBUG, "mbuf: %p, TCD[%u]:\n"
+                               "tunneling_params: %#x;\n"
+                               "l2tag2: %#hx;\n"
+                               "rsvd: %#hx;\n"
+                               "type_cmd_tso_mss: %#lx;\n",
+                               tx_pkt, tx_id,
+                               ctx_txd->tunneling_params,
+                               ctx_txd->l2tag2,
+                               ctx_txd->rsvd,
+                               ctx_txd->type_cmd_tso_mss);

And same for TDD. 
It  helped me a lot to figure out what is going on, when I did my tests.
Probably would be useful for other people too.

Konstantin

> 
> Regards,
> Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Olivier MATZ]

Hi Konstantin,

On 01/26/2015 03:15 PM, Ananyev, Konstantin wrote:
>>>> Another thing - IPIP seems to work ok by HW.
>>>> There is something wrong on our (PMD/test-pmd) side.
>>>> I think at least we have to remove the following check:
>>>> if (!l2_len) {
>>>>                  PMD_DRV_LOG(DEBUG, "L2 length set to 0");
>>>>                  return;
>>>>          }
>>>> in i40e_txd_enable_checksum().
>>>
>>> Yes, for IPIP, the check should be removed.
>>
>> Yes, I think these lines should be removed for 2 reasons:
>> - it may be the cause of ipip tunnel not working
>> - we shouldn't do these kind of tests in dataplane. I think we have to
>>    suppose that the data passed to the PMD is valid.
>>
>> I'll redo the test with ipip tomorrow with this fix and let you
>> know the result. If it works, I'll add this in the next version
>> of the patch.
>
> While you are on this, can I suggest you'll add debug logging for TCD and TDD we are writing to the TX ring?
> Something like that:
>
> +                       PMD_TX_LOG(DEBUG, "mbuf: %p, TCD[%u]:\n"
> +                               "tunneling_params: %#x;\n"
> +                               "l2tag2: %#hx;\n"
> +                               "rsvd: %#hx;\n"
> +                               "type_cmd_tso_mss: %#lx;\n",
> +                               tx_pkt, tx_id,
> +                               ctx_txd->tunneling_params,
> +                               ctx_txd->l2tag2,
> +                               ctx_txd->rsvd,
> +                               ctx_txd->type_cmd_tso_mss);
>
> And same for TDD.
> It  helped me a lot to figure out what is going on, when I did my tests.
> Probably would be useful for other people too.

Sure, I'll add this.

Also, just to let you know that I tested the ipip case without the
"if (l2_len) return" and "if (l3_len) return", and it is working.

Regards,
Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Ananyev, Konstantin]

Hi Olivier,

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, January 27, 2015 8:34 AM
> To: Ananyev, Konstantin; Liu, Jijiang; Zhang, Helin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine
> 
> Hi Konstantin,
> 
> On 01/26/2015 03:15 PM, Ananyev, Konstantin wrote:
> >>>> Another thing - IPIP seems to work ok by HW.
> >>>> There is something wrong on our (PMD/test-pmd) side.
> >>>> I think at least we have to remove the following check:
> >>>> if (!l2_len) {
> >>>>                  PMD_DRV_LOG(DEBUG, "L2 length set to 0");
> >>>>                  return;
> >>>>          }
> >>>> in i40e_txd_enable_checksum().
> >>>
> >>> Yes, for IPIP, the check should be removed.
> >>
> >> Yes, I think these lines should be removed for 2 reasons:
> >> - it may be the cause of ipip tunnel not working
> >> - we shouldn't do these kind of tests in dataplane. I think we have to
> >>    suppose that the data passed to the PMD is valid.
> >>
> >> I'll redo the test with ipip tomorrow with this fix and let you
> >> know the result. If it works, I'll add this in the next version
> >> of the patch.
> >
> > While you are on this, can I suggest you'll add debug logging for TCD and TDD we are writing to the TX ring?
> > Something like that:
> >
> > +                       PMD_TX_LOG(DEBUG, "mbuf: %p, TCD[%u]:\n"
> > +                               "tunneling_params: %#x;\n"
> > +                               "l2tag2: %#hx;\n"
> > +                               "rsvd: %#hx;\n"
> > +                               "type_cmd_tso_mss: %#lx;\n",
> > +                               tx_pkt, tx_id,
> > +                               ctx_txd->tunneling_params,
> > +                               ctx_txd->l2tag2,
> > +                               ctx_txd->rsvd,
> > +                               ctx_txd->type_cmd_tso_mss);
> >
> > And same for TDD.
> > It  helped me a lot to figure out what is going on, when I did my tests.
> > Probably would be useful for other people too.
> 
> Sure, I'll add this.
> 
> Also, just to let you know that I tested the ipip case without the
> "if (l2_len) return" and "if (l3_len) return", and it is working.

That's great.
Thanks
Konstantin

> 
> Regards,
> Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Stephen Hemminger]

On Wed, 21 Jan 2015 03:12:35 +0000
"Liu, Jijiang" <jijiang.liu@intel.com> wrote:

> > Because the dpdk looks very similar to that part of linux driver.  
> 
> A  guy from Intel  who have already confirmed that the NVGRE is not supported yet in Linux kernel.
>   
> He said "So far as I know it is not yet supported and I have no information on when it will be."

The existing GRETAP support is sufficient to support NVGRE. No new work is needed. 

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Liu, Jijiang]

Hi,

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Thursday, January 22, 2015 3:45 AM
> To: Liu, Jijiang
> Cc: Olivier MATZ; Ananyev, Konstantin; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and
> csum forwarding engine
> 
> On Wed, 21 Jan 2015 03:12:35 +0000
> "Liu, Jijiang" <jijiang.liu@intel.com> wrote:
> 
> > > Because the dpdk looks very similar to that part of linux driver.
> >
> > A  guy from Intel  who have already confirmed that the NVGRE is not
> supported yet in Linux kernel.
> >
> > He said "So far as I know it is not yet supported and I have no information
> on when it will be."
> 
> The existing GRETAP support is sufficient to support NVGRE. No new work is
> needed.

Sorry, I meant i40e NVGRE feature. 

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Liu, Jijiang]

Hi,

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Wednesday, January 21, 2015 2:16 AM
> To: Ananyev, Konstantin; Liu, Jijiang
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and
> csum forwarding engine
> 
> Hi Konstantin,
> >
> >> Case 4) and case 9) would fill the hardware registers exactly the same.
> >
> > No, they wouldn't.
> > Please read corresponding section of FVL spec and i40e_rxtx.c For case
> > 4) we only need to setup TDD (TX data descriptor) with the following
> values:
> > IIPT, IPLEN, L4T, L4LEN
> > For case 9) we need to setup both TDD and TCD (TX context descriptor)
> with the following values:
> > TDD: IIPT, IPLEN, L4T, L4LEN
> > TCD: EIPT, EIPLEN,  L4TUNT, L4TUNLEN
> >
> >> To me, it's just an API question.
> >
> > No, it is not.
> >
> > I still don't understand why you are so eager to 'forbid' it.
> > Yes we support it for FVL, but no one forces you to use it.
> 
> Well, how would you describe this 2 ways of doing the same thing in the
> offload API? Would you talk about the i40e registers? It's not because i40e
> has 2 ways to do the same operation that the DPDK should do the same.
> 
> How will you explain to a user how to choose between these 2 cases?

Talk about B method in http://dpdk.org/ml/archives/dev/2014-December/009213.html again.

DPDK Never supports a  NIC that can recognize tunneling packet for TX side before 1.8, right?
So when we need to support TX checksum offload for tunneling packet,  and we have to choose B.2.

After introducing i40e(FVL),  FVL is able to recognize tunneling packet and  support outer IP, or inner IP or outer IP and inner IP TX checksum for tunneling packet.
And you agree on "outer and inner at the same time", why do you object "only inner"? 

Actually, B.2 method is a software workaround using L2 length when NIC can't recognize tunneling packet.
When NIC is able to recognize tunneling packet, I think you shouldn't take B.2 as a standard to 'forbid' other method.


> Having to support these 2 different cases for the same thing will complexify
> all future drivers that will not work the same way than i40e.
> 
> >>> As one of possible use-cases:  HW VLAN tags insertion for both inner and
> outer packets.
> >>> FVL can do that, though as I know our PMD doesn't implement it yet.
> >>> For that, we'll need to specify at least:
> >>> outer_l2_len, outer_l3_len, l2_len.
> >>> While PKT_TX_OUTER_* might stay cleared.
> >>
> >> If a VLAN flag has to be inserted in outer header, a new flag
> >> PKT_TX_OUTER_INSERT_VLAN would be added. So my specification
> would
> >> still be correct:
> >>
> >>     The driver should look at mb->outer_lX_len only if a
> >>     PKT_TX_OUTER_* flag is present.
> >>
> >
> > Introducing PKT_TX_OUTER_INSERT_VLAN is ok.
> > Though I still think we'll need TX_*_TUNNEL flags and no need to 'forbid'
> case 9).
> > BTW, as I can see linux i40e driver for tunnelling packets uses case 9), not
> case 4), right?
> 
> I need to check this.
> 
> Regards,
> Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Olivier MATZ]

Hi Jijiang,

On 01/21/2015 09:01 AM, Liu, Jijiang wrote:
>>> I still don't understand why you are so eager to 'forbid' it.
>>> Yes we support it for FVL, but no one forces you to use it.
>>
>> Well, how would you describe this 2 ways of doing the same thing in the
>> offload API? Would you talk about the i40e registers? It's not because i40e
>> has 2 ways to do the same operation that the DPDK should do the same.
>>
>> How will you explain to a user how to choose between these 2 cases?
>
> Talk about B method in http://dpdk.org/ml/archives/dev/2014-December/009213.html again.
>
> DPDK Never supports a  NIC that can recognize tunneling packet for TX side before 1.8, right?

When you say "recognize tunnel", if you mean offlading checksum of
tunnel headers, I agree.

If you mean recognizing a tunnel packet in rx, I also agree
it's new to dpdk-1.8, but I think it's unrelated to what we
are talking about, which is tx checksum. A DPDK application
is able to generate tunnel packets by itself and offload the
checksums to the NIC.

> So when we need to support TX checksum offload for tunneling packet,  and we have to choose B.2.

I don't see why we should choose either B.1 or B.2 (I guess you want
to say B.1 here, right?).

The m->lX_len are not filled in rx today. If one day they are, it won't
prevent the application to configure the lX_len fields and offload
flags according to the API.

> After introducing i40e(FVL),  FVL is able to recognize tunneling packet and  support outer IP, or inner IP or outer IP and inner IP TX checksum for tunneling packet.
> And you agree on "outer and inner at the same time", why do you object "only inner"?
>
> Actually, B.2 method is a software workaround using L2 length when NIC can't recognize tunneling packet.
> When NIC is able to recognize tunneling packet, I think you shouldn't take B.2 as a standard to 'forbid' other method.

Again, I'm not sure there is a link between "recognizing tunneling
packets" and tx checksum offload of tunnels.

Regards,
Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Ananyev, Konstantin]

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Wednesday, January 21, 2015 9:11 AM
> To: Liu, Jijiang
> Cc: Ananyev, Konstantin; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine
> 
> Hi Jijiang,
> 
> On 01/21/2015 09:01 AM, Liu, Jijiang wrote:
> >>> I still don't understand why you are so eager to 'forbid' it.
> >>> Yes we support it for FVL, but no one forces you to use it.
> >>
> >> Well, how would you describe this 2 ways of doing the same thing in the
> >> offload API? Would you talk about the i40e registers? It's not because i40e
> >> has 2 ways to do the same operation that the DPDK should do the same.
> >>
> >> How will you explain to a user how to choose between these 2 cases?
> >
> > Talk about B method in http://dpdk.org/ml/archives/dev/2014-December/009213.html again.
> >
> > DPDK Never supports a  NIC that can recognize tunneling packet for TX side before 1.8, right?
> 
> When you say "recognize tunnel", if you mean offlading checksum of
> tunnel headers, I agree.
> 
> If you mean recognizing a tunnel packet in rx, I also agree
> it's new to dpdk-1.8, but I think it's unrelated to what we
> are talking about, which is tx checksum. A DPDK application
> is able to generate tunnel packets by itself and offload the
> checksums to the NIC.
> 
> > So when we need to support TX checksum offload for tunneling packet,  and we have to choose B.2.
> 
> I don't see why we should choose either B.1 or B.2 (I guess you want
> to say B.1 here, right?).
> 
> The m->lX_len are not filled in rx today. If one day they are, it won't
> prevent the application to configure the lX_len fields and offload
> flags according to the API.
> 
> > After introducing i40e(FVL),  FVL is able to recognize tunneling packet and  support outer IP, or inner IP or outer IP and inner IP TX
> checksum for tunneling packet.
> > And you agree on "outer and inner at the same time", why do you object "only inner"?
> >
> > Actually, B.2 method is a software workaround using L2 length when NIC can't recognize tunneling packet.
> > When NIC is able to recognize tunneling packet, I think you shouldn't take B.2 as a standard to 'forbid' other method.
> 
> Again, I'm not sure there is a link between "recognizing tunneling
> packets" and tx checksum offload of tunnels. 

I think what Jijiang doesn't talk about RX here.
What he is trying to say that with B.2 (case 4) we hide from HW the fact that the packet is tunnelling.
We just using the fact, that to calculate cksum for inner packet only, HW doesn't need to know is it a tunnelling packet or not.
All it needs is a proper values of l2_len and l3_len. 

> 
> Regards,
> Olivier

Re: [dpdk-dev] [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

[Qiu, Michael]

On 12/10/2014 9:04 AM, Jijiang Liu wrote:
> In the current codes, the "tx_checksum set (ip|udp|tcp|sctp|vxlan) (hw|sw) (port-id)" command is not easy to understand and extend, so the patch set enhances the tx_checksum command and reworks csum forwarding engine due to the change of tx_checksum command. 
> The main changes of the tx_checksum command are listed below,
>
> 1> add "tx_checksum set tunnel (hw|sw|none) (port-id)" command
>
> The command is used to set/clear tunnel flag that is used to tell the NIC that the packetg is a tunneing packet and application want hardware TX checksum offload for outer layer, or inner layer, or both.
>
> The 'none' option means that user treat tunneling packet as ordinary packet when using the csum forward engine.
> for example, let say we have a tunnel packet: eth_hdr_out/ipv4_hdr_out/udp_hdr_out/vxlan_hdr/ehtr_hdr_in/ipv4_hdr_in/tcp_hdr_in. one of several scenarios:
>
> 1) User requests HW offload for ipv4_hdr_out  checksum, and doesn't care is it a tunnelled packet or not. So he sets:
>
> tx_checksum set tunnel none 0
>
> tx_checksum set ip hw 0

Hi Jijiang,

I have one question, you know lots of command need port-id field like here, why we do not put port-id just after the command? like below:

tx_checksum (port-id) set tunnel (hw|sw|none)

Then for users, if we do not care whether it is a tunneling packet, we just ignore the field after port-id.

tx_checksum (port-id)

For code it maybe simpler to praise command, and better for user.

What all I mean is, we can put the required parameters just close the command and put the optional parameters(or can be optional) at the end of the command line.

	(Command)  (required parameter) (optional parameters)

Thus, it would be a better user experience.

But just personal idea.

Thanks,

Michael

>
> So for such case we should set tx_tunnel to 'none'.
>
> 2> add "tx_checksum set outer-ip (hw|sw) (port-id)" command
>
> The command is used to set/clear outer IP flag that is used to tell the NIC that application want hardware offload for outer layer.
>
> 3> remove the 'vxlan' option from the  "tx_checksum set (ip|udp|tcp|sctp|vxlan) (hw|sw) (port-id)" command
>
> The command is used to set IP, UDP, TCP and SCTP TX checksum flag. In the case of tunneling packet, the IP, UDP, TCP and SCTP flags always concern inner layer.
>  
> Moreover, replace the TESTPMD_TX_OFFLOAD_VXLAN_CKSUM flag with TESTPMD_TX_OFFLOAD_TUNNEL_CKSUM flag and add the TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM and TESTPMD_TX_OFFLOAD_NON_TUNNEL_CKSUM flag in test-pmd application.
>
> v2 change:
>      redefine the 'none' behaviour for "tx_checksum set tunnel (hw|sw|none) (port-id)" command.
> v3 change:
>      typo correction in cmdline help 
>
> Jijiang Liu (3):
>   add outer IP offload capability in librte_ether.
>   add outer IP checksum capability in i40e PMD
>   testpmd command lines of the tx_checksum and csum forwarding rework
>
>  app/test-pmd/cmdline.c            |  201 +++++++++++++++++++++++++++++++++++--
>  app/test-pmd/csumonly.c           |   35 ++++---
>  app/test-pmd/testpmd.h            |    6 +-
>  lib/librte_ether/rte_ethdev.h     |    1 +
>  lib/librte_pmd_i40e/i40e_ethdev.c |    3 +-
>  5 files changed, 218 insertions(+), 28 deletions(-)
>