[dpdk-dev] [PATCH v5 0/3] i40e VXLAN TX checksum rework

[dpdk-dev] [PATCH v5 0/3] i40e VXLAN TX checksum rework

[Jijiang Liu]

We have got some feedback about backward compatibility of VXLAN TX checksum offload API with 1G/10G NIC after the i40e VXLAN TX checksum codes were applied, so we have to rework the APIs on i40e, including the changes of mbuf, i40e PMD and csum forward engine.
 
The main changes in mbuf are as follows, in place of removing PKT_TX_VXLAN_CKSUM, we introduce 4 new flags: PKT_TX_OUTER_IP_CKSUM, PKT_TX_OUTER_IPV4, PKT_TX_OUTER_IPV6 and PKT_TX_UDP_TUNNEL_PKT. Replace the inner_l2_len and the inner_l3_len field with the outer_l2_len and outer_l3_len field.
 
Let's use a few examples to demonstrate how to use these new flags and existing flags in rte_mbuf.h
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. There could be several scenarios:
 
A) User requests HW offload for ipv4_hdr_out checksum.
He doesn't care is it a tunnelled packet or not. So he sets:
 
mb->l2_len = eth_hdr_out;
mb->l3_len = ipv4_hdr_out;
mb->ol_flags |= PKT_TX_IPV4_CSUM;
 
B) User is aware that it is a tunnelled 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_out + vxlan_hdr +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 | 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 tunnelled packet, but makes HW to treat it as ordinary (non-tunnelled) 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;
 
i40e PMD will support both B.1 and B.2, but ixgbe/igb/em PMD supports only B.2.
if HW supports both - it will be up to user app which method to choose.
tespmd will support both methods, and it should be configurable by user which approach to use (cmdline parameter).
So the user can try/test both methods and select an appropriate for him.
 
C) User knows that is a tunnelled packet, and wants HW offload for all 3 checksums:  outer IP hdr checksum, inner IP checksum, inner TCP checksum.
Then he has to setup all TX checksum fields:
     mb->l2_len =  udp_hdr_out + vxlan_hdr +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_OUT_IP_CKSUM  | PKT_TX_UDP_TUNNEL_PKT | PKT_TX_IP_CKSUM |  PKT_TX_TCP_CKSUM;

Change notes:
v2 changes:
     remove PKT_TX_IP_CKSUM alias.
     add PKT_TX_OUT_IP_CKSUM and PKT_TX_OUTER_IPV6 in rte_get_tx_ol_flag_name.
     spliting mbuf changes into two patches.
     fix MACLEN caculation issue in i40e driver
     fix some issues in csumonly.c
     change cover letter.
v3 changes:
     fix MACLEN caculation issue in i40e driver when non-tunneling packet 
v4 changes:
     reorganize patches to avoid compilation to be broken between patches.
     remove l4_tun_len from mbuf structure.
     add PKT_TX_OUTER_IPV4 to indicate no IP checksum offload requirement for tunneling packet.
     change i40e PMD and csum engine due to above changes.

v5 changes:
     according to Konstantin's comments, optimize process_outer_cksums() in order to avoid setting PKT_TX_OUTER_IPV4 flags for the case when user didn't enable TESTPMD_TX_OFFLOAD_VXLAN_CKSUM 
 
Jijiang Liu (3):
  Redefine PKT_TX_IPV4, PKT_TX_IPV6 and PKT_TX_VLAN_PKT;
  Replace PKT_TX_VXLAN_CKSUM with PKT_TX_UDP_TUNNEL_PKT, and add 3 TX flags, which are PKT_TX_OUTER_IP_CKSUM, PKT_TX_OUTER_IPV4 and PKT_TX_OUTER_IPV6,and rework csum forward engine and i40e pmd due to these changes;
  Replace the inner_l2_len and the inner_l3_len field with the outer_l2_len and outer_l3_len field, and rework csum forward engine and i40e pmd due to  these changes;

 app/test-pmd/csumonly.c         |   69 ++++++++++++++++++++++----------------
 lib/librte_mbuf/rte_mbuf.c      |    7 +++-
 lib/librte_mbuf/rte_mbuf.h      |   25 +++++++++----
 lib/librte_pmd_i40e/i40e_rxtx.c |   44 +++++++++++++------------
 4 files changed, 86 insertions(+), 59 deletions(-)

-- 
1.7.7.6

[dpdk-dev] [PATCH v5 1/3] mbuf:redefine three TX ol_flags

[Jijiang Liu]

The reason of redefining the PKT_TX_IPV4 and the PKT_TX_IPV6 is listed below,
It will avoid to send a packet with a bad info:
  - we receive a Ether/IP6/IP4/L4/data packet
  - the driver sets PKT_RX_IPV6_HDR
  - the stack decapsulates IP6
  - the stack sends the packet, it has the PKT_TX_IPV6 flag but it's an IPv4 packet.

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

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 2e5fce5..cbadf8e 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -141,13 +141,13 @@ extern "C" {
 #define PKT_TX_IP_CKSUM      (1ULL << 54) /**< IP cksum of TX pkt. computed by NIC. */
 #define PKT_TX_IPV4_CSUM     PKT_TX_IP_CKSUM /**< Alias of PKT_TX_IP_CKSUM. */
 
-/** Tell the NIC it's an IPv4 packet. Required for L4 checksum offload or TSO. */
-#define PKT_TX_IPV4          PKT_RX_IPV4_HDR
+/** Packet is IPv4 without requiring IP checksum offload. */
+#define PKT_TX_IPV4          (1ULL << 55)
 
-/** Tell the NIC it's an IPv6 packet. Required for L4 checksum offload or TSO. */
-#define PKT_TX_IPV6          PKT_RX_IPV6_HDR
+/** Tell the NIC it's an IPv6 packet.*/
+#define PKT_TX_IPV6          (1ULL << 56)
 
-#define PKT_TX_VLAN_PKT      (1ULL << 55) /**< TX packet is a 802.1q VLAN packet. */
+#define PKT_TX_VLAN_PKT      (1ULL << 57) /**< TX packet is a 802.1q VLAN packet. */
 
 /* Use final bit of flags to indicate a control mbuf */
 #define CTRL_MBUF_FLAG       (1ULL << 63) /**< Mbuf contains control data */
-- 
1.7.7.6

[dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM

[Jijiang Liu]

Replace PKT_TX_VXLAN_CKSUM with PKT_TX_UDP_TUNNEL_PKT in order to indicate a packet is an UDP tunneling packet, and introduce 3 TX offload flags for outer IP TX checksum, which are PKT_TX_OUTER_IP_CKSUM, PKT_TX_OUTER_IPV4 and PKT_TX_OUTER_IPV6 respectively;Rework csum forward engine and i40e PMD due to these changes.

Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
---
 app/test-pmd/csumonly.c         |    9 +++++++--
 lib/librte_mbuf/rte_mbuf.c      |    7 ++++++-
 lib/librte_mbuf/rte_mbuf.h      |   11 ++++++++++-
 lib/librte_pmd_i40e/i40e_rxtx.c |    6 +++---
 4 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index d8c080a..9094967 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -257,7 +257,7 @@ process_outer_cksums(void *outer_l3_hdr, uint16_t outer_ethertype,
 	uint64_t ol_flags = 0;
 
 	if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM)
-		ol_flags |= PKT_TX_VXLAN_CKSUM;
+		ol_flags |= PKT_TX_UDP_TUNNEL_PKT;
 
 	if (outer_ethertype == _htons(ETHER_TYPE_IPv4)) {
 		ipv4_hdr->hdr_checksum = 0;
@@ -470,7 +470,12 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
 				{ PKT_TX_UDP_CKSUM, PKT_TX_L4_MASK },
 				{ PKT_TX_TCP_CKSUM, PKT_TX_L4_MASK },
 				{ PKT_TX_SCTP_CKSUM, PKT_TX_L4_MASK },
-				{ PKT_TX_VXLAN_CKSUM, PKT_TX_VXLAN_CKSUM },
+				{ PKT_TX_UDP_TUNNEL_PKT, PKT_TX_UDP_TUNNEL_PKT },
+				{ PKT_TX_IPV4, PKT_TX_IPV4 },
+				{ PKT_TX_IPV6, PKT_TX_IPV6 },
+				{ PKT_TX_OUTER_IP_CKSUM, PKT_TX_OUTER_IP_CKSUM },
+				{ PKT_TX_OUTER_IPV4, PKT_TX_OUTER_IPV4 },
+				{ PKT_TX_OUTER_IPV6, PKT_TX_OUTER_IPV6 },
 				{ PKT_TX_TCP_SEG, PKT_TX_TCP_SEG },
 			};
 			unsigned j;
diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 87c2963..1b14e02 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -240,8 +240,13 @@ const char *rte_get_tx_ol_flag_name(uint64_t mask)
 	case PKT_TX_SCTP_CKSUM: return "PKT_TX_SCTP_CKSUM";
 	case PKT_TX_UDP_CKSUM: return "PKT_TX_UDP_CKSUM";
 	case PKT_TX_IEEE1588_TMST: return "PKT_TX_IEEE1588_TMST";
-	case PKT_TX_VXLAN_CKSUM: return "PKT_TX_VXLAN_CKSUM";
+	case PKT_TX_UDP_TUNNEL_PKT: return "PKT_TX_UDP_TUNNEL_PKT";
 	case PKT_TX_TCP_SEG: return "PKT_TX_TCP_SEG";
+	case PKT_TX_IPV4: return "PKT_TX_IPV4";
+	case PKT_TX_IPV6: return "PKT_TX_IPV6";
+	case PKT_TX_OUTER_IP_CKSUM: return "PKT_TX_OUTER_IP_CKSUM";
+	case PKT_TX_OUTER_IPV4: return "PKT_TX_OUTER_IPV4";
+	case PKT_TX_OUTER_IPV6: return "PKT_TX_OUTER_IPV6";
 	default: return NULL;
 	}
 }
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index cbadf8e..6eb898f 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -118,7 +118,7 @@ extern "C" {
  */
 #define PKT_TX_TCP_SEG       (1ULL << 49)
 
-#define PKT_TX_VXLAN_CKSUM   (1ULL << 50) /**< TX checksum of VXLAN computed by NIC */
+#define PKT_TX_UDP_TUNNEL_PKT (1ULL << 50) /**< TX packet is an UDP tunneling packet */
 #define PKT_TX_IEEE1588_TMST (1ULL << 51) /**< TX IEEE1588 packet to timestamp. */
 
 /**
@@ -149,6 +149,15 @@ extern "C" {
 
 #define PKT_TX_VLAN_PKT      (1ULL << 57) /**< TX packet is a 802.1q VLAN packet. */
 
+/** Outer IP checksum of TX packet, computed by NIC for tunneling packet */
+#define PKT_TX_OUTER_IP_CKSUM   (1ULL << 58)
+
+/** Packet is outer IPv4 without requiring IP checksum offload for tunneling packet. */
+#define PKT_TX_OUTER_IPV4   (1ULL << 59)
+
+/** Tell the NIC it's an outer IPv6 packet for tunneling packet */
+#define PKT_TX_OUTER_IPV6    (1ULL << 60)
+
 /* Use final bit of flags to indicate a control mbuf */
 #define CTRL_MBUF_FLAG       (1ULL << 63) /**< Mbuf contains control data */
 
diff --git a/lib/librte_pmd_i40e/i40e_rxtx.c b/lib/librte_pmd_i40e/i40e_rxtx.c
index 2d2ef04..078e973 100644
--- a/lib/librte_pmd_i40e/i40e_rxtx.c
+++ b/lib/librte_pmd_i40e/i40e_rxtx.c
@@ -478,7 +478,7 @@ i40e_txd_enable_checksum(uint64_t ol_flags,
 	}
 
 	/* VXLAN packet TX checksum offload */
-	if (unlikely(ol_flags & PKT_TX_VXLAN_CKSUM)) {
+	if (unlikely(ol_flags & PKT_TX_UDP_TUNNEL_PKT)) {
 		uint8_t l4tun_len;
 
 		l4tun_len = ETHER_VXLAN_HLEN + inner_l2_len;
@@ -1158,8 +1158,8 @@ i40e_calc_context_desc(uint64_t flags)
 {
 	uint64_t mask = 0ULL;
 
-	if (flags | PKT_TX_VXLAN_CKSUM)
-		mask |= PKT_TX_VXLAN_CKSUM;
+	if (flags | PKT_TX_UDP_TUNNEL_PKT)
+		mask |= PKT_TX_UDP_TUNNEL_PKT;
 
 #ifdef RTE_LIBRTE_IEEE1588
 	mask |= PKT_TX_IEEE1588_TMST;
-- 
1.7.7.6

[dpdk-dev] [PATCH v5 3/3] mbuf:replace the inner_l2_len and the inner_l3_len fields

[Jijiang Liu]

Replace the inner_l2_len and the inner_l3_len field with the outer_l2_len and outer_l3_len field, and rework csum forward engine and i40e PMD due to  these changes.

Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
---
 app/test-pmd/csumonly.c         |   58 +++++++++++++++++++++------------------
 lib/librte_mbuf/rte_mbuf.h      |    4 +-
 lib/librte_pmd_i40e/i40e_rxtx.c |   38 +++++++++++++------------
 3 files changed, 53 insertions(+), 47 deletions(-)

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index 9094967..33ef377 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -189,11 +189,12 @@ process_inner_cksums(void *l3_hdr, uint16_t ethertype, uint16_t l3_len,
 		} else {
 			if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_IP_CKSUM)
 				ol_flags |= PKT_TX_IP_CKSUM;
-			else
+			else {
 				ipv4_hdr->hdr_checksum =
 					rte_ipv4_cksum(ipv4_hdr);
+				ol_flags |= PKT_TX_IPV4;
+			}
 		}
-		ol_flags |= PKT_TX_IPV4;
 	} else if (ethertype == _htons(ETHER_TYPE_IPv6))
 		ol_flags |= PKT_TX_IPV6;
 	else
@@ -262,22 +263,23 @@ process_outer_cksums(void *outer_l3_hdr, uint16_t outer_ethertype,
 	if (outer_ethertype == _htons(ETHER_TYPE_IPv4)) {
 		ipv4_hdr->hdr_checksum = 0;
 
-		if ((testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM) == 0)
+		if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM)
+			ol_flags |= PKT_TX_OUTER_IP_CKSUM;
+		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_IPV6;
 
 	udp_hdr = (struct udp_hdr *)((char *)outer_l3_hdr + outer_l3_len);
 	/* do not recalculate udp cksum if it was 0 */
 	if (udp_hdr->dgram_cksum != 0) {
 		udp_hdr->dgram_cksum = 0;
-		if ((testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM) == 0) {
-			if (outer_ethertype == _htons(ETHER_TYPE_IPv4))
-				udp_hdr->dgram_cksum =
-					rte_ipv4_udptcp_cksum(ipv4_hdr, udp_hdr);
-			else
-				udp_hdr->dgram_cksum =
-					rte_ipv6_udptcp_cksum(ipv6_hdr, udp_hdr);
-		}
+		if (outer_ethertype == _htons(ETHER_TYPE_IPv4))
+			udp_hdr->dgram_cksum =
+				rte_ipv4_udptcp_cksum(ipv4_hdr, udp_hdr);
+		else
+			udp_hdr->dgram_cksum =
+				rte_ipv6_udptcp_cksum(ipv6_hdr, udp_hdr);
 	}
 
 	return ol_flags;
@@ -303,8 +305,7 @@ process_outer_cksums(void *outer_l3_hdr, uint16_t outer_ethertype,
  * 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 and UDP layer (if packet is
- * recognized as a vxlan packet).
+ * VxLAN flag concerns the outer IP(if packet is recognized as a vxlan packet).
  */
 static void
 pkt_burst_checksum_forward(struct fwd_stream *fs)
@@ -320,7 +321,7 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
 	uint16_t i;
 	uint64_t ol_flags;
 	uint16_t testpmd_ol_flags;
-	uint8_t l4_proto;
+	uint8_t l4_proto, l4_tun_len = 0;
 	uint16_t ethertype = 0, outer_ethertype = 0;
 	uint16_t l2_len = 0, l3_len = 0, l4_len = 0;
 	uint16_t outer_l2_len = 0, outer_l3_len = 0;
@@ -360,6 +361,7 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
 
 		ol_flags = 0;
 		tunnel = 0;
+		l4_tun_len = 0;
 		m = pkts_burst[i];
 
 		/* Update the L3/L4 checksum error packet statistics */
@@ -378,14 +380,16 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
 		if (l4_proto == IPPROTO_UDP) {
 			udp_hdr = (struct udp_hdr *)((char *)l3_hdr + l3_len);
 
+			/* check udp destination port, 4789 is the default
+			 * vxlan port (rfc7348) */
+			if (udp_hdr->dst_port == _htons(4789)) {
+				l4_tun_len = ETHER_VXLAN_HLEN;
+				tunnel = 1;
+
 			/* currently, this flag is set by i40e only if the
 			 * packet is vxlan */
-			if (((m->ol_flags & PKT_RX_TUNNEL_IPV4_HDR) ||
-					(m->ol_flags & PKT_RX_TUNNEL_IPV6_HDR)))
-				tunnel = 1;
-			/* else check udp destination port, 4789 is the default
-			 * vxlan port (rfc7348) */
-			else if (udp_hdr->dst_port == _htons(4789))
+			} else if (m->ol_flags & (PKT_RX_TUNNEL_IPV4_HDR |
+					PKT_RX_TUNNEL_IPV6_HDR))
 				tunnel = 1;
 
 			if (tunnel == 1) {
@@ -432,10 +436,10 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
 
 		if (tunnel == 1) {
 			if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM) {
-				m->l2_len = outer_l2_len;
-				m->l3_len = outer_l3_len;
-				m->inner_l2_len = l2_len;
-				m->inner_l3_len = l3_len;
+				m->outer_l2_len = outer_l2_len;
+				m->outer_l3_len = outer_l3_len;
+				m->l2_len = l4_tun_len + l2_len;
+				m->l3_len = l3_len;
 			}
 			else {
 				/* if we don't do vxlan cksum in hw,
@@ -503,8 +507,8 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
 					m->l2_len, m->l3_len, m->l4_len);
 			if ((tunnel == 1) &&
 				(testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM))
-				printf("tx: m->inner_l2_len=%d m->inner_l3_len=%d\n",
-					m->inner_l2_len, m->inner_l3_len);
+				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)
 				printf("tx: m->tso_segsz=%d\n", m->tso_segsz);
 			printf("tx: flags=");
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 6eb898f..0404261 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -276,8 +276,8 @@ struct rte_mbuf {
 			uint64_t tso_segsz:16; /**< TCP TSO segment size */
 
 			/* fields for TX offloading of tunnels */
-			uint64_t inner_l3_len:9; /**< inner L3 (IP) Hdr Length. */
-			uint64_t inner_l2_len:7; /**< inner L2 (MAC) Hdr Length. */
+			uint64_t outer_l3_len:9; /**< Outer L3 (IP) Hdr Length. */
+			uint64_t outer_l2_len:7; /**< Outer L2 (MAC) Hdr Length. */
 
 			/* uint64_t unused:8; */
 		};
diff --git a/lib/librte_pmd_i40e/i40e_rxtx.c b/lib/librte_pmd_i40e/i40e_rxtx.c
index 078e973..9f0d1eb 100644
--- a/lib/librte_pmd_i40e/i40e_rxtx.c
+++ b/lib/librte_pmd_i40e/i40e_rxtx.c
@@ -462,41 +462,43 @@ i40e_txd_enable_checksum(uint64_t ol_flags,
 			uint32_t *td_offset,
 			uint8_t l2_len,
 			uint16_t l3_len,
-			uint8_t inner_l2_len,
-			uint16_t inner_l3_len,
+			uint8_t outer_l2_len,
+			uint16_t outer_l3_len,
 			uint32_t *cd_tunneling)
 {
 	if (!l2_len) {
 		PMD_DRV_LOG(DEBUG, "L2 length set to 0");
 		return;
 	}
-	*td_offset |= (l2_len >> 1) << I40E_TX_DESC_LENGTH_MACLEN_SHIFT;
 
 	if (!l3_len) {
 		PMD_DRV_LOG(DEBUG, "L3 length set to 0");
 		return;
 	}
 
-	/* VXLAN packet TX checksum offload */
+	/* UDP tunneling packet TX checksum offload */
 	if (unlikely(ol_flags & PKT_TX_UDP_TUNNEL_PKT)) {
-		uint8_t l4tun_len;
 
-		l4tun_len = ETHER_VXLAN_HLEN + inner_l2_len;
+		*td_offset |= (outer_l2_len >> 1)
+				<< I40E_TX_DESC_LENGTH_MACLEN_SHIFT;
 
-		if (ol_flags & PKT_TX_IPV4_CSUM)
+		if (ol_flags & PKT_TX_OUTER_IP_CKSUM)
 			*cd_tunneling |= I40E_TX_CTX_EXT_IP_IPV4;
-		else if (ol_flags & PKT_TX_IPV6)
+		else if (ol_flags & PKT_TX_OUTER_IPV4)
+			*cd_tunneling |= I40E_TX_CTX_EXT_IP_IPV4_NO_CSUM;
+		else if (ol_flags & PKT_TX_OUTER_IPV6)
 			*cd_tunneling |= I40E_TX_CTX_EXT_IP_IPV6;
 
 		/* Now set the ctx descriptor fields */
-		*cd_tunneling |= (l3_len >> 2) <<
+		*cd_tunneling |= (outer_l3_len >> 2) <<
 				I40E_TXD_CTX_QW0_EXT_IPLEN_SHIFT |
 				I40E_TXD_CTX_UDP_TUNNELING |
-				(l4tun_len >> 1) <<
+				(l2_len >> 1) <<
 				I40E_TXD_CTX_QW0_NATLEN_SHIFT;
 
-		l3_len = inner_l3_len;
-	}
+	} else
+		*td_offset |= (l2_len >> 1)
+			<< I40E_TX_DESC_LENGTH_MACLEN_SHIFT;
 
 	/* Enable L3 checksum offloads */
 	if (ol_flags & PKT_TX_IPV4_CSUM) {
@@ -1190,8 +1192,8 @@ i40e_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 	uint64_t ol_flags;
 	uint8_t l2_len;
 	uint16_t l3_len;
-	uint8_t inner_l2_len;
-	uint16_t inner_l3_len;
+	uint8_t outer_l2_len;
+	uint16_t outer_l3_len;
 	uint16_t nb_used;
 	uint16_t nb_ctx;
 	uint16_t tx_last;
@@ -1219,9 +1221,9 @@ i40e_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 
 		ol_flags = tx_pkt->ol_flags;
 		l2_len = tx_pkt->l2_len;
-		inner_l2_len = tx_pkt->inner_l2_len;
 		l3_len = tx_pkt->l3_len;
-		inner_l3_len = tx_pkt->inner_l3_len;
+		outer_l2_len = tx_pkt->outer_l2_len;
+		outer_l3_len = tx_pkt->outer_l3_len;
 
 		/* Calculate the number of context descriptors needed. */
 		nb_ctx = i40e_calc_context_desc(ol_flags);
@@ -1271,8 +1273,8 @@ i40e_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 		/* Enable checksum offloading */
 		cd_tunneling_params = 0;
 		i40e_txd_enable_checksum(ol_flags, &td_cmd, &td_offset,
-						l2_len, l3_len, inner_l2_len,
-						inner_l3_len,
+						l2_len, l3_len, outer_l2_len,
+						outer_l3_len,
 						&cd_tunneling_params);
 
 		if (unlikely(nb_ctx)) {
-- 
1.7.7.6

Re: [dpdk-dev] [PATCH v5 0/3] i40e VXLAN TX checksum rework

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jijiang Liu
> Sent: Tuesday, December 02, 2014 3:06 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v5 0/3] i40e VXLAN TX checksum rework
> 
> We have got some feedback about backward compatibility of VXLAN TX checksum offload API with 1G/10G NIC after the i40e VXLAN
> TX checksum codes were applied, so we have to rework the APIs on i40e, including the changes of mbuf, i40e PMD and csum forward
> engine.
> 
> The main changes in mbuf are as follows, in place of removing PKT_TX_VXLAN_CKSUM, we introduce 4 new flags:
> PKT_TX_OUTER_IP_CKSUM, PKT_TX_OUTER_IPV4, PKT_TX_OUTER_IPV6 and PKT_TX_UDP_TUNNEL_PKT. Replace the inner_l2_len
> and the inner_l3_len field with the outer_l2_len and outer_l3_len field.
> 
> Let's use a few examples to demonstrate how to use these new flags and existing flags in rte_mbuf.h
> 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. There
> could be several scenarios:
> 
> A) User requests HW offload for ipv4_hdr_out checksum.
> He doesn't care is it a tunnelled packet or not. So he sets:
> 
> mb->l2_len = eth_hdr_out;
> mb->l3_len = ipv4_hdr_out;
> mb->ol_flags |= PKT_TX_IPV4_CSUM;
> 
> B) User is aware that it is a tunnelled 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_out + vxlan_hdr +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 | 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 tunnelled packet, but makes HW to treat it as ordinary (non-tunnelled) 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;
> 
> i40e PMD will support both B.1 and B.2, but ixgbe/igb/em PMD supports only B.2.
> if HW supports both - it will be up to user app which method to choose.
> tespmd will support both methods, and it should be configurable by user which approach to use (cmdline parameter).
> So the user can try/test both methods and select an appropriate for him.
> 
> C) User knows that is a tunnelled packet, and wants HW offload for all 3 checksums:  outer IP hdr checksum, inner IP checksum, inner
> TCP checksum.
> Then he has to setup all TX checksum fields:
>      mb->l2_len =  udp_hdr_out + vxlan_hdr +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_OUT_IP_CKSUM  | PKT_TX_UDP_TUNNEL_PKT | PKT_TX_IP_CKSUM |  PKT_TX_TCP_CKSUM;
> 
> Change notes:
> v2 changes:
>      remove PKT_TX_IP_CKSUM alias.
>      add PKT_TX_OUT_IP_CKSUM and PKT_TX_OUTER_IPV6 in rte_get_tx_ol_flag_name.
>      spliting mbuf changes into two patches.
>      fix MACLEN caculation issue in i40e driver
>      fix some issues in csumonly.c
>      change cover letter.
> v3 changes:
>      fix MACLEN caculation issue in i40e driver when non-tunneling packet
> v4 changes:
>      reorganize patches to avoid compilation to be broken between patches.
>      remove l4_tun_len from mbuf structure.
>      add PKT_TX_OUTER_IPV4 to indicate no IP checksum offload requirement for tunneling packet.
>      change i40e PMD and csum engine due to above changes.
> 
> v5 changes:
>      according to Konstantin's comments, optimize process_outer_cksums() in order to avoid setting PKT_TX_OUTER_IPV4 flags for the
> case when user didn't enable TESTPMD_TX_OFFLOAD_VXLAN_CKSUM
> 
> Jijiang Liu (3):
>   Redefine PKT_TX_IPV4, PKT_TX_IPV6 and PKT_TX_VLAN_PKT;
>   Replace PKT_TX_VXLAN_CKSUM with PKT_TX_UDP_TUNNEL_PKT, and add 3 TX flags, which are PKT_TX_OUTER_IP_CKSUM,
> PKT_TX_OUTER_IPV4 and PKT_TX_OUTER_IPV6,and rework csum forward engine and i40e pmd due to these changes;
>   Replace the inner_l2_len and the inner_l3_len field with the outer_l2_len and outer_l3_len field, and rework csum forward engine
> and i40e pmd due to  these changes;
> 
>  app/test-pmd/csumonly.c         |   69 ++++++++++++++++++++++----------------
>  lib/librte_mbuf/rte_mbuf.c      |    7 +++-
>  lib/librte_mbuf/rte_mbuf.h      |   25 +++++++++----
>  lib/librte_pmd_i40e/i40e_rxtx.c |   44 +++++++++++++------------
>  4 files changed, 86 insertions(+), 59 deletions(-)
> 
> --
> 1.7.7.6

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Re: [dpdk-dev] [PATCH v5 1/3] mbuf:redefine three TX ol_flags

[Olivier MATZ]

Hi Jijiang,

On 12/02/2014 04:06 PM, Jijiang Liu wrote:
> The reason of redefining the PKT_TX_IPV4 and the PKT_TX_IPV6 is listed below,
> It will avoid to send a packet with a bad info:
>    - we receive a Ether/IP6/IP4/L4/data packet
>    - the driver sets PKT_RX_IPV6_HDR
>    - the stack decapsulates IP6
>    - the stack sends the packet, it has the PKT_TX_IPV6 flag but it's an IPv4 packet.
>
> Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
> ---
>   lib/librte_mbuf/rte_mbuf.h |   10 +++++-----
>   1 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 2e5fce5..cbadf8e 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -141,13 +141,13 @@ extern "C" {
>   #define PKT_TX_IP_CKSUM      (1ULL << 54) /**< IP cksum of TX pkt. computed by NIC. */
>   #define PKT_TX_IPV4_CSUM     PKT_TX_IP_CKSUM /**< Alias of PKT_TX_IP_CKSUM. */
>
> -/** Tell the NIC it's an IPv4 packet. Required for L4 checksum offload or TSO. */
> -#define PKT_TX_IPV4          PKT_RX_IPV4_HDR
> +/** Packet is IPv4 without requiring IP checksum offload. */
> +#define PKT_TX_IPV4          (1ULL << 55)
>
> -/** Tell the NIC it's an IPv6 packet. Required for L4 checksum offload or TSO. */
> -#define PKT_TX_IPV6          PKT_RX_IPV6_HDR
> +/** Tell the NIC it's an IPv6 packet.*/
> +#define PKT_TX_IPV6          (1ULL << 56)
>
> -#define PKT_TX_VLAN_PKT      (1ULL << 55) /**< TX packet is a 802.1q VLAN packet. */
> +#define PKT_TX_VLAN_PKT      (1ULL << 57) /**< TX packet is a 802.1q VLAN packet. */
>
>   /* Use final bit of flags to indicate a control mbuf */
>   #define CTRL_MBUF_FLAG       (1ULL << 63) /**< Mbuf contains control data */
>

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

Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM

[Olivier MATZ]

Hi Jijiang,

On 12/02/2014 04:06 PM, Jijiang Liu wrote:
> Replace PKT_TX_VXLAN_CKSUM with PKT_TX_UDP_TUNNEL_PKT in order to indicate a packet is an UDP tunneling packet, and introduce 3 TX offload flags for outer IP TX checksum, which are PKT_TX_OUTER_IP_CKSUM, PKT_TX_OUTER_IPV4 and PKT_TX_OUTER_IPV6 respectively;Rework csum forward engine and i40e PMD due to these changes.
>
> Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
> ---
>   app/test-pmd/csumonly.c         |    9 +++++++--
>   lib/librte_mbuf/rte_mbuf.c      |    7 ++++++-
>   lib/librte_mbuf/rte_mbuf.h      |   11 ++++++++++-
>   lib/librte_pmd_i40e/i40e_rxtx.c |    6 +++---
>   4 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> index d8c080a..9094967 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -257,7 +257,7 @@ process_outer_cksums(void *outer_l3_hdr, uint16_t outer_ethertype,
>   	uint64_t ol_flags = 0;
>
>   	if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM)
> -		ol_flags |= PKT_TX_VXLAN_CKSUM;
> +		ol_flags |= PKT_TX_UDP_TUNNEL_PKT;
>
>   	if (outer_ethertype == _htons(ETHER_TYPE_IPv4)) {
>   		ipv4_hdr->hdr_checksum = 0;
> @@ -470,7 +470,12 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
>   				{ PKT_TX_UDP_CKSUM, PKT_TX_L4_MASK },
>   				{ PKT_TX_TCP_CKSUM, PKT_TX_L4_MASK },
>   				{ PKT_TX_SCTP_CKSUM, PKT_TX_L4_MASK },
> -				{ PKT_TX_VXLAN_CKSUM, PKT_TX_VXLAN_CKSUM },
> +				{ PKT_TX_UDP_TUNNEL_PKT, PKT_TX_UDP_TUNNEL_PKT },
> +				{ PKT_TX_IPV4, PKT_TX_IPV4 },
> +				{ PKT_TX_IPV6, PKT_TX_IPV6 },
> +				{ PKT_TX_OUTER_IP_CKSUM, PKT_TX_OUTER_IP_CKSUM },
> +				{ PKT_TX_OUTER_IPV4, PKT_TX_OUTER_IPV4 },
> +				{ PKT_TX_OUTER_IPV6, PKT_TX_OUTER_IPV6 },
>   				{ PKT_TX_TCP_SEG, PKT_TX_TCP_SEG },

I still think having a flag IPV4 + another flag IP_CHECKSUM is not
appropriate. I though Konstantin agreed on other flags, but I may
have misunderstood:

http://dpdk.org/ml/archives/dev/2014-November/009070.html


Olivier

Re: [dpdk-dev] [PATCH v5 3/3] mbuf:replace the inner_l2_len and the inner_l3_len fields

[Olivier MATZ]

Hi Jijiang,

On 12/02/2014 04:06 PM, Jijiang Liu wrote:
> Replace the inner_l2_len and the inner_l3_len field with the outer_l2_len and outer_l3_len field, and rework csum forward engine and i40e PMD due to  these changes.
>
> Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
> ---
>   app/test-pmd/csumonly.c         |   58 +++++++++++++++++++++------------------
>   lib/librte_mbuf/rte_mbuf.h      |    4 +-
>   lib/librte_pmd_i40e/i40e_rxtx.c |   38 +++++++++++++------------
>   3 files changed, 53 insertions(+), 47 deletions(-)
>

One more thing about the arguments of testpmd, thay may be changed
a bit to make it clearer. We may also remove the distinction between
udp/tcp/sctp and just have l4 instead.

We don't need to add a tunnel-type argument in the testpmd
command line, because the application is already able to parse
the packet and can set the proper tunnel flag by itself.

This is the current description of the csumonly forward engine:

  * 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 and UDP layer (if packet is
  * recognized as a vxlan packet).


This would give:

   tx_cksum set (outer-ip|outer-l4|ip|l4) (hw|sw) (port_id)


A/ outer-ip=sw, outer-l4=sw, ip=sw, l4=sw

   Ether / IP / UDP
     IP, UDP checksum reprocessed in sw

   Ether / IP1 / UDP1 / VxLAN / Ether / IP2 / UDP2:
     IP1, UDP1, IP2, UDP2 checksums reprocessed in sw



B/ outer-ip=hw, outer-l4=hw, ip=sw, l4=sw

   Ether / IP / UDP
     IP, UDP checksum reprocessed in sw

   Ether / IP1 / UDP1 / VxLAN / Ether / IP2 / UDP2:
     IP1, UDP1 checksums reprocessed in hw (using l2_len, l3_len)
     IP2, UDP2 checksums reprocessed in sw



C/ outer-ip=hw, outer-l4=hw, ip=hw, l4=hw

   Ether / IP / UDP
     IP, UDP checksum reprocessed in hw (using l2_len, l3_len)

   Ether / IP1 / UDP1 / VxLAN / Ether / IP2 / UDP2:
     IP1 checksums reprocessed in hw (using outer_l2_len, outer_l3_len)
     UDP1 checksum must be 0 in packet (I think it's not supported by hw
        or driver today)
     IP2, UDP2 checksums reprocessed in hw (using l2_len, l3_len)



D/ outer-ip=sw, outer-l4=sw, ip=hw, l4=hw

   Ether / IP / UDP
     IP, UDP checksum reprocessed in hw (using l2_len and l3_len)

   Ether / IP1 / UDP1 / VxLAN / Ether / IP2 / UDP2:
     not supported, we are not able to calculate the outer
     checksum in sw as some inner fields will be filled by the hw


What is your opinion?

Regards,
Olivier

Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM

[Ananyev, Konstantin]

Hi Oliver,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ
> Sent: Wednesday, December 03, 2014 11:41 AM
> To: Liu, Jijiang; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM
> 
> Hi Jijiang,
> 
> On 12/02/2014 04:06 PM, Jijiang Liu wrote:
> > Replace PKT_TX_VXLAN_CKSUM with PKT_TX_UDP_TUNNEL_PKT in order to indicate a packet is an UDP tunneling packet, and
> introduce 3 TX offload flags for outer IP TX checksum, which are PKT_TX_OUTER_IP_CKSUM, PKT_TX_OUTER_IPV4 and
> PKT_TX_OUTER_IPV6 respectively;Rework csum forward engine and i40e PMD due to these changes.
> >
> > Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
> > ---
> >   app/test-pmd/csumonly.c         |    9 +++++++--
> >   lib/librte_mbuf/rte_mbuf.c      |    7 ++++++-
> >   lib/librte_mbuf/rte_mbuf.h      |   11 ++++++++++-
> >   lib/librte_pmd_i40e/i40e_rxtx.c |    6 +++---
> >   4 files changed, 26 insertions(+), 7 deletions(-)
> >
> > diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> > index d8c080a..9094967 100644
> > --- a/app/test-pmd/csumonly.c
> > +++ b/app/test-pmd/csumonly.c
> > @@ -257,7 +257,7 @@ process_outer_cksums(void *outer_l3_hdr, uint16_t outer_ethertype,
> >   	uint64_t ol_flags = 0;
> >
> >   	if (testpmd_ol_flags & TESTPMD_TX_OFFLOAD_VXLAN_CKSUM)
> > -		ol_flags |= PKT_TX_VXLAN_CKSUM;
> > +		ol_flags |= PKT_TX_UDP_TUNNEL_PKT;
> >
> >   	if (outer_ethertype == _htons(ETHER_TYPE_IPv4)) {
> >   		ipv4_hdr->hdr_checksum = 0;
> > @@ -470,7 +470,12 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
> >   				{ PKT_TX_UDP_CKSUM, PKT_TX_L4_MASK },
> >   				{ PKT_TX_TCP_CKSUM, PKT_TX_L4_MASK },
> >   				{ PKT_TX_SCTP_CKSUM, PKT_TX_L4_MASK },
> > -				{ PKT_TX_VXLAN_CKSUM, PKT_TX_VXLAN_CKSUM },
> > +				{ PKT_TX_UDP_TUNNEL_PKT, PKT_TX_UDP_TUNNEL_PKT },
> > +				{ PKT_TX_IPV4, PKT_TX_IPV4 },
> > +				{ PKT_TX_IPV6, PKT_TX_IPV6 },
> > +				{ PKT_TX_OUTER_IP_CKSUM, PKT_TX_OUTER_IP_CKSUM },
> > +				{ PKT_TX_OUTER_IPV4, PKT_TX_OUTER_IPV4 },
> > +				{ PKT_TX_OUTER_IPV6, PKT_TX_OUTER_IPV6 },
> >   				{ PKT_TX_TCP_SEG, PKT_TX_TCP_SEG },
> 
> I still think having a flag IPV4 + another flag IP_CHECKSUM is not
> appropriate.

Sorry, didn't get you here.
Are you talking about our discussion should PKT_TX_IP_CKSUM and PKT_TX_IPV4 be mutually exclusive or not?

> I though Konstantin agreed on other flags, but I may
> have misunderstood:
> 
> http://dpdk.org/ml/archives/dev/2014-November/009070.html

In that mail, I was talking about my suggestion to make  PKT_TX_IP_CKSUM, PKT_TX_IPV4 and PKT_TX_IPV6 to occupy 2 bits.
Something like:
#define	PKT_TX_IP_CKSUM	(1 << X)
#define	PKT_TX_IPV6		(2 << X)
#define 	PKT_TX_IPV4		(3 << X)

"Even better, if we can squeeze these 3 flags into 2 bits.
Would save us 2 bits, plus might be handy, as in the PMD you can do:

switch (ol_flags & TX_L3_MASK) {
    case TX_IPV4:
       ...
       break;
    case TX_IPV6:
       ...
       break;
    case TX_IP_CKSUM:
       ...
       break;
}"

As you pointed out, it will break backward compatibility.
I agreed with that and self-NACKed it.

> 
> 
> Olivier

Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM

[Olivier MATZ]

Hi Konstantin,

On 12/03/2014 01:59 PM, Ananyev, Konstantin wrote:
>> I still think having a flag IPV4 + another flag IP_CHECKSUM is not
>> appropriate.
>
> Sorry, didn't get you here.
> Are you talking about our discussion should PKT_TX_IP_CKSUM and PKT_TX_IPV4 be mutually exclusive or not?

Yes

>> I though Konstantin agreed on other flags, but I may
>> have misunderstood:
>>
>> http://dpdk.org/ml/archives/dev/2014-November/009070.html
>
> In that mail, I was talking about my suggestion to make  PKT_TX_IP_CKSUM, PKT_TX_IPV4 and PKT_TX_IPV6 to occupy 2 bits.
> Something like:
> #define	PKT_TX_IP_CKSUM	(1 << X)
> #define	PKT_TX_IPV6		(2 << X)
> #define 	PKT_TX_IPV4		(3 << X)
>
> "Even better, if we can squeeze these 3 flags into 2 bits.
> Would save us 2 bits, plus might be handy, as in the PMD you can do:
>
> switch (ol_flags & TX_L3_MASK) {
>      case TX_IPV4:
>         ...
>         break;
>      case TX_IPV6:
>         ...
>         break;
>      case TX_IP_CKSUM:
>         ...
>         break;
> }"
>
> As you pointed out, it will break backward compatibility.
> I agreed with that and self-NACKed it.

ok, so we are back between:

1/ (Jijiang's patch)
PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */
PKT_TX_IPV6      /* packet is IPv6 */
PKT_TX_IPV4      /* packet is IPv4, and we don't want hw cksum */

with PKT_TX_IP_CKSUM and PKT_TX_IPV4 exclusive

and

2/
PKT_TX_IP_CKSUM  /* we want hw IP cksum */
PKT_TX_IPV6      /* packet is IPv6 */
PKT_TX_IPV4      /* packet is IPv4 */

with PKT_TX_IP_CKSUM implies PKT_TX_IPV4


Solution 2/ looks better from a user point of view. Anyone else has an
opinion?

Regards,
Olivier

Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM

[Liu, Jijiang]

Hi Olivier,


> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Wednesday, December 3, 2014 10:42 PM
> To: Ananyev, Konstantin; Liu, Jijiang; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce
> PKT_TX_VXLAN_CKSUM
> 
> Hi Konstantin,
> 
> On 12/03/2014 01:59 PM, Ananyev, Konstantin wrote:
> >> I still think having a flag IPV4 + another flag IP_CHECKSUM is not
> >> appropriate.
> >
> > Sorry, didn't get you here.
> > Are you talking about our discussion should PKT_TX_IP_CKSUM and
> PKT_TX_IPV4 be mutually exclusive or not?
> 
> Yes
> 
> >> I though Konstantin agreed on other flags, but I may have
> >> misunderstood:
> >>
> >> http://dpdk.org/ml/archives/dev/2014-November/009070.html
> >
> > In that mail, I was talking about my suggestion to make  PKT_TX_IP_CKSUM,
> PKT_TX_IPV4 and PKT_TX_IPV6 to occupy 2 bits.
> > Something like:
> > #define	PKT_TX_IP_CKSUM	(1 << X)
> > #define	PKT_TX_IPV6		(2 << X)
> > #define 	PKT_TX_IPV4		(3 << X)
> >
> > "Even better, if we can squeeze these 3 flags into 2 bits.
> > Would save us 2 bits, plus might be handy, as in the PMD you can do:
> >
> > switch (ol_flags & TX_L3_MASK) {
> >      case TX_IPV4:
> >         ...
> >         break;
> >      case TX_IPV6:
> >         ...
> >         break;
> >      case TX_IP_CKSUM:
> >         ...
> >         break;
> > }"
> >
> > As you pointed out, it will break backward compatibility.
> > I agreed with that and self-NACKed it.
> 
> ok, so we are back between:
> 
> 1/ (Jijiang's patch)
> PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */
> PKT_TX_IPV6      /* packet is IPv6 */
> PKT_TX_IPV4      /* packet is IPv4, and we don't want hw cksum */
> 
> with PKT_TX_IP_CKSUM and PKT_TX_IPV4 exclusive
> 
> and
> 
> 2/
> PKT_TX_IP_CKSUM  /* we want hw IP cksum */
> PKT_TX_IPV6      /* packet is IPv6 */
> PKT_TX_IPV4      /* packet is IPv4 */
> 
> with PKT_TX_IP_CKSUM implies PKT_TX_IPV4
> 
> 
> Solution 2/ looks better from a user point of view. Anyone else has an opinion?

Let's think about these IPv4/6 flags in terms of checksum and IP version/type,
 
1. For IPv6 
IP checksum is meaningful only for IPv4,  so we define 'PKT_TX_IPV6      /* packet is IPv6 */' to tell driver/HW that this is IPV6 packet, here we don't talk about the checksum for IPv6 as it is meaningless. Right?

PKT_TX_IPV6      /* packet is IPv6 */         ------ IP type: v6;  HW checksum: meaningless

2. For IPv4,
My patch:

PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */--------------------------IP type: v4;  HW checksum: Yes
PKT_TX_IPV4      /* packet is IPv4, and we don't want hw cksum */ ----------------------- IP type: v4;  HW checksum: No

You want:
PKT_TX_IP_CKSUM  /* we want hw IP cksum */-------------------------- IP type: v4;  HW checksum: Yes
PKT_TX_IPV4      /* packet is IPv4*/ ------------------------  IP type: v4; HW checksum: yes or no?
                                                                                                       driver/HW don't know, just know this is packet with IPv4 header. 
                                                                                                       HW checksum: meaningless??

> Regards,
> Olivier

Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM

[Zhang, Helin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ
> Sent: Wednesday, December 3, 2014 10:42 PM
> To: Ananyev, Konstantin; Liu, Jijiang; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce
> PKT_TX_VXLAN_CKSUM
> 
> Hi Konstantin,
> 
> On 12/03/2014 01:59 PM, Ananyev, Konstantin wrote:
> >> I still think having a flag IPV4 + another flag IP_CHECKSUM is not
> >> appropriate.
> >
> > Sorry, didn't get you here.
> > Are you talking about our discussion should PKT_TX_IP_CKSUM and
> PKT_TX_IPV4 be mutually exclusive or not?
> 
> Yes
> 
> >> I though Konstantin agreed on other flags, but I may have
> >> misunderstood:
> >>
> >> http://dpdk.org/ml/archives/dev/2014-November/009070.html
> >
> > In that mail, I was talking about my suggestion to make  PKT_TX_IP_CKSUM,
> PKT_TX_IPV4 and PKT_TX_IPV6 to occupy 2 bits.
> > Something like:
> > #define	PKT_TX_IP_CKSUM	(1 << X)
> > #define	PKT_TX_IPV6		(2 << X)
> > #define 	PKT_TX_IPV4		(3 << X)
> >
> > "Even better, if we can squeeze these 3 flags into 2 bits.
> > Would save us 2 bits, plus might be handy, as in the PMD you can do:
> >
> > switch (ol_flags & TX_L3_MASK) {
> >      case TX_IPV4:
> >         ...
> >         break;
> >      case TX_IPV6:
> >         ...
> >         break;
> >      case TX_IP_CKSUM:
> >         ...
> >         break;
> > }"
> >
> > As you pointed out, it will break backward compatibility.
> > I agreed with that and self-NACKed it.
> 
> ok, so we are back between:
> 
> 1/ (Jijiang's patch)
> PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */
> PKT_TX_IPV6      /* packet is IPv6 */
> PKT_TX_IPV4      /* packet is IPv4, and we don't want hw cksum */
> 
> with PKT_TX_IP_CKSUM and PKT_TX_IPV4 exclusive
> 
> and
> 
> 2/
> PKT_TX_IP_CKSUM  /* we want hw IP cksum */
> PKT_TX_IPV6      /* packet is IPv6 */
> PKT_TX_IPV4      /* packet is IPv4 */
There is another bit flag named 'PKT_TX_IPV4_CSUM' which uses the
same bit of 'PKT_TX_IP_CSUM'. It is for identifying if ipv4 hardware
checksum offload is needed or not.
It seems that we do not need 'PKT_TX_IPV6_CSUM'.
'PKT_TX_IPV4' and 'PKT_TX_IPV6' just indicates its packet type, and I guess
other features should not be contained in it, according to its name.

So here I got the option 3:
PKT_TX_IPV4_CKSUM  /* we want hw IPv4 cksum */
PKT_TX_IPV6      /* packet is IPv6 */
PKT_TX_IPV4      /* packet is IPv4 */

> 
> with PKT_TX_IP_CKSUM implies PKT_TX_IPV4
> 
> 
> Solution 2/ looks better from a user point of view. Anyone else has an opinion?
> 
> Regards,
> Olivier

Regards,
Helin

Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM

[Liu, Jijiang]

Hi Helin,

> -----Original Message-----
> From: Zhang, Helin
> Sent: Thursday, December 4, 2014 2:52 PM
> To: Olivier MATZ; Ananyev, Konstantin; Liu, Jijiang; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce
> PKT_TX_VXLAN_CKSUM
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ
> > Sent: Wednesday, December 3, 2014 10:42 PM
> > To: Ananyev, Konstantin; Liu, Jijiang; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and
> > repalce PKT_TX_VXLAN_CKSUM
> >
> > Hi Konstantin,
> >
> > On 12/03/2014 01:59 PM, Ananyev, Konstantin wrote:
> > >> I still think having a flag IPV4 + another flag IP_CHECKSUM is not
> > >> appropriate.
> > >
> > > Sorry, didn't get you here.
> > > Are you talking about our discussion should PKT_TX_IP_CKSUM and
> > PKT_TX_IPV4 be mutually exclusive or not?
> >
> > Yes
> >
> > >> I though Konstantin agreed on other flags, but I may have
> > >> misunderstood:
> > >>
> > >> http://dpdk.org/ml/archives/dev/2014-November/009070.html
> > >
> > > In that mail, I was talking about my suggestion to make
> > > PKT_TX_IP_CKSUM,
> > PKT_TX_IPV4 and PKT_TX_IPV6 to occupy 2 bits.
> > > Something like:
> > > #define	PKT_TX_IP_CKSUM	(1 << X)
> > > #define	PKT_TX_IPV6		(2 << X)
> > > #define 	PKT_TX_IPV4		(3 << X)
> > >
> > > "Even better, if we can squeeze these 3 flags into 2 bits.
> > > Would save us 2 bits, plus might be handy, as in the PMD you can do:
> > >
> > > switch (ol_flags & TX_L3_MASK) {
> > >      case TX_IPV4:
> > >         ...
> > >         break;
> > >      case TX_IPV6:
> > >         ...
> > >         break;
> > >      case TX_IP_CKSUM:
> > >         ...
> > >         break;
> > > }"
> > >
> > > As you pointed out, it will break backward compatibility.
> > > I agreed with that and self-NACKed it.
> >
> > ok, so we are back between:
> >
> > 1/ (Jijiang's patch)
> > PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */
> > PKT_TX_IPV6      /* packet is IPv6 */
> > PKT_TX_IPV4      /* packet is IPv4, and we don't want hw cksum */
> >
> > with PKT_TX_IP_CKSUM and PKT_TX_IPV4 exclusive
> >
> > and
> >
> > 2/
> > PKT_TX_IP_CKSUM  /* we want hw IP cksum */
> > PKT_TX_IPV6      /* packet is IPv6 */
> > PKT_TX_IPV4      /* packet is IPv4 */
> There is another bit flag named 'PKT_TX_IPV4_CSUM' which uses the same bit of
> 'PKT_TX_IP_CSUM'. It is for identifying if ipv4 hardware checksum offload is
> needed or not.
> It seems that we do not need 'PKT_TX_IPV6_CSUM'.
> 'PKT_TX_IPV4' and 'PKT_TX_IPV6' just indicates its packet type, and I guess other
> features should not be contained in it, according to its name.
> 
> So here I got the option 3:
> PKT_TX_IPV4_CKSUM  /* we want hw IPv4 cksum */
> PKT_TX_IPV6      /* packet is IPv6 */
> PKT_TX_IPV4      /* packet is IPv4 */

In TX side,  if just tell driver/HW this is a IPV4 packet , and don't tell  driver/HW whether TX checksum  or other offload  is required or not , the flag is senseless,  and hardware will not do any offload.

The flag is not used in igb/ixgbe codes, which is just used in i40e codes, and set this bit(IPv4 packet with no IP checksum offload) in i40e driver.
...
	} else if (ol_flags & PKT_TX_IPV4) {
		*td_cmd |= I40E_TX_DESC_CMD_IIPT_IPV4;
...
		

> >
> > with PKT_TX_IP_CKSUM implies PKT_TX_IPV4
> >
> >
> > Solution 2/ looks better from a user point of view. Anyone else has an opinion?
> >
> > Regards,
> > Olivier
> 
> Regards,
> Helin

Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM

[Ananyev, Konstantin]

Hi Helin,

> -----Original Message-----
> From: Zhang, Helin
> Sent: Thursday, December 04, 2014 6:52 AM
> To: Olivier MATZ; Ananyev, Konstantin; Liu, Jijiang; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ
> > Sent: Wednesday, December 3, 2014 10:42 PM
> > To: Ananyev, Konstantin; Liu, Jijiang; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce
> > PKT_TX_VXLAN_CKSUM
> >
> > Hi Konstantin,
> >
> > On 12/03/2014 01:59 PM, Ananyev, Konstantin wrote:
> > >> I still think having a flag IPV4 + another flag IP_CHECKSUM is not
> > >> appropriate.
> > >
> > > Sorry, didn't get you here.
> > > Are you talking about our discussion should PKT_TX_IP_CKSUM and
> > PKT_TX_IPV4 be mutually exclusive or not?
> >
> > Yes
> >
> > >> I though Konstantin agreed on other flags, but I may have
> > >> misunderstood:
> > >>
> > >> http://dpdk.org/ml/archives/dev/2014-November/009070.html
> > >
> > > In that mail, I was talking about my suggestion to make  PKT_TX_IP_CKSUM,
> > PKT_TX_IPV4 and PKT_TX_IPV6 to occupy 2 bits.
> > > Something like:
> > > #define	PKT_TX_IP_CKSUM	(1 << X)
> > > #define	PKT_TX_IPV6		(2 << X)
> > > #define 	PKT_TX_IPV4		(3 << X)
> > >
> > > "Even better, if we can squeeze these 3 flags into 2 bits.
> > > Would save us 2 bits, plus might be handy, as in the PMD you can do:
> > >
> > > switch (ol_flags & TX_L3_MASK) {
> > >      case TX_IPV4:
> > >         ...
> > >         break;
> > >      case TX_IPV6:
> > >         ...
> > >         break;
> > >      case TX_IP_CKSUM:
> > >         ...
> > >         break;
> > > }"
> > >
> > > As you pointed out, it will break backward compatibility.
> > > I agreed with that and self-NACKed it.
> >
> > ok, so we are back between:
> >
> > 1/ (Jijiang's patch)
> > PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */
> > PKT_TX_IPV6      /* packet is IPv6 */
> > PKT_TX_IPV4      /* packet is IPv4, and we don't want hw cksum */
> >
> > with PKT_TX_IP_CKSUM and PKT_TX_IPV4 exclusive
> >
> > and
> >
> > 2/
> > PKT_TX_IP_CKSUM  /* we want hw IP cksum */
> > PKT_TX_IPV6      /* packet is IPv6 */
> > PKT_TX_IPV4      /* packet is IPv4 */
> There is another bit flag named 'PKT_TX_IPV4_CSUM' which uses the
> same bit of 'PKT_TX_IP_CSUM'. It is for identifying if ipv4 hardware
> checksum offload is needed or not.

Yes, 'PKT_TX_IPV4_CSUM is an alias to PKT_TX_IP_CKSUM  and we are going to remove it.

> It seems that we do not need 'PKT_TX_IPV6_CSUM'.

No one even planned it.

> 'PKT_TX_IPV4' and 'PKT_TX_IPV6' just indicates its packet type, and I guess
> other features should not be contained in it, according to its name.
> 
> So here I got the option 3:
> PKT_TX_IPV4_CKSUM  /* we want hw IPv4 cksum */
> PKT_TX_IPV6      /* packet is IPv6 */
> PKT_TX_IPV4      /* packet is IPv4 */

Hmm, and how this is different from what we have now in the Jijiang's patch?
Except that you renamed PKT_TX_IP_CKSUM to PKT_TX_IPV4_CKSUM?   

Konstantin

> 
> >
> > with PKT_TX_IP_CKSUM implies PKT_TX_IPV4
> >
> >
> > Solution 2/ looks better from a user point of view. Anyone else has an opinion?
> >
> > Regards,
> > Olivier
> 
> Regards,
> Helin

Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM

[Ananyev, Konstantin]

> -----Original Message-----
> From: Liu, Jijiang
> Sent: Thursday, December 04, 2014 2:08 AM
> To: Olivier MATZ
> Cc: Ananyev, Konstantin; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM
> 
> Hi Olivier,
> 
> 
> > -----Original Message-----
> > From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> > Sent: Wednesday, December 3, 2014 10:42 PM
> > To: Ananyev, Konstantin; Liu, Jijiang; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce
> > PKT_TX_VXLAN_CKSUM
> >
> > Hi Konstantin,
> >
> > On 12/03/2014 01:59 PM, Ananyev, Konstantin wrote:
> > >> I still think having a flag IPV4 + another flag IP_CHECKSUM is not
> > >> appropriate.
> > >
> > > Sorry, didn't get you here.
> > > Are you talking about our discussion should PKT_TX_IP_CKSUM and
> > PKT_TX_IPV4 be mutually exclusive or not?
> >
> > Yes
> >
> > >> I though Konstantin agreed on other flags, but I may have
> > >> misunderstood:
> > >>
> > >> http://dpdk.org/ml/archives/dev/2014-November/009070.html
> > >
> > > In that mail, I was talking about my suggestion to make  PKT_TX_IP_CKSUM,
> > PKT_TX_IPV4 and PKT_TX_IPV6 to occupy 2 bits.
> > > Something like:
> > > #define	PKT_TX_IP_CKSUM	(1 << X)
> > > #define	PKT_TX_IPV6		(2 << X)
> > > #define 	PKT_TX_IPV4		(3 << X)
> > >
> > > "Even better, if we can squeeze these 3 flags into 2 bits.
> > > Would save us 2 bits, plus might be handy, as in the PMD you can do:
> > >
> > > switch (ol_flags & TX_L3_MASK) {
> > >      case TX_IPV4:
> > >         ...
> > >         break;
> > >      case TX_IPV6:
> > >         ...
> > >         break;
> > >      case TX_IP_CKSUM:
> > >         ...
> > >         break;
> > > }"
> > >
> > > As you pointed out, it will break backward compatibility.
> > > I agreed with that and self-NACKed it.
> >
> > ok, so we are back between:
> >
> > 1/ (Jijiang's patch)
> > PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */
> > PKT_TX_IPV6      /* packet is IPv6 */
> > PKT_TX_IPV4      /* packet is IPv4, and we don't want hw cksum */
> >
> > with PKT_TX_IP_CKSUM and PKT_TX_IPV4 exclusive
> >
> > and
> >
> > 2/
> > PKT_TX_IP_CKSUM  /* we want hw IP cksum */
> > PKT_TX_IPV6      /* packet is IPv6 */
> > PKT_TX_IPV4      /* packet is IPv4 */
> >
> > with PKT_TX_IP_CKSUM implies PKT_TX_IPV4
> >
> >
> > Solution 2/ looks better from a user point of view. Anyone else has an opinion?
> 
> Let's think about these IPv4/6 flags in terms of checksum and IP version/type,
> 
> 1. For IPv6
> IP checksum is meaningful only for IPv4,  so we define 'PKT_TX_IPV6      /* packet is IPv6 */' to tell driver/HW that this is IPV6 packet,
> here we don't talk about the checksum for IPv6 as it is meaningless. Right?
> 
> PKT_TX_IPV6      /* packet is IPv6 */         ------ IP type: v6;  HW checksum: meaningless
> 
> 2. For IPv4,
> My patch:
> 
> PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */--------------------------IP type: v4;  HW checksum: Yes
> PKT_TX_IPV4      /* packet is IPv4, and we don't want hw cksum */ ----------------------- IP type: v4;  HW checksum: No
> 
> You want:
> PKT_TX_IP_CKSUM  /* we want hw IP cksum */-------------------------- IP type: v4;  HW checksum: Yes
> PKT_TX_IPV4      /* packet is IPv4*/ ------------------------  IP type: v4; HW checksum: yes or no?
>                                                                                                        driver/HW don't know, just know this is packet with IPv4 header.
>                                                                                                        HW checksum: meaningless??

Yep, that's why I also don't like that suggestion: PKT_TX_IPV4 itself doesn't contain all information.
PMD will have to check PKT_TX_IP_CKSUM  anyway.
Konstantin

> 
> > Regards,
> > Olivier

Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM

[Thomas Monjalon]

Hi,

2014-12-04 10:23, Ananyev, Konstantin:
> From: Liu, Jijiang
> > From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> > > On 12/03/2014 01:59 PM, Ananyev, Konstantin wrote:
> > > >> I still think having a flag IPV4 + another flag IP_CHECKSUM is not
> > > >> appropriate.
> > > >
> > > > Sorry, didn't get you here.
> > > > Are you talking about our discussion should PKT_TX_IP_CKSUM and
> > > > PKT_TX_IPV4 be mutually exclusive or not?
> > >
> > > Yes
> > >
> > > >> I though Konstantin agreed on other flags, but I may have
> > > >> misunderstood:
> > > >>
> > > >> http://dpdk.org/ml/archives/dev/2014-November/009070.html
> > > >
> > > > In that mail, I was talking about my suggestion to make  PKT_TX_IP_CKSUM,
> > > PKT_TX_IPV4 and PKT_TX_IPV6 to occupy 2 bits.
> > > > Something like:
> > > > #define	PKT_TX_IP_CKSUM	(1 << X)
> > > > #define	PKT_TX_IPV6		(2 << X)
> > > > #define 	PKT_TX_IPV4		(3 << X)
> > > >
> > > > "Even better, if we can squeeze these 3 flags into 2 bits.
> > > > Would save us 2 bits, plus might be handy, as in the PMD you can do:
> > > >
> > > > switch (ol_flags & TX_L3_MASK) {
> > > >      case TX_IPV4:
> > > >         ...
> > > >         break;
> > > >      case TX_IPV6:
> > > >         ...
> > > >         break;
> > > >      case TX_IP_CKSUM:
> > > >         ...
> > > >         break;
> > > > }"
> > > >
> > > > As you pointed out, it will break backward compatibility.
> > > > I agreed with that and self-NACKed it.
> > >
> > > ok, so we are back between:
> > >
> > > 1/ (Jijiang's patch)
> > > PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */
> > > PKT_TX_IPV6      /* packet is IPv6 */
> > > PKT_TX_IPV4      /* packet is IPv4, and we don't want hw cksum */
> > >
> > > with PKT_TX_IP_CKSUM and PKT_TX_IPV4 exclusive
> > >
> > > and
> > >
> > > 2/
> > > PKT_TX_IP_CKSUM  /* we want hw IP cksum */
> > > PKT_TX_IPV6      /* packet is IPv6 */
> > > PKT_TX_IPV4      /* packet is IPv4 */
> > >
> > > with PKT_TX_IP_CKSUM implies PKT_TX_IPV4
> > >
> > >
> > > Solution 2/ looks better from a user point of view. Anyone else has an opinion?
> > 
> > Let's think about these IPv4/6 flags in terms of checksum and IP version/type,
> > 
> > 1. For IPv6
> > IP checksum is meaningful only for IPv4,  so we define 'PKT_TX_IPV6      /* packet is IPv6 */' to tell driver/HW that this is IPV6 packet,
> > here we don't talk about the checksum for IPv6 as it is meaningless. Right?
> > 
> > PKT_TX_IPV6      /* packet is IPv6 */         ------ IP type: v6;  HW checksum: meaningless
> > 
> > 2. For IPv4,
> > My patch:
> > 
> > PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */--------------------------IP type: v4;  HW checksum: Yes
> > PKT_TX_IPV4      /* packet is IPv4, and we don't want hw cksum */ ----------------------- IP type: v4;  HW checksum: No
> > 
> > You want:
> > PKT_TX_IP_CKSUM  /* we want hw IP cksum */-------------------------- IP type: v4;  HW checksum: Yes
> > PKT_TX_IPV4      /* packet is IPv4*/ ------------------------  IP type: v4; HW checksum: yes or no?
> >                                                                                                        driver/HW don't know, just know this is packet with IPv4 header.
> >                                                                                                        HW checksum: meaningless??
> 
> Yep, that's why I also don't like that suggestion: PKT_TX_IPV4 itself doesn't contain all information.
> PMD will have to check PKT_TX_IP_CKSUM  anyway.

I prefer solution 2 because a flag should bring only 1 information.
It's simply saner and could fit to more situations in the future.

-- 
Thomas

Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM

[Ananyev, Konstantin]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, December 04, 2014 10:45 AM
> To: Ananyev, Konstantin; Olivier MATZ
> Cc: dev@dpdk.org; Liu, Jijiang
> Subject: Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM
> 
> Hi,
> 
> 2014-12-04 10:23, Ananyev, Konstantin:
> > From: Liu, Jijiang
> > > From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> > > > On 12/03/2014 01:59 PM, Ananyev, Konstantin wrote:
> > > > >> I still think having a flag IPV4 + another flag IP_CHECKSUM is not
> > > > >> appropriate.
> > > > >
> > > > > Sorry, didn't get you here.
> > > > > Are you talking about our discussion should PKT_TX_IP_CKSUM and
> > > > > PKT_TX_IPV4 be mutually exclusive or not?
> > > >
> > > > Yes
> > > >
> > > > >> I though Konstantin agreed on other flags, but I may have
> > > > >> misunderstood:
> > > > >>
> > > > >> http://dpdk.org/ml/archives/dev/2014-November/009070.html
> > > > >
> > > > > In that mail, I was talking about my suggestion to make  PKT_TX_IP_CKSUM,
> > > > PKT_TX_IPV4 and PKT_TX_IPV6 to occupy 2 bits.
> > > > > Something like:
> > > > > #define	PKT_TX_IP_CKSUM	(1 << X)
> > > > > #define	PKT_TX_IPV6		(2 << X)
> > > > > #define 	PKT_TX_IPV4		(3 << X)
> > > > >
> > > > > "Even better, if we can squeeze these 3 flags into 2 bits.
> > > > > Would save us 2 bits, plus might be handy, as in the PMD you can do:
> > > > >
> > > > > switch (ol_flags & TX_L3_MASK) {
> > > > >      case TX_IPV4:
> > > > >         ...
> > > > >         break;
> > > > >      case TX_IPV6:
> > > > >         ...
> > > > >         break;
> > > > >      case TX_IP_CKSUM:
> > > > >         ...
> > > > >         break;
> > > > > }"
> > > > >
> > > > > As you pointed out, it will break backward compatibility.
> > > > > I agreed with that and self-NACKed it.
> > > >
> > > > ok, so we are back between:
> > > >
> > > > 1/ (Jijiang's patch)
> > > > PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */
> > > > PKT_TX_IPV6      /* packet is IPv6 */
> > > > PKT_TX_IPV4      /* packet is IPv4, and we don't want hw cksum */
> > > >
> > > > with PKT_TX_IP_CKSUM and PKT_TX_IPV4 exclusive
> > > >
> > > > and
> > > >
> > > > 2/
> > > > PKT_TX_IP_CKSUM  /* we want hw IP cksum */
> > > > PKT_TX_IPV6      /* packet is IPv6 */
> > > > PKT_TX_IPV4      /* packet is IPv4 */
> > > >
> > > > with PKT_TX_IP_CKSUM implies PKT_TX_IPV4
> > > >
> > > >
> > > > Solution 2/ looks better from a user point of view. Anyone else has an opinion?
> > >
> > > Let's think about these IPv4/6 flags in terms of checksum and IP version/type,
> > >
> > > 1. For IPv6
> > > IP checksum is meaningful only for IPv4,  so we define 'PKT_TX_IPV6      /* packet is IPv6 */' to tell driver/HW that this is IPV6
> packet,
> > > here we don't talk about the checksum for IPv6 as it is meaningless. Right?
> > >
> > > PKT_TX_IPV6      /* packet is IPv6 */         ------ IP type: v6;  HW checksum: meaningless
> > >
> > > 2. For IPv4,
> > > My patch:
> > >
> > > PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */--------------------------IP type: v4;  HW checksum: Yes
> > > PKT_TX_IPV4      /* packet is IPv4, and we don't want hw cksum */ ----------------------- IP type: v4;  HW checksum: No
> > >
> > > You want:
> > > PKT_TX_IP_CKSUM  /* we want hw IP cksum */-------------------------- IP type: v4;  HW checksum: Yes
> > > PKT_TX_IPV4      /* packet is IPv4*/ ------------------------  IP type: v4; HW checksum: yes or no?
> > >                                                                                                        driver/HW don't know, just know this is packet with IPv4 header.
> > >                                                                                                        HW checksum: meaningless??
> >
> > Yep, that's why I also don't like that suggestion: PKT_TX_IPV4 itself doesn't contain all information.
> > PMD will have to check PKT_TX_IP_CKSUM  anyway.
> 
> I prefer solution 2 because a flag should bring only 1 information.

Why is that? For example in mbuf we already have a flag that brings 2 things:
PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */

If it would be possible to compress 10 meanings into 1 bit, I would happily do that.
Unfortunately, it is rarely possible :)

> It's simply saner and could fit to more situations in the future.

Could you give an example of such situation?
I personally couldn't come up with the case where #2 would have any real advantage. 
Konstantin

> 
> --
> Thomas

Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM

[Olivier MATZ]

Hi,

On 12/04/2014 11:19 AM, Ananyev, Konstantin wrote:
>>> 1/ (Jijiang's patch)
>>> PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */
>>> PKT_TX_IPV6      /* packet is IPv6 */
>>> PKT_TX_IPV4      /* packet is IPv4, and we don't want hw cksum */
>>>
>>> with PKT_TX_IP_CKSUM and PKT_TX_IPV4 exclusive
>>>
>>> and
>>>
>>> 2/
>>> PKT_TX_IP_CKSUM  /* we want hw IP cksum */
>>> PKT_TX_IPV6      /* packet is IPv6 */
>>> PKT_TX_IPV4      /* packet is IPv4 */
>> There is another bit flag named 'PKT_TX_IPV4_CSUM' which uses the
>> same bit of 'PKT_TX_IP_CSUM'. It is for identifying if ipv4 hardware
>> checksum offload is needed or not.
>
> Yes, 'PKT_TX_IPV4_CSUM is an alias to PKT_TX_IP_CKSUM  and we are going to remove it.
>
>> It seems that we do not need 'PKT_TX_IPV6_CSUM'.
>
> No one even planned it.
>
>> 'PKT_TX_IPV4' and 'PKT_TX_IPV6' just indicates its packet type, and I guess
>> other features should not be contained in it, according to its name.
>>
>> So here I got the option 3:
>> PKT_TX_IPV4_CKSUM  /* we want hw IPv4 cksum */
>> PKT_TX_IPV6      /* packet is IPv6 */
>> PKT_TX_IPV4      /* packet is IPv4 */
>
> Hmm, and how this is different from what we have now in the Jijiang's patch?
> Except that you renamed PKT_TX_IP_CKSUM to PKT_TX_IPV4_CKSUM?

I think it's more like solution 2 with a renaming. And it is more
coherent to always have "IPV4" on all flag names.

Regards,
Olivier

Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM

[Olivier MATZ]

Hi,

On 12/04/2014 12:03 PM, Ananyev, Konstantin wrote:
>>>>> 1/ (Jijiang's patch)
>>>>> PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */
>>>>> PKT_TX_IPV6      /* packet is IPv6 */
>>>>> PKT_TX_IPV4      /* packet is IPv4, and we don't want hw cksum */
>>>>>
>>>>> with PKT_TX_IP_CKSUM and PKT_TX_IPV4 exclusive
>>>>>
>>>>> and
>>>>>
>>>>> 2/
>>>>> PKT_TX_IP_CKSUM  /* we want hw IP cksum */
>>>>> PKT_TX_IPV6      /* packet is IPv6 */
>>>>> PKT_TX_IPV4      /* packet is IPv4 */
>>>>>
>>>>> with PKT_TX_IP_CKSUM implies PKT_TX_IPV4
>>>>>
>>>>>
>>>>> Solution 2/ looks better from a user point of view. Anyone else has an opinion?
>>>>
>>>> Let's think about these IPv4/6 flags in terms of checksum and IP version/type,
>>>>
>>>> 1. For IPv6
>>>> IP checksum is meaningful only for IPv4,  so we define 'PKT_TX_IPV6      /* packet is IPv6 */' to tell driver/HW that this is IPV6
>> packet,
>>>> here we don't talk about the checksum for IPv6 as it is meaningless. Right?
>>>>
>>>> PKT_TX_IPV6      /* packet is IPv6 */         ------ IP type: v6;  HW checksum: meaningless
>>>>
>>>> 2. For IPv4,
>>>> My patch:
>>>>
>>>> PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */--------------------------IP type: v4;  HW checksum: Yes
>>>> PKT_TX_IPV4      /* packet is IPv4, and we don't want hw cksum */ ----------------------- IP type: v4;  HW checksum: No
>>>>
>>>> You want:
>>>> PKT_TX_IP_CKSUM  /* we want hw IP cksum */-------------------------- IP type: v4;  HW checksum: Yes
>>>> PKT_TX_IPV4      /* packet is IPv4*/ ------------------------  IP type: v4; HW checksum: yes or no?
>>>>                                                                                                         driver/HW don't know, just know this is packet with IPv4 header.
>>>>                                                                                                         HW checksum: meaningless??
>>>
>>> Yep, that's why I also don't like that suggestion: PKT_TX_IPV4 itself doesn't contain all information.
>>> PMD will have to check PKT_TX_IP_CKSUM  anyway.
>>
>> I prefer solution 2 because a flag should bring only 1 information.
>
> Why is that? For example in mbuf we already have a flag that brings 2 things:
> PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */

For the user, it's clearer to have one information in a flag.
If you just look at the name of the flag, the natural meaning is 2/,
else we would need to rename them in:
   PKT_TX_IPV4_CKSUM
   PKT_TX_IPV4_NO_CKSUM

> If it would be possible to compress 10 meanings into 1 bit, I would happily do that.
> Unfortunately, it is rarely possible :)
>
>> It's simply saner and could fit to more situations in the future.
>
> Could you give an example of such situation?
> I personally couldn't come up with the case where #2 would have any real advantage.

in solution 2/, PKT_TX_IP_CKSUM implies PKT_TX_IPV4 so checking
PKT_TX_IP_CKSUM is still enough in drivers.

In the driver, it is also simpler. With solution 1/:

/* check if we need ipcsum */
if (flags & PKT_TX_IP_CKSUM)

/* check if packet is ipv4, may be needed to set a hw field */
if (flags & (PKT_TX_IP_CKSUM|PKT_TX_IPV4))


With solution 2/

/* check if we need ipcsum */
if (flags & PKT_TX_IP_CKSUM)

/* check if packet is ipv4, may be needed to set a hw field */
if (flags & PKT_TX_IPV4)


I agree it can looks like a detail, but I really think it's important
to have the most logical and straightforward api for mbuf, as it's
the core of DPDK.

Regards,
Olivier

Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM

[Ananyev, Konstantin]

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Thursday, December 04, 2014 1:48 PM
> To: Ananyev, Konstantin; Zhang, Helin; Liu, Jijiang; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM
> 
> Hi,
> 
> On 12/04/2014 11:19 AM, Ananyev, Konstantin wrote:
> >>> 1/ (Jijiang's patch)
> >>> PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */
> >>> PKT_TX_IPV6      /* packet is IPv6 */
> >>> PKT_TX_IPV4      /* packet is IPv4, and we don't want hw cksum */
> >>>
> >>> with PKT_TX_IP_CKSUM and PKT_TX_IPV4 exclusive
> >>>
> >>> and
> >>>
> >>> 2/
> >>> PKT_TX_IP_CKSUM  /* we want hw IP cksum */
> >>> PKT_TX_IPV6      /* packet is IPv6 */
> >>> PKT_TX_IPV4      /* packet is IPv4 */
> >> There is another bit flag named 'PKT_TX_IPV4_CSUM' which uses the
> >> same bit of 'PKT_TX_IP_CSUM'. It is for identifying if ipv4 hardware
> >> checksum offload is needed or not.
> >
> > Yes, 'PKT_TX_IPV4_CSUM is an alias to PKT_TX_IP_CKSUM  and we are going to remove it.
> >
> >> It seems that we do not need 'PKT_TX_IPV6_CSUM'.
> >
> > No one even planned it.
> >
> >> 'PKT_TX_IPV4' and 'PKT_TX_IPV6' just indicates its packet type, and I guess
> >> other features should not be contained in it, according to its name.
> >>
> >> So here I got the option 3:
> >> PKT_TX_IPV4_CKSUM  /* we want hw IPv4 cksum */
> >> PKT_TX_IPV6      /* packet is IPv6 */
> >> PKT_TX_IPV4      /* packet is IPv4 */
> >
> > Hmm, and how this is different from what we have now in the Jijiang's patch?
> > Except that you renamed PKT_TX_IP_CKSUM to PKT_TX_IPV4_CKSUM?
> 
> I think it's more like solution 2 with a renaming. And it is more
> coherent to always have "IPV4" on all flag names.

About renaming PKT_TX_IP_CKSUM to PKT_TX_IPV4_CKSUM: I don't mind.
But that means we would have to update all the drivers and apps that using PKT_TX_IP_CKSUM.
Again there are customers that using that flag in their apps right now.
They would have to change it too.
For me -  too much hassle for such small nit.

Konstantin

> 
> Regards,
> Olivier

Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM

[Ananyev, Konstantin]

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Thursday, December 04, 2014 1:51 PM
> To: Ananyev, Konstantin; Thomas Monjalon
> Cc: dev@dpdk.org; Liu, Jijiang
> Subject: Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM
> 
> Hi,
> 
> On 12/04/2014 12:03 PM, Ananyev, Konstantin wrote:
> >>>>> 1/ (Jijiang's patch)
> >>>>> PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */
> >>>>> PKT_TX_IPV6      /* packet is IPv6 */
> >>>>> PKT_TX_IPV4      /* packet is IPv4, and we don't want hw cksum */
> >>>>>
> >>>>> with PKT_TX_IP_CKSUM and PKT_TX_IPV4 exclusive
> >>>>>
> >>>>> and
> >>>>>
> >>>>> 2/
> >>>>> PKT_TX_IP_CKSUM  /* we want hw IP cksum */
> >>>>> PKT_TX_IPV6      /* packet is IPv6 */
> >>>>> PKT_TX_IPV4      /* packet is IPv4 */
> >>>>>
> >>>>> with PKT_TX_IP_CKSUM implies PKT_TX_IPV4
> >>>>>
> >>>>>
> >>>>> Solution 2/ looks better from a user point of view. Anyone else has an opinion?
> >>>>
> >>>> Let's think about these IPv4/6 flags in terms of checksum and IP version/type,
> >>>>
> >>>> 1. For IPv6
> >>>> IP checksum is meaningful only for IPv4,  so we define 'PKT_TX_IPV6      /* packet is IPv6 */' to tell driver/HW that this is IPV6
> >> packet,
> >>>> here we don't talk about the checksum for IPv6 as it is meaningless. Right?
> >>>>
> >>>> PKT_TX_IPV6      /* packet is IPv6 */         ------ IP type: v6;  HW checksum: meaningless
> >>>>
> >>>> 2. For IPv4,
> >>>> My patch:
> >>>>
> >>>> PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */--------------------------IP type: v4;  HW checksum: Yes
> >>>> PKT_TX_IPV4      /* packet is IPv4, and we don't want hw cksum */ ----------------------- IP type: v4;  HW checksum: No
> >>>>
> >>>> You want:
> >>>> PKT_TX_IP_CKSUM  /* we want hw IP cksum */-------------------------- IP type: v4;  HW checksum: Yes
> >>>> PKT_TX_IPV4      /* packet is IPv4*/ ------------------------  IP type: v4; HW checksum: yes or no?
> >>>>                                                                                                         driver/HW don't know, just know this is packet with IPv4 header.
> >>>>                                                                                                         HW checksum: meaningless??
> >>>
> >>> Yep, that's why I also don't like that suggestion: PKT_TX_IPV4 itself doesn't contain all information.
> >>> PMD will have to check PKT_TX_IP_CKSUM  anyway.
> >>
> >> I prefer solution 2 because a flag should bring only 1 information.
> >
> > Why is that? For example in mbuf we already have a flag that brings 2 things:
> > PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */
> 
> For the user, it's clearer to have one information in a flag.
> If you just look at the name of the flag, the natural meaning is 2/,
> else we would need to rename them in:
>    PKT_TX_IPV4_CKSUM
>    PKT_TX_IPV4_NO_CKSUM
> 
> > If it would be possible to compress 10 meanings into 1 bit, I would happily do that.
> > Unfortunately, it is rarely possible :)
> >
> >> It's simply saner and could fit to more situations in the future.
> >
> > Could you give an example of such situation?
> > I personally couldn't come up with the case where #2 would have any real advantage.
> 
> in solution 2/, PKT_TX_IP_CKSUM implies PKT_TX_IPV4 so checking
> PKT_TX_IP_CKSUM is still enough in drivers.

Both 1 and 2 seems backward compatible.

> 
> In the driver, it is also simpler. With solution 1/:
> 
> /* check if we need ipcsum */
> if (flags & PKT_TX_IP_CKSUM)
> 
> /* check if packet is ipv4, may be needed to set a hw field */
> if (flags & (PKT_TX_IP_CKSUM|PKT_TX_IPV4))

Do you really mean 1 here? When all 3 flags are mutually exclusive?
If so, it doesn't look right. For 1 both (PKT_TX_IP_CKSUM|PKT_TX_IPV4) should never be up.  

> 
> 
> With solution 2/
> 
> /* check if we need ipcsum */
> if (flags & PKT_TX_IP_CKSUM)
> 
> /* check if packet is ipv4, may be needed to set a hw field */
> if (flags & PKT_TX_IPV4)

The thing is that it wouldn't be possible with FVL driver - it has to setup mutually exclusive fields for these 2 cases: 
PKT_TX_IP_CKSUM - ipv4 with HW checksum
PKT_TX_IPV4 - ipv4 without HW checksum

So with #2, driver has either:
if (flags & PKT_TX_IP_CKSUM) {...} else if (flags & PKT_TX_IPV4) {...}
And always keep condition for PKT_TX_IP_CKSUM first.
Or do:
if (flags & PKT_TX_IPV4) {...} if (flags & PKT_TX_IP_CKSUM) {...}
and in that case always keep condition for PKT_TX_IP_CKSUM last, so it always overwrite PKT_TX_IPV4 settings.

Basically with #2 PKT_TX_IPV4 is not enough to make a decision, even if it is set, we'll have to check for PKT_TX_IP_CKSUM anyway.

While with 1 we can put them in any order, both:
If (flags & PKT_TX_IP_CKSUM) {...} else if (flags & PKT_TX_IPV4) {...}
And
If (flags & PKT_TX_IPV4) {...} else if (flags & PKT_TX_IP_CKSUM) {...}
Will work.

Konstantin

> 
> 
> I agree it can looks like a detail, but I really think it's important
> to have the most logical and straightforward api for mbuf, as it's
> the core of DPDK.
> 
> Regards,
> Olivier

Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM

[Zhang, Helin]

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Thursday, December 4, 2014 6:20 PM
> To: Zhang, Helin; Olivier MATZ; Liu, Jijiang; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce
> PKT_TX_VXLAN_CKSUM
> 
> Hi Helin,
> 
> > -----Original Message-----
> > From: Zhang, Helin
> > Sent: Thursday, December 04, 2014 6:52 AM
> > To: Olivier MATZ; Ananyev, Konstantin; Liu, Jijiang; dev@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and
> > repalce PKT_TX_VXLAN_CKSUM
> >
> >
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ
> > > Sent: Wednesday, December 3, 2014 10:42 PM
> > > To: Ananyev, Konstantin; Liu, Jijiang; dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags
> > > and repalce PKT_TX_VXLAN_CKSUM
> > >
> > > Hi Konstantin,
> > >
> > > On 12/03/2014 01:59 PM, Ananyev, Konstantin wrote:
> > > >> I still think having a flag IPV4 + another flag IP_CHECKSUM is
> > > >> not appropriate.
> > > >
> > > > Sorry, didn't get you here.
> > > > Are you talking about our discussion should PKT_TX_IP_CKSUM and
> > > PKT_TX_IPV4 be mutually exclusive or not?
> > >
> > > Yes
> > >
> > > >> I though Konstantin agreed on other flags, but I may have
> > > >> misunderstood:
> > > >>
> > > >> http://dpdk.org/ml/archives/dev/2014-November/009070.html
> > > >
> > > > In that mail, I was talking about my suggestion to make
> > > > PKT_TX_IP_CKSUM,
> > > PKT_TX_IPV4 and PKT_TX_IPV6 to occupy 2 bits.
> > > > Something like:
> > > > #define	PKT_TX_IP_CKSUM	(1 << X)
> > > > #define	PKT_TX_IPV6		(2 << X)
> > > > #define 	PKT_TX_IPV4		(3 << X)
> > > >
> > > > "Even better, if we can squeeze these 3 flags into 2 bits.
> > > > Would save us 2 bits, plus might be handy, as in the PMD you can do:
> > > >
> > > > switch (ol_flags & TX_L3_MASK) {
> > > >      case TX_IPV4:
> > > >         ...
> > > >         break;
> > > >      case TX_IPV6:
> > > >         ...
> > > >         break;
> > > >      case TX_IP_CKSUM:
> > > >         ...
> > > >         break;
> > > > }"
> > > >
> > > > As you pointed out, it will break backward compatibility.
> > > > I agreed with that and self-NACKed it.
> > >
> > > ok, so we are back between:
> > >
> > > 1/ (Jijiang's patch)
> > > PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */
> > > PKT_TX_IPV6      /* packet is IPv6 */
> > > PKT_TX_IPV4      /* packet is IPv4, and we don't want hw cksum */
> > >
> > > with PKT_TX_IP_CKSUM and PKT_TX_IPV4 exclusive
> > >
> > > and
> > >
> > > 2/
> > > PKT_TX_IP_CKSUM  /* we want hw IP cksum */
> > > PKT_TX_IPV6      /* packet is IPv6 */
> > > PKT_TX_IPV4      /* packet is IPv4 */
> > There is another bit flag named 'PKT_TX_IPV4_CSUM' which uses the same
> > bit of 'PKT_TX_IP_CSUM'. It is for identifying if ipv4 hardware
> > checksum offload is needed or not.
> 
> Yes, 'PKT_TX_IPV4_CSUM is an alias to PKT_TX_IP_CKSUM  and we are going to
> remove it.
> 
> > It seems that we do not need 'PKT_TX_IPV6_CSUM'.
> 
> No one even planned it.
> 
> > 'PKT_TX_IPV4' and 'PKT_TX_IPV6' just indicates its packet type, and I
> > guess other features should not be contained in it, according to its name.
> >
> > So here I got the option 3:
> > PKT_TX_IPV4_CKSUM  /* we want hw IPv4 cksum */
> > PKT_TX_IPV6      /* packet is IPv6 */
> > PKT_TX_IPV4      /* packet is IPv4 */
> 
> Hmm, and how this is different from what we have now in the Jijiang's patch?
> Except that you renamed PKT_TX_IP_CKSUM to PKT_TX_IPV4_CKSUM?
The comments are different, PKT_TX_IPV4 has different meanings.

Regards,
Helin

> 
> Konstantin
> 
> >
> > >
> > > with PKT_TX_IP_CKSUM implies PKT_TX_IPV4
> > >
> > >
> > > Solution 2/ looks better from a user point of view. Anyone else has an
> opinion?
> > >
> > > Regards,
> > > Olivier
> >
> > Regards,
> > Helin

Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM

[Liu, Jijiang]

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Friday, December 5, 2014 6:56 AM
> To: Olivier MATZ; Thomas Monjalon
> Cc: dev@dpdk.org; Liu, Jijiang
> Subject: RE: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce
> PKT_TX_VXLAN_CKSUM
> 
> 
> 
> > -----Original Message-----
> > From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> > Sent: Thursday, December 04, 2014 1:51 PM
> > To: Ananyev, Konstantin; Thomas Monjalon
> > Cc: dev@dpdk.org; Liu, Jijiang
> > Subject: Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and
> > repalce PKT_TX_VXLAN_CKSUM
> >
> > Hi,
> >
> > On 12/04/2014 12:03 PM, Ananyev, Konstantin wrote:
> > >>>>> 1/ (Jijiang's patch)
> > >>>>> PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */
> > >>>>> PKT_TX_IPV6      /* packet is IPv6 */
> > >>>>> PKT_TX_IPV4      /* packet is IPv4, and we don't want hw cksum */
> > >>>>>
> > >>>>> with PKT_TX_IP_CKSUM and PKT_TX_IPV4 exclusive
> > >>>>>
> > >>>>> and
> > >>>>>
> > >>>>> 2/
> > >>>>> PKT_TX_IP_CKSUM  /* we want hw IP cksum */
> > >>>>> PKT_TX_IPV6      /* packet is IPv6 */
> > >>>>> PKT_TX_IPV4      /* packet is IPv4 */
> > >>>>>
> > >>>>> with PKT_TX_IP_CKSUM implies PKT_TX_IPV4
> > >>>>>
> > >>>>>
> > >>>>> Solution 2/ looks better from a user point of view. Anyone else has an
> opinion?
> > >>>>
> > >>>> Let's think about these IPv4/6 flags in terms of checksum and IP
> > >>>> version/type,
> > >>>>
> > >>>> 1. For IPv6
> > >>>> IP checksum is meaningful only for IPv4,  so we define 'PKT_TX_IPV6      /*
> packet is IPv6 */' to tell driver/HW that this is IPV6
> > >> packet,
> > >>>> here we don't talk about the checksum for IPv6 as it is meaningless.
> Right?
> > >>>>
> > >>>> PKT_TX_IPV6      /* packet is IPv6 */         ------ IP type: v6;  HW checksum:
> meaningless
> > >>>>
> > >>>> 2. For IPv4,
> > >>>> My patch:
> > >>>>
> > >>>> PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */-----------
> ---------------IP type: v4;  HW checksum: Yes
> > >>>> PKT_TX_IPV4      /* packet is IPv4, and we don't want hw cksum */ --------
> --------------- IP type: v4;  HW checksum: No
> > >>>>
> > >>>> You want:
> > >>>> PKT_TX_IP_CKSUM  /* we want hw IP cksum */-------------------------- IP
> type: v4;  HW checksum: Yes
> > >>>> PKT_TX_IPV4      /* packet is IPv4*/ ------------------------  IP type: v4; HW
> checksum: yes or no?
> > >>>>                                                                                                         driver/HW don't
> know, just know this is packet with IPv4 header.
> > >>>>                                                                                                         HW checksum:
> meaningless??
> > >>>
> > >>> Yep, that's why I also don't like that suggestion: PKT_TX_IPV4 itself doesn't
> contain all information.
> > >>> PMD will have to check PKT_TX_IP_CKSUM  anyway.
> > >>
> > >> I prefer solution 2 because a flag should bring only 1 information.
> > >
> > > Why is that? For example in mbuf we already have a flag that brings 2 things:
> > > PKT_TX_IP_CKSUM  /* packet is IPv4, and we want hw cksum */
> >
> > For the user, it's clearer to have one information in a flag.
> > If you just look at the name of the flag, the natural meaning is 2/,
> > else we would need to rename them in:
> >    PKT_TX_IPV4_CKSUM
> >    PKT_TX_IPV4_NO_CKSUM
> >
> > > If it would be possible to compress 10 meanings into 1 bit, I would happily do
> that.
> > > Unfortunately, it is rarely possible :)
> > >
> > >> It's simply saner and could fit to more situations in the future.
> > >
> > > Could you give an example of such situation?
> > > I personally couldn't come up with the case where #2 would have any real
> advantage.
> >
> > in solution 2/, PKT_TX_IP_CKSUM implies PKT_TX_IPV4 so checking
> > PKT_TX_IP_CKSUM is still enough in drivers.
> 
> Both 1 and 2 seems backward compatible.
> 
> >
> > In the driver, it is also simpler. With solution 1/:
> >
> > /* check if we need ipcsum */
> > if (flags & PKT_TX_IP_CKSUM)
> >
> > /* check if packet is ipv4, may be needed to set a hw field */ if
> > (flags & (PKT_TX_IP_CKSUM|PKT_TX_IPV4))
> 
> Do you really mean 1 here? When all 3 flags are mutually exclusive?
> If so, it doesn't look right. For 1 both (PKT_TX_IP_CKSUM|PKT_TX_IPV4) should
> never be up.
> 
> >
> >
> > With solution 2/
> >
> > /* check if we need ipcsum */
> > if (flags & PKT_TX_IP_CKSUM)
> >
> > /* check if packet is ipv4, may be needed to set a hw field */ if
> > (flags & PKT_TX_IPV4)
> 
> The thing is that it wouldn't be possible with FVL driver - it has to setup mutually
> exclusive fields for these 2 cases:
> PKT_TX_IP_CKSUM - ipv4 with HW checksum
> PKT_TX_IPV4 - ipv4 without HW checksum
> 
> So with #2, driver has either:
> if (flags & PKT_TX_IP_CKSUM) {...} else if (flags & PKT_TX_IPV4) {...} And always
> keep condition for PKT_TX_IP_CKSUM first.
> Or do:
> if (flags & PKT_TX_IPV4) {...} if (flags & PKT_TX_IP_CKSUM) {...} and in that case
> always keep condition for PKT_TX_IP_CKSUM last, so it always overwrite
> PKT_TX_IPV4 settings.
> 
> Basically with #2 PKT_TX_IPV4 is not enough to make a decision, even if it is set,
> we'll have to check for PKT_TX_IP_CKSUM anyway.
> 
> While with 1 we can put them in any order, both:
> If (flags & PKT_TX_IP_CKSUM) {...} else if (flags & PKT_TX_IPV4) {...} And If (flags
> & PKT_TX_IPV4) {...} else if (flags & PKT_TX_IP_CKSUM) {...} Will work.
> 
> Konstantin

Yes. I agree.

PKT_TX_IPV4      /* packet is IPv4 */ 
This flag don't have too much offload meaning for TX side,  because we can't use this information to set transmit descriptor ( or set offload register ) precisely , so it is not a real offload flag.

PKT_TX_IPV4      /* packet is IPv4, and we don't want hw cksum */
It  is a offload flag, we can use this flag to set  transmit descriptor precisely.  
Yes, the comments are different, the PKT_TX_IPV4 has different meanings, but we have to consider which comment will affect if offload work or not.

BTW, we pay too much time on this topic...


> >
> >
> > I agree it can looks like a detail, but I really think it's important
> > to have the most logical and straightforward api for mbuf, as it's the
> > core of DPDK.
> >
> > Regards,
> > Olivier

Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM

[Olivier MATZ]

Hi,

On 12/02/2014 04:06 PM, Jijiang Liu wrote:
> Replace PKT_TX_VXLAN_CKSUM with PKT_TX_UDP_TUNNEL_PKT in order to indicate a packet is an UDP tunneling packet, and introduce 3 TX offload flags for outer IP TX checksum, which are PKT_TX_OUTER_IP_CKSUM, PKT_TX_OUTER_IPV4 and PKT_TX_OUTER_IPV6 respectively;Rework csum forward engine and i40e PMD due to these changes.
> 
> Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
> ---
>  app/test-pmd/csumonly.c         |    9 +++++++--
>  lib/librte_mbuf/rte_mbuf.c      |    7 ++++++-
>  lib/librte_mbuf/rte_mbuf.h      |   11 ++++++++++-
>  lib/librte_pmd_i40e/i40e_rxtx.c |    6 +++---
>  4 files changed, 26 insertions(+), 7 deletions(-)
> 

As we need to conclude on this:
Acked-by: Olivier Matz <olivier.matz@6wind.com>

Just few minor comments below:

> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index cbadf8e..6eb898f 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -118,7 +118,7 @@ extern "C" {
>   */
>  #define PKT_TX_TCP_SEG       (1ULL << 49)
>  
> -#define PKT_TX_VXLAN_CKSUM   (1ULL << 50) /**< TX checksum of VXLAN computed by NIC */
> +#define PKT_TX_UDP_TUNNEL_PKT (1ULL << 50) /**< TX packet is an UDP tunneling packet */
>  #define PKT_TX_IEEE1588_TMST (1ULL << 51) /**< TX IEEE1588 packet to timestamp. */

We could replace the comment by:

"Tell the NIC it's a UDP tunneled packet. It must be specified when
 using hw outer checksum offload (PKT_TX_OUTER_IP_CKSUM)"

>  
>  /**
> @@ -149,6 +149,15 @@ extern "C" {
>  
>  #define PKT_TX_VLAN_PKT      (1ULL << 57) /**< TX packet is a 802.1q VLAN packet. */
>  
> +/** Outer IP checksum of TX packet, computed by NIC for tunneling packet */
> +#define PKT_TX_OUTER_IP_CKSUM   (1ULL << 58)

Maybe add:
"The tunnel type must also be specified, ex: PKT_TX_UDP_TUNNEL_PKT"

(altought I don't understand why the hw need this info)


Regards,
Olivier

Re: [dpdk-dev] [PATCH v5 3/3] mbuf:replace the inner_l2_len and the inner_l3_len fields

[Olivier MATZ]

Hi,

On 12/02/2014 04:06 PM, Jijiang Liu wrote:
> Replace the inner_l2_len and the inner_l3_len field with the outer_l2_len and outer_l3_len field, and rework csum forward engine and i40e PMD due to  these changes.
> 
> Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
> ---
>  app/test-pmd/csumonly.c         |   58 +++++++++++++++++++++------------------
>  lib/librte_mbuf/rte_mbuf.h      |    4 +-
>  lib/librte_pmd_i40e/i40e_rxtx.c |   38 +++++++++++++------------
>  3 files changed, 53 insertions(+), 47 deletions(-)
> 

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

One minor comment below:

> @@ -303,8 +305,7 @@ process_outer_cksums(void *outer_l3_hdr, uint16_t outer_ethertype,
>   * 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 and UDP layer (if packet is
> - * recognized as a vxlan packet).
> + * VxLAN flag concerns the outer IP(if packet is recognized as a vxlan packet).
>   */
>  static void
>  pkt_burst_checksum_forward(struct fwd_stream *fs)

Typo (no space before parenthesis)

Regards,
Olivier

Re: [dpdk-dev] [PATCH v5 0/3] i40e VXLAN TX checksum rework

[Thomas Monjalon]

> > We have got some feedback about backward compatibility of VXLAN TX checksum offload API with 1G/10G NIC after the i40e VXLAN
> > TX checksum codes were applied, so we have to rework the APIs on i40e, including the changes of mbuf, i40e PMD and csum forward
> > engine.
> > 
> > The main changes in mbuf are as follows, in place of removing PKT_TX_VXLAN_CKSUM, we introduce 4 new flags:
> > PKT_TX_OUTER_IP_CKSUM, PKT_TX_OUTER_IPV4, PKT_TX_OUTER_IPV6 and PKT_TX_UDP_TUNNEL_PKT. Replace the inner_l2_len
> > and the inner_l3_len field with the outer_l2_len and outer_l3_len field.
> > 
> > Let's use a few examples to demonstrate how to use these new flags and existing flags in rte_mbuf.h
> > 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. There
> > could be several scenarios:
> > 
> > A) User requests HW offload for ipv4_hdr_out checksum.
> > He doesn't care is it a tunnelled packet or not. So he sets:
> > 
> > mb->l2_len = eth_hdr_out;
> > mb->l3_len = ipv4_hdr_out;
> > mb->ol_flags |= PKT_TX_IPV4_CSUM;
> > 
> > B) User is aware that it is a tunnelled 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_out + vxlan_hdr +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 | 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 tunnelled packet, but makes HW to treat it as ordinary (non-tunnelled) 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;
> > 
> > i40e PMD will support both B.1 and B.2, but ixgbe/igb/em PMD supports only B.2.
> > if HW supports both - it will be up to user app which method to choose.
> > tespmd will support both methods, and it should be configurable by user which approach to use (cmdline parameter).
> > So the user can try/test both methods and select an appropriate for him.
> > 
> > C) User knows that is a tunnelled packet, and wants HW offload for all 3 checksums:  outer IP hdr checksum, inner IP checksum, inner
> > TCP checksum.
> > Then he has to setup all TX checksum fields:
> >      mb->l2_len =  udp_hdr_out + vxlan_hdr +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_OUT_IP_CKSUM  | PKT_TX_UDP_TUNNEL_PKT | PKT_TX_IP_CKSUM |  PKT_TX_TCP_CKSUM;
> > 
> > Change notes:
> > v2 changes:
> >      remove PKT_TX_IP_CKSUM alias.
> >      add PKT_TX_OUT_IP_CKSUM and PKT_TX_OUTER_IPV6 in rte_get_tx_ol_flag_name.
> >      spliting mbuf changes into two patches.
> >      fix MACLEN caculation issue in i40e driver
> >      fix some issues in csumonly.c
> >      change cover letter.
> > v3 changes:
> >      fix MACLEN caculation issue in i40e driver when non-tunneling packet
> > v4 changes:
> >      reorganize patches to avoid compilation to be broken between patches.
> >      remove l4_tun_len from mbuf structure.
> >      add PKT_TX_OUTER_IPV4 to indicate no IP checksum offload requirement for tunneling packet.
> >      change i40e PMD and csum engine due to above changes.
> > 
> > v5 changes:
> >      according to Konstantin's comments, optimize process_outer_cksums() in order to avoid setting PKT_TX_OUTER_IPV4 flags for the
> > case when user didn't enable TESTPMD_TX_OFFLOAD_VXLAN_CKSUM
> > 
> > Jijiang Liu (3):
> >   Redefine PKT_TX_IPV4, PKT_TX_IPV6 and PKT_TX_VLAN_PKT;
> >   Replace PKT_TX_VXLAN_CKSUM with PKT_TX_UDP_TUNNEL_PKT, and add 3 TX flags, which are PKT_TX_OUTER_IP_CKSUM,
> > PKT_TX_OUTER_IPV4 and PKT_TX_OUTER_IPV6,and rework csum forward engine and i40e pmd due to these changes;
> >   Replace the inner_l2_len and the inner_l3_len field with the outer_l2_len and outer_l3_len field, and rework csum forward engine
> > and i40e pmd due to  these changes;
> 
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Applied with small comments changes suggested by Olivier.

Thanks everyone for finding a consensus.
It's not easy to accept different of views but we finally did it!

Lesson learned: every details of an API must be explicited with clear and
short sentences.

Thanks
-- 
Thomas

Re: [dpdk-dev] [PATCH v5 0/3] i40e VXLAN TX checksum rework

[Ananyev, Konstantin]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, December 05, 2014 4:07 PM
> To: Liu, Jijiang
> Cc: dev@dpdk.org; Ananyev, Konstantin; olivier.matz@6wind.com
> Subject: Re: [dpdk-dev] [PATCH v5 0/3] i40e VXLAN TX checksum rework
> 
> > > We have got some feedback about backward compatibility of VXLAN TX checksum offload API with 1G/10G NIC after the i40e
> VXLAN
> > > TX checksum codes were applied, so we have to rework the APIs on i40e, including the changes of mbuf, i40e PMD and csum
> forward
> > > engine.
> > >
> > > The main changes in mbuf are as follows, in place of removing PKT_TX_VXLAN_CKSUM, we introduce 4 new flags:
> > > PKT_TX_OUTER_IP_CKSUM, PKT_TX_OUTER_IPV4, PKT_TX_OUTER_IPV6 and PKT_TX_UDP_TUNNEL_PKT. Replace the
> inner_l2_len
> > > and the inner_l3_len field with the outer_l2_len and outer_l3_len field.
> > >
> > > Let's use a few examples to demonstrate how to use these new flags and existing flags in rte_mbuf.h
> > > 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.
> There
> > > could be several scenarios:
> > >
> > > A) User requests HW offload for ipv4_hdr_out checksum.
> > > He doesn't care is it a tunnelled packet or not. So he sets:
> > >
> > > mb->l2_len = eth_hdr_out;
> > > mb->l3_len = ipv4_hdr_out;
> > > mb->ol_flags |= PKT_TX_IPV4_CSUM;
> > >
> > > B) User is aware that it is a tunnelled 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_out + vxlan_hdr +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 | 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 tunnelled packet, but makes HW to treat it as ordinary (non-tunnelled) 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;
> > >
> > > i40e PMD will support both B.1 and B.2, but ixgbe/igb/em PMD supports only B.2.
> > > if HW supports both - it will be up to user app which method to choose.
> > > tespmd will support both methods, and it should be configurable by user which approach to use (cmdline parameter).
> > > So the user can try/test both methods and select an appropriate for him.
> > >
> > > C) User knows that is a tunnelled packet, and wants HW offload for all 3 checksums:  outer IP hdr checksum, inner IP checksum,
> inner
> > > TCP checksum.
> > > Then he has to setup all TX checksum fields:
> > >      mb->l2_len =  udp_hdr_out + vxlan_hdr +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_OUT_IP_CKSUM  | PKT_TX_UDP_TUNNEL_PKT | PKT_TX_IP_CKSUM |  PKT_TX_TCP_CKSUM;
> > >
> > > Change notes:
> > > v2 changes:
> > >      remove PKT_TX_IP_CKSUM alias.
> > >      add PKT_TX_OUT_IP_CKSUM and PKT_TX_OUTER_IPV6 in rte_get_tx_ol_flag_name.
> > >      spliting mbuf changes into two patches.
> > >      fix MACLEN caculation issue in i40e driver
> > >      fix some issues in csumonly.c
> > >      change cover letter.
> > > v3 changes:
> > >      fix MACLEN caculation issue in i40e driver when non-tunneling packet
> > > v4 changes:
> > >      reorganize patches to avoid compilation to be broken between patches.
> > >      remove l4_tun_len from mbuf structure.
> > >      add PKT_TX_OUTER_IPV4 to indicate no IP checksum offload requirement for tunneling packet.
> > >      change i40e PMD and csum engine due to above changes.
> > >
> > > v5 changes:
> > >      according to Konstantin's comments, optimize process_outer_cksums() in order to avoid setting PKT_TX_OUTER_IPV4 flags for
> the
> > > case when user didn't enable TESTPMD_TX_OFFLOAD_VXLAN_CKSUM
> > >
> > > Jijiang Liu (3):
> > >   Redefine PKT_TX_IPV4, PKT_TX_IPV6 and PKT_TX_VLAN_PKT;
> > >   Replace PKT_TX_VXLAN_CKSUM with PKT_TX_UDP_TUNNEL_PKT, and add 3 TX flags, which are PKT_TX_OUTER_IP_CKSUM,
> > > PKT_TX_OUTER_IPV4 and PKT_TX_OUTER_IPV6,and rework csum forward engine and i40e pmd due to these changes;
> > >   Replace the inner_l2_len and the inner_l3_len field with the outer_l2_len and outer_l3_len field, and rework csum forward
> engine
> > > and i40e pmd due to  these changes;
> >
> > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 
> Applied with small comments changes suggested by Olivier.
> 
> Thanks everyone for finding a consensus.
> It's not easy to accept different of views but we finally did it!
> 
> Lesson learned: every details of an API must be explicited with clear and
> short sentences.

>From my side I would like to thank Olivier for all his tremendous help with that feature.
Konstantin

> 
> Thanks
> --
> Thomas