DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH v3 0/4] i40e VXLAN TX checksum rework
@ 2014-11-27 17:03 Jijiang Liu
  2014-11-27 17:03 ` [dpdk-dev] [PATCH v3 1/4] mbuf:add three TX offload flags and change three fields Jijiang Liu
                   ` (4 more replies)
  0 siblings, 5 replies; 17+ messages in thread
From: Jijiang Liu @ 2014-11-27 17:03 UTC (permalink / raw)
  To: dev

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 engine.
 
The main changes in mbuf are as follows,
In place of removing PKT_TX_VXLAN_CKSUM, we introducing 3 new flags: PKT_TX_OUTER_IP_CKSUM,PKT_TX_OUTER_IPV6 and PKT_TX_UDP_TUNNEL_PKT, and a new field: l4_tun_len.
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 =  eth_hdr_in;
     mb->l3_len = ipv4_hdr_in;
     mb->outer_l2_len = eth_hdr_out;
     mb->outer_l3_len = ipv4_hdr_out;
     mb->l4tun_len = vxlan_hdr;
     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.
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.
 
Now, B.2 is exactly what Oliver suggested.
I think it has few important advantages over B.1:
First of all - compatibility. It works across all HW we currently support (i40e/ixgbe/igb/em).
Second - it is probably faster even on FVL, as for it we have to fill only TXD, while with approach #2 we have to fill both TCD and TXD.
 
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 =  eth_hdr_in;
     mb->l3_len = ipv4_hdr_in;
     mb->outer_l2_len = eth_hdr_out;
     mb->outer_l3_len = ipv4_hdr_out;
     mb->l4tun_len = vxlan_hdr;
     mb->ol_flags |= PKT_TX_OUT_IP_CKSUM  | PKT_TX_UDP_TUNNEL | PKT_TX_IP_CKSUM |  PKT_TX_TCP_CKSUM;

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 
 
Jijiang Liu (4):
  mbuf change for 3 new flags and 3 fields
  mbuf change for PKT_TX_IPV4 and PKT_TX_IPV6
  i40e PMD change in i40e_rxtx.c
  rework csum forward engine


 app/test-pmd/csumonly.c         |   65 ++++++++++++++++++++++-----------------
 lib/librte_mbuf/rte_mbuf.c      |    6 +++-
 lib/librte_mbuf/rte_mbuf.h      |   22 ++++++++-----
 lib/librte_pmd_i40e/i40e_rxtx.c |   49 ++++++++++++++++-------------
 4 files changed, 82 insertions(+), 60 deletions(-)

-- 
1.7.7.6

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [dpdk-dev] [PATCH v3 1/4] mbuf:add three TX offload flags and change three fields
  2014-11-27 17:03 [dpdk-dev] [PATCH v3 0/4] i40e VXLAN TX checksum rework Jijiang Liu
@ 2014-11-27 17:03 ` Jijiang Liu
  2014-11-28  9:36   ` Olivier MATZ
  2014-11-27 17:03 ` [dpdk-dev] [PATCH v3 2/4] mbuf:change PKT_TX_IPV4 and PKT_TX_IPV6 definition Jijiang Liu
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 17+ messages in thread
From: Jijiang Liu @ 2014-11-27 17:03 UTC (permalink / raw)
  To: dev

In place of removing the PKT_TX_VXLAN_CKSUM, we introduce 3 new flags: PKT_TX_OUTER_IP_CKSUM, PKT_TX_OUTER_IPV6 and PKT_TX_UDP_TUNNEL_PKT, and a new field: l4_tun_len.
Replace the inner_l2_len and the inner_l3_len field with the outer_l2_len and outer_l3_len field.
 
PKT_TX_OUTER_IP_CKSUM: is not used for non-tunnelling packet;hardware outer checksum for tunnelling packet.
PKT_TX_OUTER_IPV6: Tell the NIC it's an outer IPv6 packet for tunneling packet.
PKT_TX_UDP_TUNNEL_PKT: is used to tell PMD that the transmit packet is a UDP tunneling packet.
l4_tun_len: for VXLAN packet, it should be udp header length plus VXLAN header length.


Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
---
 lib/librte_mbuf/rte_mbuf.c |    6 +++++-
 lib/librte_mbuf/rte_mbuf.h |   18 +++++++++++-------
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 87c2963..3c47477 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -240,7 +240,11 @@ 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_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_IPV6: return "PKT_TX_OUTER_IPV6";
 	case PKT_TX_TCP_SEG: return "PKT_TX_TCP_SEG";
 	default: return NULL;
 	}
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 367fc56..22ee555 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -99,10 +99,9 @@ extern "C" {
 #define PKT_RX_TUNNEL_IPV6_HDR (1ULL << 12) /**< RX tunnel packet with IPv6 header. */
 #define PKT_RX_FDIR_ID       (1ULL << 13) /**< FD id reported if FDIR match. */
 #define PKT_RX_FDIR_FLX      (1ULL << 14) /**< Flexible bytes reported if FDIR match. */
-/* add new RX flags here */
 
 /* add new TX flags here */
-#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. */
 
 /**
@@ -125,13 +124,19 @@ 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. */
 
+#define PKT_TX_VLAN_PKT      (1ULL << 55) /**< TX packet is a 802.1q VLAN packet. */
+
 /** Tell the NIC it's an IPv4 packet. Required for L4 checksum offload or TSO. */
 #define PKT_TX_IPV4          PKT_RX_IPV4_HDR
 
 /** Tell the NIC it's an IPv6 packet. Required for L4 checksum offload or TSO. */
 #define PKT_TX_IPV6          PKT_RX_IPV6_HDR
 
-#define PKT_TX_VLAN_PKT      (1ULL << 55) /**< TX packet is a 802.1q VLAN packet. */
+/** Outer IP cksum of TX pkt. computed by NIC for tunneling packet */
+#define PKT_TX_OUTER_IP_CKSUM   (1ULL << 58)
+
+/** Tell the NIC it's an outer IPv6 packet for tunneling packet.*/
+#define PKT_TX_OUTER_IPV6    (1ULL << 59)
 
 /**
  * TCP segmentation offload. To enable this offload feature for a
@@ -266,10 +271,9 @@ 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 unused:8; */
+			uint64_t outer_l3_len:9; /**< outer L3 (IP) Hdr Length. */
+			uint64_t outer_l2_len:7; /**< outer L2 (MAC) Hdr Length. */
+			uint64_t l4_tun_len:8; /**< L4 tunnelling header length */
 		};
 	};
 } __rte_cache_aligned;
-- 
1.7.7.6

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [dpdk-dev] [PATCH v3 2/4] mbuf:change PKT_TX_IPV4 and PKT_TX_IPV6 definition
  2014-11-27 17:03 [dpdk-dev] [PATCH v3 0/4] i40e VXLAN TX checksum rework Jijiang Liu
  2014-11-27 17:03 ` [dpdk-dev] [PATCH v3 1/4] mbuf:add three TX offload flags and change three fields Jijiang Liu
@ 2014-11-27 17:03 ` Jijiang Liu
  2014-11-28  9:37   ` Olivier MATZ
  2014-11-27 17:03 ` [dpdk-dev] [PATCH v3 3/4] i40e:PMD change for VXLAN TX checksum Jijiang Liu
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 17+ messages in thread
From: Jijiang Liu @ 2014-11-27 17:03 UTC (permalink / raw)
  To: dev

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 |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 22ee555..f6b3185 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -127,10 +127,10 @@ extern "C" {
 #define PKT_TX_VLAN_PKT      (1ULL << 55) /**< TX packet is a 802.1q VLAN packet. */
 
 /** Tell the NIC it's an IPv4 packet. Required for L4 checksum offload or TSO. */
-#define PKT_TX_IPV4          PKT_RX_IPV4_HDR
+#define PKT_TX_IPV4          (1ULL << 56)
 
 /** Tell the NIC it's an IPv6 packet. Required for L4 checksum offload or TSO. */
-#define PKT_TX_IPV6          PKT_RX_IPV6_HDR
+#define PKT_TX_IPV6          (1ULL << 57)
 
 /** Outer IP cksum of TX pkt. computed by NIC for tunneling packet */
 #define PKT_TX_OUTER_IP_CKSUM   (1ULL << 58)
-- 
1.7.7.6

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [dpdk-dev] [PATCH v3 3/4] i40e:PMD change for VXLAN TX checksum
  2014-11-27 17:03 [dpdk-dev] [PATCH v3 0/4] i40e VXLAN TX checksum rework Jijiang Liu
  2014-11-27 17:03 ` [dpdk-dev] [PATCH v3 1/4] mbuf:add three TX offload flags and change three fields Jijiang Liu
  2014-11-27 17:03 ` [dpdk-dev] [PATCH v3 2/4] mbuf:change PKT_TX_IPV4 and PKT_TX_IPV6 definition Jijiang Liu
@ 2014-11-27 17:03 ` Jijiang Liu
  2014-11-27 17:03 ` [dpdk-dev] [PATCH v3 4/4] testpmd:rework csum forward engine Jijiang Liu
  2014-11-27 17:24 ` [dpdk-dev] [PATCH v3 0/4] i40e VXLAN TX checksum rework Ananyev, Konstantin
  4 siblings, 0 replies; 17+ messages in thread
From: Jijiang Liu @ 2014-11-27 17:03 UTC (permalink / raw)
  To: dev

Rework the i40e PMD codes using the new introduced ol_flags and fields.

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

diff --git a/lib/librte_pmd_i40e/i40e_rxtx.c b/lib/librte_pmd_i40e/i40e_rxtx.c
index cce6911..be06e8f 100644
--- a/lib/librte_pmd_i40e/i40e_rxtx.c
+++ b/lib/librte_pmd_i40e/i40e_rxtx.c
@@ -456,48 +456,48 @@ i40e_rxd_build_fdir(volatile union i40e_rx_desc *rxdp, struct rte_mbuf *mb)
 #endif
 	return flags;
 }
+
 static inline void
 i40e_txd_enable_checksum(uint64_t ol_flags,
 			uint32_t *td_cmd,
 			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,
+			uint8_t l4_tun_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 */
-	if (unlikely(ol_flags & PKT_TX_VXLAN_CKSUM)) {
-		uint8_t l4tun_len;
-
-		l4tun_len = ETHER_VXLAN_HLEN + inner_l2_len;
+	/* UDP tunneling packet TX checksum offload */
+	if (unlikely(ol_flags & PKT_TX_UDP_TUNNEL_PKT)) {
+		uint16_t l4_tunnel_len;
 
-		if (ol_flags & PKT_TX_IPV4_CSUM)
+		*td_offset |= (outer_l2_len >> 1)
+				<< I40E_TX_DESC_LENGTH_MACLEN_SHIFT;
+		l4_tunnel_len = l4_tun_len + l2_len;
+		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_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) <<
+				(l4_tunnel_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) {
 		*td_cmd |= I40E_TX_DESC_CMD_IIPT_IPV4_CSUM;
@@ -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;
@@ -1190,8 +1190,9 @@ 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;
+	uint8_t l4_tun_len;
 	uint16_t nb_used;
 	uint16_t nb_ctx;
 	uint16_t tx_last;
@@ -1219,9 +1220,12 @@ 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;
+
+		/* L4 Tunneling Length */
+		l4_tun_len = tx_pkt->l4_tun_len;
 
 		/* Calculate the number of context descriptors needed. */
 		nb_ctx = i40e_calc_context_desc(ol_flags);
@@ -1271,8 +1275,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, l4_tun_len,
 						&cd_tunneling_params);
 
 		if (unlikely(nb_ctx)) {
-- 
1.7.7.6

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [dpdk-dev] [PATCH v3 4/4] testpmd:rework csum forward engine
  2014-11-27 17:03 [dpdk-dev] [PATCH v3 0/4] i40e VXLAN TX checksum rework Jijiang Liu
                   ` (2 preceding siblings ...)
  2014-11-27 17:03 ` [dpdk-dev] [PATCH v3 3/4] i40e:PMD change for VXLAN TX checksum Jijiang Liu
@ 2014-11-27 17:03 ` Jijiang Liu
  2014-11-28  9:50   ` Olivier MATZ
  2014-11-27 17:24 ` [dpdk-dev] [PATCH v3 0/4] i40e VXLAN TX checksum rework Ananyev, Konstantin
  4 siblings, 1 reply; 17+ messages in thread
From: Jijiang Liu @ 2014-11-27 17:03 UTC (permalink / raw)
  To: dev

The changes include:
1. use the new introduced ol_flags and fields in csumonly.c file;
2. fix an issue of outer UDP checksum check; 
3. change process logic in the process_inner_cksums();

Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
---
 app/test-pmd/csumonly.c |   65 ++++++++++++++++++++++++++--------------------
 1 files changed, 37 insertions(+), 28 deletions(-)

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index d8c080a..f7ad3d8 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
@@ -257,27 +258,28 @@ 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;
 
-		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 (outer_ethertype == _htons(ETHER_TYPE_IPv6))
+		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,7 +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
+ * VxLAN flag concerns the outer IP(if packet is
  * recognized as a vxlan packet).
  */
 static void
@@ -320,7 +322,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 +362,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 */
@@ -377,15 +380,17 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
 		/* check if it's a supported tunnel (only vxlan for now) */
 		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 +437,11 @@ 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 = l2_len;
+				m->l3_len = l3_len;
+				m->l4_tun_len = l4_tun_len;
 			}
 			else {
 				/* if we don't do vxlan cksum in hw,
@@ -470,7 +476,9 @@ 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_OUTER_IP_CKSUM, PKT_TX_OUTER_IP_CKSUM },
+				{ PKT_TX_OUTER_IPV6, PKT_TX_OUTER_IPV6 },
 				{ PKT_TX_TCP_SEG, PKT_TX_TCP_SEG },
 			};
 			unsigned j;
@@ -498,8 +506,9 @@ 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->l4_tun_len=%d", m->outer_l2_len,
+					m->outer_l3_len, m->l4_tun_len);
 			if (tso_segsz != 0)
 				printf("tx: m->tso_segsz=%d\n", m->tso_segsz);
 			printf("tx: flags=");
-- 
1.7.7.6

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH v3 0/4] i40e VXLAN TX checksum rework
  2014-11-27 17:03 [dpdk-dev] [PATCH v3 0/4] i40e VXLAN TX checksum rework Jijiang Liu
                   ` (3 preceding siblings ...)
  2014-11-27 17:03 ` [dpdk-dev] [PATCH v3 4/4] testpmd:rework csum forward engine Jijiang Liu
@ 2014-11-27 17:24 ` Ananyev, Konstantin
  4 siblings, 0 replies; 17+ messages in thread
From: Ananyev, Konstantin @ 2014-11-27 17:24 UTC (permalink / raw)
  To: Liu, Jijiang, dev



> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jijiang Liu
> Sent: Thursday, November 27, 2014 5:03 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v3 0/4] 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 engine.
> 
> The main changes in mbuf are as follows,
> In place of removing PKT_TX_VXLAN_CKSUM, we introducing 3 new flags: PKT_TX_OUTER_IP_CKSUM,PKT_TX_OUTER_IPV6 and
> PKT_TX_UDP_TUNNEL_PKT, and a new field: l4_tun_len.
> 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 =  eth_hdr_in;
>      mb->l3_len = ipv4_hdr_in;
>      mb->outer_l2_len = eth_hdr_out;
>      mb->outer_l3_len = ipv4_hdr_out;
>      mb->l4tun_len = vxlan_hdr;
>      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.
> 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.
> 
> Now, B.2 is exactly what Oliver suggested.
> I think it has few important advantages over B.1:
> First of all - compatibility. It works across all HW we currently support (i40e/ixgbe/igb/em).
> Second - it is probably faster even on FVL, as for it we have to fill only TXD, while with approach #2 we have to fill both TCD and TXD.
> 
> 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 =  eth_hdr_in;
>      mb->l3_len = ipv4_hdr_in;
>      mb->outer_l2_len = eth_hdr_out;
>      mb->outer_l3_len = ipv4_hdr_out;
>      mb->l4tun_len = vxlan_hdr;
>      mb->ol_flags |= PKT_TX_OUT_IP_CKSUM  | PKT_TX_UDP_TUNNEL | PKT_TX_IP_CKSUM |  PKT_TX_TCP_CKSUM;
> 
> 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
> 
> Jijiang Liu (4):
>   mbuf change for 3 new flags and 3 fields
>   mbuf change for PKT_TX_IPV4 and PKT_TX_IPV6
>   i40e PMD change in i40e_rxtx.c
>   rework csum forward engine
> 
> 
>  app/test-pmd/csumonly.c         |   65 ++++++++++++++++++++++-----------------
>  lib/librte_mbuf/rte_mbuf.c      |    6 +++-
>  lib/librte_mbuf/rte_mbuf.h      |   22 ++++++++-----
>  lib/librte_pmd_i40e/i40e_rxtx.c |   49 ++++++++++++++++-------------
>  4 files changed, 82 insertions(+), 60 deletions(-)
> 
> --
> 1.7.7.6

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/4] mbuf:add three TX offload flags and change three fields
  2014-11-27 17:03 ` [dpdk-dev] [PATCH v3 1/4] mbuf:add three TX offload flags and change three fields Jijiang Liu
@ 2014-11-28  9:36   ` Olivier MATZ
  2014-11-28 10:27     ` Ananyev, Konstantin
  2014-11-28 10:33     ` Liu, Jijiang
  0 siblings, 2 replies; 17+ messages in thread
From: Olivier MATZ @ 2014-11-28  9:36 UTC (permalink / raw)
  To: Jijiang Liu, dev

Hi Jijiang,

On 11/27/2014 06:03 PM, Jijiang Liu wrote:
>  /** Tell the NIC it's an IPv4 packet. Required for L4 checksum offload or TSO. */
>  #define PKT_TX_IPV4          PKT_RX_IPV4_HDR
>  
>  /** Tell the NIC it's an IPv6 packet. Required for L4 checksum offload or TSO. */
>  #define PKT_TX_IPV6          PKT_RX_IPV6_HDR

The description still does not match what we discussed. Either we
have PKT_TX_IPV4 meaning "packet is IPv4 without requiring IP cksum
offload", or  "packet is IPv4". I prefer the second one, but whatever
the choice is, the comments must be coherent.

> -#define PKT_TX_VLAN_PKT      (1ULL << 55) /**< TX packet is a 802.1q VLAN packet. */
> +/** Outer IP cksum of TX pkt. computed by NIC for tunneling packet */
> +#define PKT_TX_OUTER_IP_CKSUM   (1ULL << 58)
> +
> +/** Tell the NIC it's an outer IPv6 packet for tunneling packet.*/
> +#define PKT_TX_OUTER_IPV6    (1ULL << 59)

I think we should have the same flags with the same meanings for
inner and outer:

- PKT_TX_IPV4, PKT_TX_IP_CKSUM, PKT_TX_IPV6
- PKT_TX_OUTER_IPV4, PKT_TX_OUTER_IP_CKSUM, PKT_TX_OUTER_IPV6

In your patch there is no PKT_TX_OUTER_IPV4 flag.

Regards,
Olivier

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH v3 2/4] mbuf:change PKT_TX_IPV4 and PKT_TX_IPV6 definition
  2014-11-27 17:03 ` [dpdk-dev] [PATCH v3 2/4] mbuf:change PKT_TX_IPV4 and PKT_TX_IPV6 definition Jijiang Liu
@ 2014-11-28  9:37   ` Olivier MATZ
  0 siblings, 0 replies; 17+ messages in thread
From: Olivier MATZ @ 2014-11-28  9:37 UTC (permalink / raw)
  To: Jijiang Liu, dev



On 11/27/2014 06:03 PM, Jijiang Liu wrote:
> 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 |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 22ee555..f6b3185 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -127,10 +127,10 @@ extern "C" {
>  #define PKT_TX_VLAN_PKT      (1ULL << 55) /**< TX packet is a 802.1q VLAN packet. */
>  
>  /** Tell the NIC it's an IPv4 packet. Required for L4 checksum offload or TSO. */
> -#define PKT_TX_IPV4          PKT_RX_IPV4_HDR
> +#define PKT_TX_IPV4          (1ULL << 56)
>  
>  /** Tell the NIC it's an IPv6 packet. Required for L4 checksum offload or TSO. */
> -#define PKT_TX_IPV6          PKT_RX_IPV6_HDR
> +#define PKT_TX_IPV6          (1ULL << 57)
>  
>  /** Outer IP cksum of TX pkt. computed by NIC for tunneling packet */
>  #define PKT_TX_OUTER_IP_CKSUM   (1ULL << 58)
> 

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH v3 4/4] testpmd:rework csum forward engine
  2014-11-27 17:03 ` [dpdk-dev] [PATCH v3 4/4] testpmd:rework csum forward engine Jijiang Liu
@ 2014-11-28  9:50   ` Olivier MATZ
  2014-11-28 10:10     ` Thomas Monjalon
  0 siblings, 1 reply; 17+ messages in thread
From: Olivier MATZ @ 2014-11-28  9:50 UTC (permalink / raw)
  To: Jijiang Liu, dev

Hi Jijiang,

On 11/27/2014 06:03 PM, Jijiang Liu wrote:
> @@ -303,7 +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
> + * VxLAN flag concerns the outer IP(if packet is
>   * recognized as a vxlan packet).
>   */

Please reindent the comment properly, taking care of the space before
the parenthesis.

Another comment that concerns the whole patchset. It's maybe a question
for Thomas, but I think that having patches that don't break compilation
between each other would help when bissecting.

In this case, I wonder if it's possible to split in something like:

- change PKT_TX_IPV4 and PKT_TX_IPV6 definition
- add + rename flags (update them in mbuf + testpmd + i40)
- rename inner_l{23}, l{23} in l{23}, outer_l{23} (update them in mbuf
  + testpmd + i40)

I'm not sure it's feasible, I let Thomas give his mind about this.

Regards,
Olivier

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH v3 4/4] testpmd:rework csum forward engine
  2014-11-28  9:50   ` Olivier MATZ
@ 2014-11-28 10:10     ` Thomas Monjalon
  0 siblings, 0 replies; 17+ messages in thread
From: Thomas Monjalon @ 2014-11-28 10:10 UTC (permalink / raw)
  To: Jijiang Liu; +Cc: dev

2014-11-28 10:50, Olivier MATZ:
> Hi Jijiang,
> 
> On 11/27/2014 06:03 PM, Jijiang Liu wrote:
> > @@ -303,7 +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
> > + * VxLAN flag concerns the outer IP(if packet is
> >   * recognized as a vxlan packet).
> >   */
> 
> Please reindent the comment properly, taking care of the space before
> the parenthesis.
> 
> Another comment that concerns the whole patchset. It's maybe a question
> for Thomas, but I think that having patches that don't break compilation
> between each other would help when bissecting.
> 
> In this case, I wonder if it's possible to split in something like:
> 
> - change PKT_TX_IPV4 and PKT_TX_IPV6 definition
> - add + rename flags (update them in mbuf + testpmd + i40)
> - rename inner_l{23}, l{23} in l{23}, outer_l{23} (update them in mbuf
>   + testpmd + i40)
> 
> I'm not sure it's feasible, I let Thomas give his mind about this.

Exact, compilation must not be broken between patches.
The proposal from Olivier seems OK.

-- 
Thomas

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/4] mbuf:add three TX offload flags and change three fields
  2014-11-28  9:36   ` Olivier MATZ
@ 2014-11-28 10:27     ` Ananyev, Konstantin
  2014-11-28 10:33     ` Liu, Jijiang
  1 sibling, 0 replies; 17+ messages in thread
From: Ananyev, Konstantin @ 2014-11-28 10:27 UTC (permalink / raw)
  To: Olivier MATZ, Liu, Jijiang, dev



> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ
> Sent: Friday, November 28, 2014 9:37 AM
> To: Liu, Jijiang; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 1/4] mbuf:add three TX offload flags and change three fields
> 
> Hi Jijiang,
> 
> On 11/27/2014 06:03 PM, Jijiang Liu wrote:
> >  /** Tell the NIC it's an IPv4 packet. Required for L4 checksum offload or TSO. */
> >  #define PKT_TX_IPV4          PKT_RX_IPV4_HDR
> >
> >  /** Tell the NIC it's an IPv6 packet. Required for L4 checksum offload or TSO. */
> >  #define PKT_TX_IPV6          PKT_RX_IPV6_HDR
> 
> The description still does not match what we discussed. Either we
> have PKT_TX_IPV4 meaning "packet is IPv4 without requiring IP cksum
> offload", or  "packet is IPv4". I prefer the second one, but whatever
> the choice is, the comments must be coherent.
> 
> > -#define PKT_TX_VLAN_PKT      (1ULL << 55) /**< TX packet is a 802.1q VLAN packet. */
> > +/** Outer IP cksum of TX pkt. computed by NIC for tunneling packet */
> > +#define PKT_TX_OUTER_IP_CKSUM   (1ULL << 58)
> > +
> > +/** Tell the NIC it's an outer IPv6 packet for tunneling packet.*/
> > +#define PKT_TX_OUTER_IPV6    (1ULL << 59)
> 
> I think we should have the same flags with the same meanings for
> inner and outer:
> 
> - PKT_TX_IPV4, PKT_TX_IP_CKSUM, PKT_TX_IPV6
> - PKT_TX_OUTER_IPV4, PKT_TX_OUTER_IP_CKSUM, PKT_TX_OUTER_IPV6

Good point.
I still think: should we squeeze each of these triple in 2 bits?
Same as we doing for L4 checksum flags:

#define PKT_TX_IP_CKSUM   (1ULL << X)
#define PKT_TX_IPV6                (2ULL << X)
#define PKT_TX_IPV4                (3ULL << X)

Same of outer flags.
Of course it means that they need to be mutually exclusive.

> 
> In your patch there is no PKT_TX_OUTER_IPV4 flag.
> 
> Regards,
> Olivier

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/4] mbuf:add three TX offload flags and change three fields
  2014-11-28  9:36   ` Olivier MATZ
  2014-11-28 10:27     ` Ananyev, Konstantin
@ 2014-11-28 10:33     ` Liu, Jijiang
  2014-11-28 10:40       ` Ananyev, Konstantin
  1 sibling, 1 reply; 17+ messages in thread
From: Liu, Jijiang @ 2014-11-28 10:33 UTC (permalink / raw)
  To: Olivier MATZ, dev

Hi Olivier,

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Friday, November 28, 2014 5:37 PM
> To: Liu, Jijiang; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 1/4] mbuf:add three TX offload flags and
> change three fields
> 
> Hi Jijiang,
> 
> On 11/27/2014 06:03 PM, Jijiang Liu wrote:
> >  /** Tell the NIC it's an IPv4 packet. Required for L4 checksum offload or TSO.
> */
> >  #define PKT_TX_IPV4          PKT_RX_IPV4_HDR
> >
> >  /** Tell the NIC it's an IPv6 packet. Required for L4 checksum offload or TSO.
> */
> >  #define PKT_TX_IPV6          PKT_RX_IPV6_HDR
> 
> The description still does not match what we discussed. Either we have
> PKT_TX_IPV4 meaning "packet is IPv4 without requiring IP cksum offload", or
> "packet is IPv4". I prefer the second one, but whatever the choice is, the
> comments must be coherent.
>
I agree.
 "packet is IPv4" is ok for me, too.
The comment "Required for L4 checksum offload or TSO" is not added by me,  I should have thought you added it during developing TSO.
Anyway, we came to an agreement for  PKT_TX_IPV6/4 meaning, I will change  the two flags comments.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/4] mbuf:add three TX offload flags and change three fields
  2014-11-28 10:33     ` Liu, Jijiang
@ 2014-11-28 10:40       ` Ananyev, Konstantin
  2014-11-28 11:00         ` Olivier MATZ
  0 siblings, 1 reply; 17+ messages in thread
From: Ananyev, Konstantin @ 2014-11-28 10:40 UTC (permalink / raw)
  To: Liu, Jijiang, Olivier MATZ, dev



> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Liu, Jijiang
> Sent: Friday, November 28, 2014 10:33 AM
> To: Olivier MATZ; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 1/4] mbuf:add three TX offload flags and change three fields
> 
> Hi Olivier,
> 
> > -----Original Message-----
> > From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> > Sent: Friday, November 28, 2014 5:37 PM
> > To: Liu, Jijiang; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v3 1/4] mbuf:add three TX offload flags and
> > change three fields
> >
> > Hi Jijiang,
> >
> > On 11/27/2014 06:03 PM, Jijiang Liu wrote:
> > >  /** Tell the NIC it's an IPv4 packet. Required for L4 checksum offload or TSO.
> > */
> > >  #define PKT_TX_IPV4          PKT_RX_IPV4_HDR
> > >
> > >  /** Tell the NIC it's an IPv6 packet. Required for L4 checksum offload or TSO.
> > */
> > >  #define PKT_TX_IPV6          PKT_RX_IPV6_HDR
> >
> > The description still does not match what we discussed. Either we have
> > PKT_TX_IPV4 meaning "packet is IPv4 without requiring IP cksum offload", or
> > "packet is IPv4". I prefer the second one, but whatever the choice is, the
> > comments must be coherent.
> >
> I agree.
>  "packet is IPv4" is ok for me, too.
> The comment "Required for L4 checksum offload or TSO" is not added by me,  I should have thought you added it during developing
> TSO.
> Anyway, we came to an agreement for  PKT_TX_IPV6/4 meaning, I will change  the two flags comments.
> 
> 

Well, I still prefer them to be mutually exclusive.
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;
}

For the upper layer, I think there would be no big difference, what ways we will choose.

Konstantin
  

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/4] mbuf:add three TX offload flags and change three fields
  2014-11-28 10:40       ` Ananyev, Konstantin
@ 2014-11-28 11:00         ` Olivier MATZ
  2014-11-28 11:13           ` Ananyev, Konstantin
  0 siblings, 1 reply; 17+ messages in thread
From: Olivier MATZ @ 2014-11-28 11:00 UTC (permalink / raw)
  To: Ananyev, Konstantin, Liu, Jijiang, dev

Hi Konstantin,

On 11/28/2014 11:40 AM, Ananyev, Konstantin wrote:
> 
> Well, I still prefer them to be mutually exclusive.
> 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;
> }
> 
> For the upper layer, I think there would be no big difference, what ways we will choose.

I think the 2 informations are transversal, and that's why I would
prefer 2 flags. Also, having 2 separate flags would also help to keep
backward compatibility with previous versions.

It may help to have other points of view to make the good decision.
I'll follow the majority.

Regards,
Olivier

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/4] mbuf:add three TX offload flags and change three fields
  2014-11-28 11:00         ` Olivier MATZ
@ 2014-11-28 11:13           ` Ananyev, Konstantin
  2014-11-28 11:18             ` Olivier MATZ
  0 siblings, 1 reply; 17+ messages in thread
From: Ananyev, Konstantin @ 2014-11-28 11:13 UTC (permalink / raw)
  To: Olivier MATZ, Liu, Jijiang, dev




> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Friday, November 28, 2014 11:00 AM
> To: Ananyev, Konstantin; Liu, Jijiang; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 1/4] mbuf:add three TX offload flags and change three fields
> 
> Hi Konstantin,
> 
> On 11/28/2014 11:40 AM, Ananyev, Konstantin wrote:
> >
> > Well, I still prefer them to be mutually exclusive.
> > 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;
> > }
> >
> > For the upper layer, I think there would be no big difference, what ways we will choose.
> 
> I think the 2 informations are transversal, and that's why I would
> prefer 2 flags. Also, having 2 separate flags would also help to keep
> backward compatibility with previous versions.

Hmm, not sure how we will break compatibility in that case?
If we'll make TX_IP_CKSUM to be 1 bit value (1 << X) then for current drivers nothing should change, no?

> 
> It may help to have other points of view to make the good decision.
> I'll follow the majority.
> 
> Regards,
> Olivier

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/4] mbuf:add three TX offload flags and change three fields
  2014-11-28 11:13           ` Ananyev, Konstantin
@ 2014-11-28 11:18             ` Olivier MATZ
  2014-11-28 15:46               ` Ananyev, Konstantin
  0 siblings, 1 reply; 17+ messages in thread
From: Olivier MATZ @ 2014-11-28 11:18 UTC (permalink / raw)
  To: Ananyev, Konstantin, Liu, Jijiang, dev

Hi Konstantin,

On 11/28/2014 12:13 PM, Ananyev, Konstantin wrote:
>>> For the upper layer, I think there would be no big difference, what ways we will choose.
>>
>> I think the 2 informations are transversal, and that's why I would
>> prefer 2 flags. Also, having 2 separate flags would also help to keep
>> backward compatibility with previous versions.
> 
> Hmm, not sure how we will break compatibility in that case?
> If we'll make TX_IP_CKSUM to be 1 bit value (1 << X) then for current drivers nothing should change, no?

Yes, you're right, sorry.

Olivier

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/4] mbuf:add three TX offload flags and change three fields
  2014-11-28 11:18             ` Olivier MATZ
@ 2014-11-28 15:46               ` Ananyev, Konstantin
  0 siblings, 0 replies; 17+ messages in thread
From: Ananyev, Konstantin @ 2014-11-28 15:46 UTC (permalink / raw)
  To: Olivier MATZ, Liu, Jijiang, dev



> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Friday, November 28, 2014 11:19 AM
> To: Ananyev, Konstantin; Liu, Jijiang; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 1/4] mbuf:add three TX offload flags and change three fields
> 
> Hi Konstantin,
> 
> On 11/28/2014 12:13 PM, Ananyev, Konstantin wrote:
> >>> For the upper layer, I think there would be no big difference, what ways we will choose.
> >>
> >> I think the 2 informations are transversal, and that's why I would
> >> prefer 2 flags. Also, having 2 separate flags would also help to keep
> >> backward compatibility with previous versions.
> >
> > Hmm, not sure how we will break compatibility in that case?
> > If we'll make TX_IP_CKSUM to be 1 bit value (1 << X) then for current drivers nothing should change, no?
> 
> Yes, you're right, sorry.

Actually, you right - squeezing these 3 flags into 2 bits  would break backward compatibility.
Sorry, didn't think properly.
Konstantin

> 
> Olivier

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2014-11-28 15:46 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-27 17:03 [dpdk-dev] [PATCH v3 0/4] i40e VXLAN TX checksum rework Jijiang Liu
2014-11-27 17:03 ` [dpdk-dev] [PATCH v3 1/4] mbuf:add three TX offload flags and change three fields Jijiang Liu
2014-11-28  9:36   ` Olivier MATZ
2014-11-28 10:27     ` Ananyev, Konstantin
2014-11-28 10:33     ` Liu, Jijiang
2014-11-28 10:40       ` Ananyev, Konstantin
2014-11-28 11:00         ` Olivier MATZ
2014-11-28 11:13           ` Ananyev, Konstantin
2014-11-28 11:18             ` Olivier MATZ
2014-11-28 15:46               ` Ananyev, Konstantin
2014-11-27 17:03 ` [dpdk-dev] [PATCH v3 2/4] mbuf:change PKT_TX_IPV4 and PKT_TX_IPV6 definition Jijiang Liu
2014-11-28  9:37   ` Olivier MATZ
2014-11-27 17:03 ` [dpdk-dev] [PATCH v3 3/4] i40e:PMD change for VXLAN TX checksum Jijiang Liu
2014-11-27 17:03 ` [dpdk-dev] [PATCH v3 4/4] testpmd:rework csum forward engine Jijiang Liu
2014-11-28  9:50   ` Olivier MATZ
2014-11-28 10:10     ` Thomas Monjalon
2014-11-27 17:24 ` [dpdk-dev] [PATCH v3 0/4] i40e VXLAN TX checksum rework Ananyev, Konstantin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).