[dpdk-dev] [PATCH 0/4] vhost vlan tag and TSO fixes/cleanups

[dpdk-dev] [PATCH 0/4] vhost vlan tag and TSO fixes/cleanups

[Yuanhan Liu]

Ksiadz reported that TSO won't work for OVS with NIC, even with those
similar changes from the commit 9fd72e3cbd29 ("examples/vhost: add
virtio offload").

This gives me another chance to look at the TSO implementation a bit
deeper, and then came up with this small patch set, which moves some
left settings for enabling TSO to vhost lib.

With this patch set, an application can do mimimal (or even no)
changes to get the TSO capability. Take OVS as example, it just need
set MTU correctly and set the NIC port txq_flags properly to enable
NIC offloading ability, which is disabled by default for some drivers.

Patch 4 is a vlan tag fix reported by Qian.

---
Yuanhan Liu (4):
  vhost: remove unnecessary return
  vhost: complete TSO settings
  examples/vhost: remove unnessary settings for TX offload
  examples/vhost: fix wrong vlan_tag

 examples/vhost/main.c         | 64 +++----------------------------------------
 lib/librte_vhost/vhost_rxtx.c | 49 +++++++++++++++++++++++----------
 2 files changed, 39 insertions(+), 74 deletions(-)

-- 
1.9.0

[dpdk-dev] [PATCH 1/4] vhost: remove unnecessary return

[Yuanhan Liu]

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 lib/librte_vhost/vhost_rxtx.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
index b4da665..7d1224c 100644
--- a/lib/librte_vhost/vhost_rxtx.c
+++ b/lib/librte_vhost/vhost_rxtx.c
@@ -123,8 +123,6 @@ virtio_enqueue_offload(struct rte_mbuf *m_buf, struct virtio_net_hdr *net_hdr)
 		net_hdr->hdr_len = m_buf->l2_len + m_buf->l3_len
 					+ m_buf->l4_len;
 	}
-
-	return;
 }
 
 static inline void
-- 
1.9.0

[dpdk-dev] [PATCH 2/4] vhost: complete TSO settings

[Yuanhan Liu]

Commit d0cf91303d73 ("vhost: add Tx offload capabilities") has only
done partial settings for enabling TSO, and left the following part
to the application, say vhost-switch example, by commit 9fd72e3cbd29
("examples/vhost: add virtio offload").

- Setting PKT_TX_IP_CKSUM and ipv4_hdr->hdr_checksum = 0 for IPv4.

- calculate the pseudo header checksum without taking ip_len in
  account, and set it in the TCP header

Here we complete the left part in vhost side, so that an user (such
as OVS) can do minimal (or even no) changes to get TSO enabled.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 lib/librte_vhost/vhost_rxtx.c | 47 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 12 deletions(-)

diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
index 7d1224c..a204703 100644
--- a/lib/librte_vhost/vhost_rxtx.c
+++ b/lib/librte_vhost/vhost_rxtx.c
@@ -602,11 +602,11 @@ rte_vhost_enqueue_burst(struct virtio_net *dev, uint16_t queue_id,
 }
 
 static void
-parse_ethernet(struct rte_mbuf *m, uint16_t *l4_proto, void **l4_hdr)
+parse_ethernet(struct rte_mbuf *m, void **l3_hdr,
+	       void **l4_hdr, uint16_t *l4_proto)
 {
 	struct ipv4_hdr *ipv4_hdr;
 	struct ipv6_hdr *ipv6_hdr;
-	void *l3_hdr = NULL;
 	struct ether_hdr *eth_hdr;
 	uint16_t ethertype;
 
@@ -622,21 +622,19 @@ parse_ethernet(struct rte_mbuf *m, uint16_t *l4_proto, void **l4_hdr)
 		ethertype = rte_be_to_cpu_16(vlan_hdr->eth_proto);
 	}
 
-	l3_hdr = (char *)eth_hdr + m->l2_len;
+	*l3_hdr = (char *)eth_hdr + m->l2_len;
 
 	switch (ethertype) {
 	case ETHER_TYPE_IPv4:
-		ipv4_hdr = (struct ipv4_hdr *)l3_hdr;
+		ipv4_hdr  = *l3_hdr;
 		*l4_proto = ipv4_hdr->next_proto_id;
 		m->l3_len = (ipv4_hdr->version_ihl & 0x0f) * 4;
-		*l4_hdr = (char *)l3_hdr + m->l3_len;
 		m->ol_flags |= PKT_TX_IPV4;
 		break;
 	case ETHER_TYPE_IPv6:
-		ipv6_hdr = (struct ipv6_hdr *)l3_hdr;
+		ipv6_hdr = *l3_hdr;
 		*l4_proto = ipv6_hdr->proto;
 		m->l3_len = sizeof(struct ipv6_hdr);
-		*l4_hdr = (char *)l3_hdr + m->l3_len;
 		m->ol_flags |= PKT_TX_IPV6;
 		break;
 	default:
@@ -644,16 +642,28 @@ parse_ethernet(struct rte_mbuf *m, uint16_t *l4_proto, void **l4_hdr)
 		*l4_proto = 0;
 		break;
 	}
+
+	*l4_hdr = (char *)*l3_hdr + m->l3_len;
+}
+
+static uint16_t
+get_psd_sum(void *l3_hdr, uint64_t ol_flags)
+{
+	if (ol_flags & PKT_TX_IPV4)
+		return rte_ipv4_phdr_cksum(l3_hdr, ol_flags);
+	else
+		return rte_ipv6_phdr_cksum(l3_hdr, ol_flags);
 }
 
 static inline void __attribute__((always_inline))
 vhost_dequeue_offload(struct virtio_net_hdr *hdr, struct rte_mbuf *m)
 {
-	uint16_t l4_proto = 0;
-	void *l4_hdr = NULL;
-	struct tcp_hdr *tcp_hdr = NULL;
+	void *l3_hdr;
+	void *l4_hdr;
+	uint16_t l4_proto;
+
+	parse_ethernet(m, &l3_hdr, &l4_hdr, &l4_proto);
 
-	parse_ethernet(m, &l4_proto, &l4_hdr);
 	if (hdr->flags == VIRTIO_NET_HDR_F_NEEDS_CSUM) {
 		if (hdr->csum_start == (m->l2_len + m->l3_len)) {
 			switch (hdr->csum_offset) {
@@ -676,13 +686,26 @@ vhost_dequeue_offload(struct virtio_net_hdr *hdr, struct rte_mbuf *m)
 	}
 
 	if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
+		struct ipv4_hdr *ipv4_hdr = l3_hdr;
+		struct tcp_hdr *tcp_hdr   = l4_hdr;
+
 		switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
 		case VIRTIO_NET_HDR_GSO_TCPV4:
+			/*
+			 * According to comments for PKT_TX_TCP_SEG
+			 * at rte_mbuf.h, we need following settings
+			 * for IPv4.
+			 */
+			m->ol_flags |= PKT_TX_IP_CKSUM;
+			ipv4_hdr->hdr_checksum = 0;
+
+			/* Fall through */
 		case VIRTIO_NET_HDR_GSO_TCPV6:
-			tcp_hdr = (struct tcp_hdr *)l4_hdr;
 			m->ol_flags |= PKT_TX_TCP_SEG;
 			m->tso_segsz = hdr->gso_size;
 			m->l4_len = (tcp_hdr->data_off & 0xf0) >> 2;
+
+			tcp_hdr->cksum = get_psd_sum(l3_hdr, m->ol_flags);
 			break;
 		default:
 			RTE_LOG(WARNING, VHOST_DATA,
-- 
1.9.0

[dpdk-dev] [PATCH 3/4] examples/vhost: remove unnessary settings for TX offload

[Yuanhan Liu]

We now got all required settings to make TSO work at vhost lib.
We also don't need to calculate the pseudo header checksum just
for the checksum offloading case, as the TCP/IP stack would have
done that.

So, those settings are not necessary; remove them.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 examples/vhost/main.c | 58 ---------------------------------------------------
 1 file changed, 58 deletions(-)

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index a45cddb..ae1e110 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -51,9 +51,6 @@
 #include <rte_malloc.h>
 #include <rte_virtio_net.h>
 #include <rte_ip.h>
-#include <rte_tcp.h>
-#include <rte_udp.h>
-#include <rte_sctp.h>
 
 #include "main.h"
 
@@ -1147,58 +1144,6 @@ find_local_dest(struct virtio_net *dev, struct rte_mbuf *m,
 	return 0;
 }
 
-static uint16_t
-get_psd_sum(void *l3_hdr, uint64_t ol_flags)
-{
-	if (ol_flags & PKT_TX_IPV4)
-		return rte_ipv4_phdr_cksum(l3_hdr, ol_flags);
-	else /* assume ethertype == ETHER_TYPE_IPv6 */
-		return rte_ipv6_phdr_cksum(l3_hdr, ol_flags);
-}
-
-static void virtio_tx_offload(struct rte_mbuf *m)
-{
-	void *l3_hdr;
-	struct ipv4_hdr *ipv4_hdr = NULL;
-	struct tcp_hdr *tcp_hdr = NULL;
-	struct udp_hdr *udp_hdr = NULL;
-	struct sctp_hdr *sctp_hdr = NULL;
-	struct ether_hdr *eth_hdr = rte_pktmbuf_mtod(m, struct ether_hdr *);
-
-	l3_hdr = (char *)eth_hdr + m->l2_len;
-
-	if (m->tso_segsz != 0) {
-		ipv4_hdr = (struct ipv4_hdr *)l3_hdr;
-		tcp_hdr = (struct tcp_hdr *)((char *)l3_hdr + m->l3_len);
-		m->ol_flags |= PKT_TX_IP_CKSUM;
-		ipv4_hdr->hdr_checksum = 0;
-		tcp_hdr->cksum = get_psd_sum(l3_hdr, m->ol_flags);
-		return;
-	}
-
-	if (m->ol_flags & PKT_TX_L4_MASK) {
-		switch (m->ol_flags & PKT_TX_L4_MASK) {
-		case PKT_TX_TCP_CKSUM:
-			tcp_hdr = (struct tcp_hdr *)
-					((char *)l3_hdr + m->l3_len);
-			tcp_hdr->cksum = get_psd_sum(l3_hdr, m->ol_flags);
-			break;
-		case PKT_TX_UDP_CKSUM:
-			udp_hdr = (struct udp_hdr *)
-					((char *)l3_hdr + m->l3_len);
-			udp_hdr->dgram_cksum = get_psd_sum(l3_hdr, m->ol_flags);
-			break;
-		case PKT_TX_SCTP_CKSUM:
-			sctp_hdr = (struct sctp_hdr *)
-					((char *)l3_hdr + m->l3_len);
-			sctp_hdr->cksum = 0;
-			break;
-		default:
-			break;
-		}
-	}
-}
-
 /*
  * This function routes the TX packet to the correct interface. This may be a local device
  * or the physical port.
@@ -1265,9 +1210,6 @@ virtio_tx_route(struct vhost_dev *vdev, struct rte_mbuf *m, uint16_t vlan_tag)
 		m->vlan_tci = vlan_tag;
 	}
 
-	if ((m->ol_flags & PKT_TX_L4_MASK) || (m->ol_flags & PKT_TX_TCP_SEG))
-		virtio_tx_offload(m);
-
 	tx_q->m_table[len] = m;
 	len++;
 	if (enable_stats) {
-- 
1.9.0

[dpdk-dev] [PATCH 4/4] examples/vhost: fix wrong vlan_tag

[Yuanhan Liu]

While the last arg of virtio_tx_route() asks a vlan tag, we currently
feed it with device_fh, which is wrong. Fix it.

Fixes: 4796ad63ba1f ("examples/vhost: import userspace vhost application")

Reported-by: Qian Xu <qian.q.xu@intel.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 examples/vhost/main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index ae1e110..00ae0de 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -1364,8 +1364,10 @@ switch_worker(__attribute__((unused)) void *arg)
 							rte_pktmbuf_free(pkts_burst[--tx_count]);
 					}
 				}
-				for (i = 0; i < tx_count; ++i)
-					virtio_tx_route(vdev, pkts_burst[i], (uint16_t)dev->device_fh);
+				for (i = 0; i < tx_count; ++i) {
+					virtio_tx_route(vdev, pkts_burst[i],
+						vlan_tags[(uint16_t)dev->device_fh]);
+				}
 			}
 
 			/*move to the next device in the list*/
-- 
1.9.0

Re: [dpdk-dev] [PATCH 2/4] vhost: complete TSO settings

[Yuanhan Liu]

On Fri, Mar 25, 2016 at 02:01:32PM +0800, Yuanhan Liu wrote:
> Commit d0cf91303d73 ("vhost: add Tx offload capabilities") has only
> done partial settings for enabling TSO, and left the following part
> to the application, say vhost-switch example, by commit 9fd72e3cbd29
> ("examples/vhost: add virtio offload").
> 
> - Setting PKT_TX_IP_CKSUM and ipv4_hdr->hdr_checksum = 0 for IPv4.
> 
> - calculate the pseudo header checksum without taking ip_len in
>   account, and set it in the TCP header
> 
> Here we complete the left part in vhost side, so that an user (such
> as OVS) can do minimal (or even no) changes to get TSO enabled.
> 
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
...
> +			ipv4_hdr->hdr_checksum = 0;

Nah.. we can't do that here. This hurts VM2VM case badly.

Thanks Qian for letting me be aware of it.

	--yliu

[dpdk-dev] [PATCH v2 0/4] vhost vlan tag and TSO fixes/cleanups

[Yuanhan Liu]

v2: - we can't remove the left part of TSO settings to lib vhost, which
      hurts VM2VM performance badly.

Ksiadz reported that TSO won't work for OVS with NIC, even with those
similar changes from the commit 9fd72e3cbd29 ("examples/vhost: add
virtio offload").

This gives me another chance to look at the TSO implementation a bit
deeper, and then came up with this small patch set, which includes a
TSO cleanup and fix.

Patch 4 is a vlan tag fix reported from Qian.

---
Yuanhan Liu (4):
  vhost: remove unnecessary return
  examples/vhost: remove unnecessary pseudo checksum calc
  examples/vhost: fix offload settings
  examples/vhost: fix wrong vlan_tag

 examples/vhost/main.c         | 44 ++++++++++---------------------------------
 lib/librte_vhost/vhost_rxtx.c |  2 --
 2 files changed, 10 insertions(+), 36 deletions(-)

-- 
1.9.0

[dpdk-dev] [PATCH v2 1/4] vhost: remove unnecessary return

[Yuanhan Liu]

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 lib/librte_vhost/vhost_rxtx.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
index b4da665..7d1224c 100644
--- a/lib/librte_vhost/vhost_rxtx.c
+++ b/lib/librte_vhost/vhost_rxtx.c
@@ -123,8 +123,6 @@ virtio_enqueue_offload(struct rte_mbuf *m_buf, struct virtio_net_hdr *net_hdr)
 		net_hdr->hdr_len = m_buf->l2_len + m_buf->l3_len
 					+ m_buf->l4_len;
 	}
-
-	return;
 }
 
 static inline void
-- 
1.9.0

[dpdk-dev] [PATCH v2 2/4] examples/vhost: remove unnecessary pseudo checksum calc

[Yuanhan Liu]

For checksum offloading only case, the TCP/IP stack would
have calculated the pseudo checksum. Therefore, we don't
need to re-calculate it again here; remove it.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 examples/vhost/main.c | 41 ++++++-----------------------------------
 1 file changed, 6 insertions(+), 35 deletions(-)

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index a45cddb..ceabbce 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -52,8 +52,6 @@
 #include <rte_virtio_net.h>
 #include <rte_ip.h>
 #include <rte_tcp.h>
-#include <rte_udp.h>
-#include <rte_sctp.h>
 
 #include "main.h"
 
@@ -1161,42 +1159,15 @@ static void virtio_tx_offload(struct rte_mbuf *m)
 	void *l3_hdr;
 	struct ipv4_hdr *ipv4_hdr = NULL;
 	struct tcp_hdr *tcp_hdr = NULL;
-	struct udp_hdr *udp_hdr = NULL;
-	struct sctp_hdr *sctp_hdr = NULL;
 	struct ether_hdr *eth_hdr = rte_pktmbuf_mtod(m, struct ether_hdr *);
 
 	l3_hdr = (char *)eth_hdr + m->l2_len;
 
-	if (m->tso_segsz != 0) {
-		ipv4_hdr = (struct ipv4_hdr *)l3_hdr;
-		tcp_hdr = (struct tcp_hdr *)((char *)l3_hdr + m->l3_len);
-		m->ol_flags |= PKT_TX_IP_CKSUM;
-		ipv4_hdr->hdr_checksum = 0;
-		tcp_hdr->cksum = get_psd_sum(l3_hdr, m->ol_flags);
-		return;
-	}
-
-	if (m->ol_flags & PKT_TX_L4_MASK) {
-		switch (m->ol_flags & PKT_TX_L4_MASK) {
-		case PKT_TX_TCP_CKSUM:
-			tcp_hdr = (struct tcp_hdr *)
-					((char *)l3_hdr + m->l3_len);
-			tcp_hdr->cksum = get_psd_sum(l3_hdr, m->ol_flags);
-			break;
-		case PKT_TX_UDP_CKSUM:
-			udp_hdr = (struct udp_hdr *)
-					((char *)l3_hdr + m->l3_len);
-			udp_hdr->dgram_cksum = get_psd_sum(l3_hdr, m->ol_flags);
-			break;
-		case PKT_TX_SCTP_CKSUM:
-			sctp_hdr = (struct sctp_hdr *)
-					((char *)l3_hdr + m->l3_len);
-			sctp_hdr->cksum = 0;
-			break;
-		default:
-			break;
-		}
-	}
+	ipv4_hdr = (struct ipv4_hdr *)l3_hdr;
+	tcp_hdr = (struct tcp_hdr *)((char *)l3_hdr + m->l3_len);
+	m->ol_flags |= PKT_TX_IP_CKSUM;
+	ipv4_hdr->hdr_checksum = 0;
+	tcp_hdr->cksum = get_psd_sum(l3_hdr, m->ol_flags);
 }
 
 /*
@@ -1265,7 +1236,7 @@ virtio_tx_route(struct vhost_dev *vdev, struct rte_mbuf *m, uint16_t vlan_tag)
 		m->vlan_tci = vlan_tag;
 	}
 
-	if ((m->ol_flags & PKT_TX_L4_MASK) || (m->ol_flags & PKT_TX_TCP_SEG))
+	if (m->ol_flags & PKT_TX_TCP_SEG)
 		virtio_tx_offload(m);
 
 	tx_q->m_table[len] = m;
-- 
1.9.0

[dpdk-dev] [PATCH v2 3/4] examples/vhost: fix offload settings

[Yuanhan Liu]

Comments for PKT_TX_TCP_SEG at rte_mbuf says that we should only set
PKT_TX_IP_CKSUM and reset ip hdr checksum for IPv4:

  - if it's IPv4, set the PKT_TX_IP_CKSUM flag and write the IP checksum
    to 0 in the packet

Fixes: 9fd72e3cbd29 ("examples/vhost: add virtio offload")

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 examples/vhost/main.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index ceabbce..86e5c24 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -1163,10 +1163,13 @@ static void virtio_tx_offload(struct rte_mbuf *m)
 
 	l3_hdr = (char *)eth_hdr + m->l2_len;
 
-	ipv4_hdr = (struct ipv4_hdr *)l3_hdr;
+	if (m->ol_flags & PKT_TX_IPV4) {
+		ipv4_hdr = l3_hdr;
+		ipv4_hdr->hdr_checksum = 0;
+		m->ol_flags |= PKT_TX_IP_CKSUM;
+	}
+
 	tcp_hdr = (struct tcp_hdr *)((char *)l3_hdr + m->l3_len);
-	m->ol_flags |= PKT_TX_IP_CKSUM;
-	ipv4_hdr->hdr_checksum = 0;
 	tcp_hdr->cksum = get_psd_sum(l3_hdr, m->ol_flags);
 }
 
-- 
1.9.0

[dpdk-dev] [PATCH v2 4/4] examples/vhost: fix wrong vlan_tag

[Yuanhan Liu]

While the last arg of virtio_tx_route() asks a vlan tag, we currently
feed it with device_fh, which is wrong. Fix it.

Fixes: 4796ad63ba1f ("examples/vhost: import userspace vhost application")

Reported-by: Qian Xu <qian.q.xu@intel.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 examples/vhost/main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index 86e5c24..28c17af 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -1396,8 +1396,10 @@ switch_worker(__attribute__((unused)) void *arg)
 							rte_pktmbuf_free(pkts_burst[--tx_count]);
 					}
 				}
-				for (i = 0; i < tx_count; ++i)
-					virtio_tx_route(vdev, pkts_burst[i], (uint16_t)dev->device_fh);
+				for (i = 0; i < tx_count; ++i) {
+					virtio_tx_route(vdev, pkts_burst[i],
+						vlan_tags[(uint16_t)dev->device_fh]);
+				}
 			}
 
 			/*move to the next device in the list*/
-- 
1.9.0

Re: [dpdk-dev] [PATCH v2 0/4] vhost vlan tag and TSO fixes/cleanups

[Thomas Monjalon]

2016-03-25 15:58, Yuanhan Liu:
> v2: - we can't remove the left part of TSO settings to lib vhost, which
>       hurts VM2VM performance badly.
> 
> Ksiadz reported that TSO won't work for OVS with NIC, even with those
> similar changes from the commit 9fd72e3cbd29 ("examples/vhost: add
> virtio offload").
> 
> This gives me another chance to look at the TSO implementation a bit
> deeper, and then came up with this small patch set, which includes a
> TSO cleanup and fix.
> 
> Patch 4 is a vlan tag fix reported from Qian.

Applied, thanks