All of lore.kernel.org
 help / color / mirror / Atom feed
* [net-next PATCH 0/8] Fix Tunnel features and enable GSO partial for Mellanox adapters
@ 2016-04-25 18:30 Alexander Duyck
  2016-04-25 18:31 ` [net-next PATCH 1/8] net: Disable segmentation if checksumming is not supported Alexander Duyck
                   ` (8 more replies)
  0 siblings, 9 replies; 18+ messages in thread
From: Alexander Duyck @ 2016-04-25 18:30 UTC (permalink / raw)
  To: talal, netdev, davem, galp, ogerlitz, eranbe

This patch series is meant to allow us to get the best performance possible
for Mellanox ConnectX-3/4 adapters in terms of VXLAN tunnels.

The first few patches address issues I found when just trying to collect
performance numbers.  Specifically I was unable to get rates of any more
than 1 or 2 Mb/s if I was using a tunnel that ran over IPv6.  In addition I
found a few other items related to GSO_PARTIAL and the TSO_MANGLEID that
needed to be addressed.

The last 4 patches go through and enable GSO_PARTIAL for tunnels that have
an outer checksum enabled, and then enable IPv6 support where we can.  In
my tests I found that the mlx4 doesn't support outer IPv6 but the mlx5 did
so the code is updated to reflect that in the patches that enable IPv6
support.

---

Alexander Duyck (8):
      net: Disable segmentation if checksumming is not supported
      gso: Only allow GSO_PARTIAL if we can checksum the inner protocol
      net: Fix netdev_fix_features so that TSO_MANGLEID is only available with TSO
      vxlan: Add checksum check to the features check function
      mlx4: Add support for UDP tunnel segmentation with outer checksum offload
      mlx4: Add support for inner IPv6 checksum offloads and TSO
      mlx5e: Add support for UDP tunnel segmentation with outer checksum offload
      mlx5e: Fix IPv6 tunnel checksum offload


 drivers/net/ethernet/mellanox/mlx4/en_netdev.c    |   38 +++++++++++++++++----
 drivers/net/ethernet/mellanox/mlx4/en_tx.c        |   15 +++++++-
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c |   10 ++++--
 include/linux/if_ether.h                          |    5 +++
 include/net/vxlan.h                               |    4 ++
 net/core/dev.c                                    |    6 +++
 net/core/skbuff.c                                 |    6 ++-
 7 files changed, 67 insertions(+), 17 deletions(-)

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

* [net-next PATCH 1/8] net: Disable segmentation if checksumming is not supported
  2016-04-25 18:30 [net-next PATCH 0/8] Fix Tunnel features and enable GSO partial for Mellanox adapters Alexander Duyck
@ 2016-04-25 18:31 ` Alexander Duyck
  2016-04-25 18:31 ` [net-next PATCH 2/8] gso: Only allow GSO_PARTIAL if we can checksum the inner protocol Alexander Duyck
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 18+ messages in thread
From: Alexander Duyck @ 2016-04-25 18:31 UTC (permalink / raw)
  To: talal, netdev, davem, galp, ogerlitz, eranbe

In the case of the mlx4 and mlx5 driver they do not support IPv6 checksum
offload for tunnels.  With this being the case we should disable GSO in
addition to the checksum offload features when we find that a device cannot
perform a checksum on a given packet type.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 net/core/dev.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 6324bc9267f7..d6d9f286c4e1 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2815,7 +2815,7 @@ static netdev_features_t harmonize_features(struct sk_buff *skb,
 
 	if (skb->ip_summed != CHECKSUM_NONE &&
 	    !can_checksum_protocol(features, type)) {
-		features &= ~NETIF_F_CSUM_MASK;
+		features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
 	} else if (illegal_highdma(skb->dev, skb)) {
 		features &= ~NETIF_F_SG;
 	}

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

* [net-next PATCH 2/8] gso: Only allow GSO_PARTIAL if we can checksum the inner protocol
  2016-04-25 18:30 [net-next PATCH 0/8] Fix Tunnel features and enable GSO partial for Mellanox adapters Alexander Duyck
  2016-04-25 18:31 ` [net-next PATCH 1/8] net: Disable segmentation if checksumming is not supported Alexander Duyck
@ 2016-04-25 18:31 ` Alexander Duyck
  2016-04-25 18:31 ` [net-next PATCH 3/8] net: Fix netdev_fix_features so that TSO_MANGLEID is only available with TSO Alexander Duyck
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 18+ messages in thread
From: Alexander Duyck @ 2016-04-25 18:31 UTC (permalink / raw)
  To: talal, netdev, davem, galp, ogerlitz, eranbe

This patch addresses a possible issue that can occur if we get into any odd
corner cases where we support TSO for a given protocol but not the checksum
or scatter-gather offload.  There are few drivers floating around that
setup their tunnels this way and by enforcing the checksum piece we can
avoid mangling any frames.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 net/core/skbuff.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 7ff7788b0151..d2871d081750 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3080,8 +3080,7 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 	unsigned int headroom;
 	unsigned int len = head_skb->len;
 	__be16 proto;
-	bool csum;
-	int sg = !!(features & NETIF_F_SG);
+	bool csum, sg;
 	int nfrags = skb_shinfo(head_skb)->nr_frags;
 	int err = -ENOMEM;
 	int i = 0;
@@ -3093,13 +3092,14 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 	if (unlikely(!proto))
 		return ERR_PTR(-EINVAL);
 
+	sg = !!(features & NETIF_F_SG);
 	csum = !!can_checksum_protocol(features, proto);
 
 	/* GSO partial only requires that we trim off any excess that
 	 * doesn't fit into an MSS sized block, so take care of that
 	 * now.
 	 */
-	if (features & NETIF_F_GSO_PARTIAL) {
+	if (sg && csum && (features & NETIF_F_GSO_PARTIAL)) {
 		partial_segs = len / mss;
 		mss *= partial_segs;
 	}

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

* [net-next PATCH 3/8] net: Fix netdev_fix_features so that TSO_MANGLEID is only available with TSO
  2016-04-25 18:30 [net-next PATCH 0/8] Fix Tunnel features and enable GSO partial for Mellanox adapters Alexander Duyck
  2016-04-25 18:31 ` [net-next PATCH 1/8] net: Disable segmentation if checksumming is not supported Alexander Duyck
  2016-04-25 18:31 ` [net-next PATCH 2/8] gso: Only allow GSO_PARTIAL if we can checksum the inner protocol Alexander Duyck
@ 2016-04-25 18:31 ` Alexander Duyck
  2016-04-25 18:31 ` [net-next PATCH 4/8] vxlan: Add checksum check to the features check function Alexander Duyck
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 18+ messages in thread
From: Alexander Duyck @ 2016-04-25 18:31 UTC (permalink / raw)
  To: talal, netdev, davem, galp, ogerlitz, eranbe

This change makes it so that we will strip the TSO_MANGLEID bit if TSO is
not present.  This way we will also handle ECN correctly of TSO is not
present.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 net/core/dev.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index d6d9f286c4e1..6a5ef49ed1ab 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6720,6 +6720,10 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
 		features &= ~NETIF_F_TSO6;
 	}
 
+	/* TSO with IPv4 ID mangling requires IPv4 TSO be enabled */
+	if ((features & NETIF_F_TSO_MANGLEID) && !(features & NETIF_F_TSO))
+		features &= ~NETIF_F_TSO_MANGLEID;
+
 	/* TSO ECN requires that TSO is present as well. */
 	if ((features & NETIF_F_ALL_TSO) == NETIF_F_TSO_ECN)
 		features &= ~NETIF_F_TSO_ECN;

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

* [net-next PATCH 4/8] vxlan: Add checksum check to the features check function
  2016-04-25 18:30 [net-next PATCH 0/8] Fix Tunnel features and enable GSO partial for Mellanox adapters Alexander Duyck
                   ` (2 preceding siblings ...)
  2016-04-25 18:31 ` [net-next PATCH 3/8] net: Fix netdev_fix_features so that TSO_MANGLEID is only available with TSO Alexander Duyck
@ 2016-04-25 18:31 ` Alexander Duyck
  2016-04-25 18:31 ` [net-next PATCH 5/8] mlx4: Add support for UDP tunnel segmentation with outer checksum offload Alexander Duyck
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 18+ messages in thread
From: Alexander Duyck @ 2016-04-25 18:31 UTC (permalink / raw)
  To: talal, netdev, davem, galp, ogerlitz, eranbe

We need to perform an additional check on the inner headers to determine if
we can offload the checksum for them.  Previously this check didn't occur
so we would generate an invalid frame in the case of an IPv6 header
encapsulated inside of an IPv4 tunnel.  To fix this I added a secondary
check to vxlan_features_check so that we can verify that we can offload the
inner checksum.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 include/linux/if_ether.h |    5 +++++
 include/net/vxlan.h      |    4 +++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/linux/if_ether.h b/include/linux/if_ether.h
index d5569734f672..548fd535fd02 100644
--- a/include/linux/if_ether.h
+++ b/include/linux/if_ether.h
@@ -28,6 +28,11 @@ static inline struct ethhdr *eth_hdr(const struct sk_buff *skb)
 	return (struct ethhdr *)skb_mac_header(skb);
 }
 
+static inline struct ethhdr *inner_eth_hdr(const struct sk_buff *skb)
+{
+	return (struct ethhdr *)skb_inner_mac_header(skb);
+}
+
 int eth_header_parse(const struct sk_buff *skb, unsigned char *haddr);
 
 extern ssize_t sysfs_format_mac(char *buf, const unsigned char *addr, int len);
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index 673e9f9e6da7..b8803165df91 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -317,7 +317,9 @@ static inline netdev_features_t vxlan_features_check(struct sk_buff *skb,
 	    (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
 	     skb->inner_protocol != htons(ETH_P_TEB) ||
 	     (skb_inner_mac_header(skb) - skb_transport_header(skb) !=
-	      sizeof(struct udphdr) + sizeof(struct vxlanhdr))))
+	      sizeof(struct udphdr) + sizeof(struct vxlanhdr)) ||
+	     (skb->ip_summed != CHECKSUM_NONE &&
+	      !can_checksum_protocol(features, inner_eth_hdr(skb)->h_proto))))
 		return features & ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
 
 	return features;

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

* [net-next PATCH 5/8] mlx4: Add support for UDP tunnel segmentation with outer checksum offload
  2016-04-25 18:30 [net-next PATCH 0/8] Fix Tunnel features and enable GSO partial for Mellanox adapters Alexander Duyck
                   ` (3 preceding siblings ...)
  2016-04-25 18:31 ` [net-next PATCH 4/8] vxlan: Add checksum check to the features check function Alexander Duyck
@ 2016-04-25 18:31 ` Alexander Duyck
  2016-04-25 18:31 ` [net-next PATCH 6/8] mlx4: Add support for inner IPv6 checksum offloads and TSO Alexander Duyck
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 18+ messages in thread
From: Alexander Duyck @ 2016-04-25 18:31 UTC (permalink / raw)
  To: talal, netdev, davem, galp, ogerlitz, eranbe

This patch assumes that the mlx4 hardware will ignore existing IPv4/v6
header fields for length and checksum as well as the length and checksum
fields for outer UDP headers.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c |   17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 8bd143dda95d..bce37cbfde24 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -2358,7 +2358,9 @@ out:
 
 	/* set offloads */
 	priv->dev->hw_enc_features |= NETIF_F_IP_CSUM | NETIF_F_RXCSUM |
-				      NETIF_F_TSO | NETIF_F_GSO_UDP_TUNNEL;
+				      NETIF_F_TSO | NETIF_F_GSO_UDP_TUNNEL |
+				      NETIF_F_GSO_UDP_TUNNEL_CSUM |
+				      NETIF_F_GSO_PARTIAL;
 }
 
 static void mlx4_en_del_vxlan_offloads(struct work_struct *work)
@@ -2368,7 +2370,9 @@ static void mlx4_en_del_vxlan_offloads(struct work_struct *work)
 						 vxlan_del_task);
 	/* unset offloads */
 	priv->dev->hw_enc_features &= ~(NETIF_F_IP_CSUM | NETIF_F_RXCSUM |
-				      NETIF_F_TSO | NETIF_F_GSO_UDP_TUNNEL);
+					NETIF_F_TSO | NETIF_F_GSO_UDP_TUNNEL |
+					NETIF_F_GSO_UDP_TUNNEL_CSUM |
+					NETIF_F_GSO_PARTIAL);
 
 	ret = mlx4_SET_PORT_VXLAN(priv->mdev->dev, priv->port,
 				  VXLAN_STEER_BY_OUTER_MAC, 0);
@@ -2992,8 +2996,13 @@ int mlx4_en_init_netdev(struct mlx4_en_dev *mdev, int port,
 	}
 
 	if (mdev->dev->caps.tunnel_offload_mode == MLX4_TUNNEL_OFFLOAD_MODE_VXLAN) {
-		dev->hw_features |= NETIF_F_GSO_UDP_TUNNEL;
-		dev->features    |= NETIF_F_GSO_UDP_TUNNEL;
+		dev->hw_features |= NETIF_F_GSO_UDP_TUNNEL |
+				    NETIF_F_GSO_UDP_TUNNEL_CSUM |
+				    NETIF_F_GSO_PARTIAL;
+		dev->features    |= NETIF_F_GSO_UDP_TUNNEL |
+				    NETIF_F_GSO_UDP_TUNNEL_CSUM |
+				    NETIF_F_GSO_PARTIAL;
+		dev->gso_partial_features = NETIF_F_GSO_UDP_TUNNEL_CSUM;
 	}
 
 	mdev->pndev[port] = dev;

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

* [net-next PATCH 6/8] mlx4: Add support for inner IPv6 checksum offloads and TSO
  2016-04-25 18:30 [net-next PATCH 0/8] Fix Tunnel features and enable GSO partial for Mellanox adapters Alexander Duyck
                   ` (4 preceding siblings ...)
  2016-04-25 18:31 ` [net-next PATCH 5/8] mlx4: Add support for UDP tunnel segmentation with outer checksum offload Alexander Duyck
@ 2016-04-25 18:31 ` Alexander Duyck
  2016-04-26 14:37   ` Saeed Mahameed
  2016-04-25 18:31 ` [net-next PATCH 7/8] mlx5e: Add support for UDP tunnel segmentation with outer checksum offload Alexander Duyck
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 18+ messages in thread
From: Alexander Duyck @ 2016-04-25 18:31 UTC (permalink / raw)
  To: talal, netdev, davem, galp, ogerlitz, eranbe

>From what I can tell the ConnectX-3 will support an inner IPv6 checksum and
segmentation offload, however it cannot support outer IPv6 headers.  For
this reason I am adding the feature to the hw_enc_features and adding an
extra check to the features_check call that will disable GSO and checksum
offload in the case that the encapsulated frame has an outer IP version of
that is not 4.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c |   25 +++++++++++++++++++-----
 drivers/net/ethernet/mellanox/mlx4/en_tx.c     |   15 ++++++++++++--
 2 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index bce37cbfde24..6f28ac58251c 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -2357,8 +2357,10 @@ out:
 	}
 
 	/* set offloads */
-	priv->dev->hw_enc_features |= NETIF_F_IP_CSUM | NETIF_F_RXCSUM |
-				      NETIF_F_TSO | NETIF_F_GSO_UDP_TUNNEL |
+	priv->dev->hw_enc_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
+				      NETIF_F_RXCSUM |
+				      NETIF_F_TSO | NETIF_F_TSO6 |
+				      NETIF_F_GSO_UDP_TUNNEL |
 				      NETIF_F_GSO_UDP_TUNNEL_CSUM |
 				      NETIF_F_GSO_PARTIAL;
 }
@@ -2369,8 +2371,10 @@ static void mlx4_en_del_vxlan_offloads(struct work_struct *work)
 	struct mlx4_en_priv *priv = container_of(work, struct mlx4_en_priv,
 						 vxlan_del_task);
 	/* unset offloads */
-	priv->dev->hw_enc_features &= ~(NETIF_F_IP_CSUM | NETIF_F_RXCSUM |
-					NETIF_F_TSO | NETIF_F_GSO_UDP_TUNNEL |
+	priv->dev->hw_enc_features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
+					NETIF_F_RXCSUM |
+					NETIF_F_TSO | NETIF_F_TSO6 |
+					NETIF_F_GSO_UDP_TUNNEL |
 					NETIF_F_GSO_UDP_TUNNEL_CSUM |
 					NETIF_F_GSO_PARTIAL);
 
@@ -2431,7 +2435,18 @@ static netdev_features_t mlx4_en_features_check(struct sk_buff *skb,
 						netdev_features_t features)
 {
 	features = vlan_features_check(skb, features);
-	return vxlan_features_check(skb, features);
+	features = vxlan_features_check(skb, features);
+
+	/* The ConnectX-3 doesn't support outer IPv6 checksums but it does
+	 * support inner IPv6 checksums and segmentation so  we need to
+	 * strip that feature if this is an IPv6 encapsulated frame.
+	 */
+	if (skb->encapsulation &&
+	    (skb->ip_summed == CHECKSUM_PARTIAL) &&
+	    (ip_hdr(skb)->version != 4))
+		features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
+
+	return features;
 }
 #endif
 
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
index c0d7b7296236..c9f5388ea22a 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
@@ -41,6 +41,7 @@
 #include <linux/vmalloc.h>
 #include <linux/tcp.h>
 #include <linux/ip.h>
+#include <linux/ipv6.h>
 #include <linux/moduleparam.h>
 
 #include "mlx4_en.h"
@@ -918,8 +919,18 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 				 tx_ind, fragptr);
 
 	if (skb->encapsulation) {
-		struct iphdr *ipv4 = (struct iphdr *)skb_inner_network_header(skb);
-		if (ipv4->protocol == IPPROTO_TCP || ipv4->protocol == IPPROTO_UDP)
+		union {
+			struct iphdr *v4;
+			struct ipv6hdr *v6;
+			unsigned char *hdr;
+		} ip;
+		u8 proto;
+
+		ip.hdr = skb_inner_network_header(skb);
+		proto = (ip.v4->version == 4) ? ip.v4->protocol :
+						ip.v6->nexthdr;
+
+		if (proto == IPPROTO_TCP || proto == IPPROTO_UDP)
 			op_own |= cpu_to_be32(MLX4_WQE_CTRL_IIP | MLX4_WQE_CTRL_ILP);
 		else
 			op_own |= cpu_to_be32(MLX4_WQE_CTRL_IIP);

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

* [net-next PATCH 7/8] mlx5e: Add support for UDP tunnel segmentation with outer checksum offload
  2016-04-25 18:30 [net-next PATCH 0/8] Fix Tunnel features and enable GSO partial for Mellanox adapters Alexander Duyck
                   ` (5 preceding siblings ...)
  2016-04-25 18:31 ` [net-next PATCH 6/8] mlx4: Add support for inner IPv6 checksum offloads and TSO Alexander Duyck
@ 2016-04-25 18:31 ` Alexander Duyck
  2016-04-25 18:31 ` [net-next PATCH 8/8] mlx5e: Fix IPv6 tunnel " Alexander Duyck
  2016-04-29 17:31 ` [net-next PATCH 0/8] Fix Tunnel features and enable GSO partial for Mellanox adapters David Miller
  8 siblings, 0 replies; 18+ messages in thread
From: Alexander Duyck @ 2016-04-25 18:31 UTC (permalink / raw)
  To: talal, netdev, davem, galp, ogerlitz, eranbe

This patch assumes that the mlx5 hardware will ignore existing IPv4/v6
header fields for length and checksum as well as the length and checksum
fields for outer UDP headers.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index d485d1e4e100..a687d6624c8e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -2580,13 +2580,18 @@ static void mlx5e_build_netdev(struct net_device *netdev)
 	netdev->hw_features      |= NETIF_F_HW_VLAN_CTAG_FILTER;
 
 	if (mlx5e_vxlan_allowed(mdev)) {
-		netdev->hw_features     |= NETIF_F_GSO_UDP_TUNNEL;
+		netdev->hw_features     |= NETIF_F_GSO_UDP_TUNNEL |
+					   NETIF_F_GSO_UDP_TUNNEL_CSUM |
+					   NETIF_F_GSO_PARTIAL;
 		netdev->hw_enc_features |= NETIF_F_IP_CSUM;
 		netdev->hw_enc_features |= NETIF_F_RXCSUM;
 		netdev->hw_enc_features |= NETIF_F_TSO;
 		netdev->hw_enc_features |= NETIF_F_TSO6;
 		netdev->hw_enc_features |= NETIF_F_RXHASH;
 		netdev->hw_enc_features |= NETIF_F_GSO_UDP_TUNNEL;
+		netdev->hw_enc_features |= NETIF_F_GSO_UDP_TUNNEL_CSUM |
+					   NETIF_F_GSO_PARTIAL;
+		netdev->gso_partial_features = NETIF_F_GSO_UDP_TUNNEL_CSUM;
 	}
 
 	netdev->features          = netdev->hw_features;

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

* [net-next PATCH 8/8] mlx5e: Fix IPv6 tunnel checksum offload
  2016-04-25 18:30 [net-next PATCH 0/8] Fix Tunnel features and enable GSO partial for Mellanox adapters Alexander Duyck
                   ` (6 preceding siblings ...)
  2016-04-25 18:31 ` [net-next PATCH 7/8] mlx5e: Add support for UDP tunnel segmentation with outer checksum offload Alexander Duyck
@ 2016-04-25 18:31 ` Alexander Duyck
  2016-04-29 17:31 ` [net-next PATCH 0/8] Fix Tunnel features and enable GSO partial for Mellanox adapters David Miller
  8 siblings, 0 replies; 18+ messages in thread
From: Alexander Duyck @ 2016-04-25 18:31 UTC (permalink / raw)
  To: talal, netdev, davem, galp, ogerlitz, eranbe

The mlx5 driver exposes support for TSO6 but not IPv6 csum for hardware
encapsulated tunnels.  This leads to issues as it triggers warnings in
skb_checksum_help as it ends up being called as we report supporting the
segmentation but not the checksumming for IPv6 frames.

This patch corrects that and drops 2 features that don't actually need to
be supported in hw_enc_features since they are Rx features and don't
actually impact anything by being present in hw_enc_features.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index a687d6624c8e..4728abd46cb2 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -2584,10 +2584,9 @@ static void mlx5e_build_netdev(struct net_device *netdev)
 					   NETIF_F_GSO_UDP_TUNNEL_CSUM |
 					   NETIF_F_GSO_PARTIAL;
 		netdev->hw_enc_features |= NETIF_F_IP_CSUM;
-		netdev->hw_enc_features |= NETIF_F_RXCSUM;
+		netdev->hw_enc_features |= NETIF_F_IPV6_CSUM;
 		netdev->hw_enc_features |= NETIF_F_TSO;
 		netdev->hw_enc_features |= NETIF_F_TSO6;
-		netdev->hw_enc_features |= NETIF_F_RXHASH;
 		netdev->hw_enc_features |= NETIF_F_GSO_UDP_TUNNEL;
 		netdev->hw_enc_features |= NETIF_F_GSO_UDP_TUNNEL_CSUM |
 					   NETIF_F_GSO_PARTIAL;

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

* Re: [net-next PATCH 6/8] mlx4: Add support for inner IPv6 checksum offloads and TSO
  2016-04-25 18:31 ` [net-next PATCH 6/8] mlx4: Add support for inner IPv6 checksum offloads and TSO Alexander Duyck
@ 2016-04-26 14:37   ` Saeed Mahameed
  2016-04-26 15:50     ` Alex Duyck
  0 siblings, 1 reply; 18+ messages in thread
From: Saeed Mahameed @ 2016-04-26 14:37 UTC (permalink / raw)
  To: Alexander Duyck, talal, netdev, davem, galp, ogerlitz, eranbe



On 4/25/2016 9:31 PM, Alexander Duyck wrote:
> >From what I can tell the ConnectX-3 will support an inner IPv6 checksum and
> segmentation offload, however it cannot support outer IPv6 headers.  For
> this reason I am adding the feature to the hw_enc_features and adding an
> extra check to the features_check call that will disable GSO and checksum
> offload in the case that the encapsulated frame has an outer IP version of
> that is not 4.

Hi Alex,

Can you share the testing commands of running vxlan over IPv6 and what 
exactly didn't work for you ?
we would like to test this in house and understand what went wrong, 
theoretically there shouldn't be a difference between IPv6/IPv4 outer
checksum offloading in ConnectX-3.

Anyway, I suspect it might be related to a driver bug most likely in 
get_real_size function @en_tx.c
specifically in : *lso_header_size = (skb_inner_transport_header(skb) - 
skb->data) + inner_tcp_hdrlen(skb);

will check this and get back to you.

for the mlx5 patches I will also go through them later today.

> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
> ---
>   drivers/net/ethernet/mellanox/mlx4/en_netdev.c |   25 +++++++++++++++++++-----
>   drivers/net/ethernet/mellanox/mlx4/en_tx.c     |   15 ++++++++++++--
>   2 files changed, 33 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> index bce37cbfde24..6f28ac58251c 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> @@ -2357,8 +2357,10 @@ out:
>   	}
>   
>   	/* set offloads */
> -	priv->dev->hw_enc_features |= NETIF_F_IP_CSUM | NETIF_F_RXCSUM |
> -				      NETIF_F_TSO | NETIF_F_GSO_UDP_TUNNEL |
> +	priv->dev->hw_enc_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> +				      NETIF_F_RXCSUM |
> +				      NETIF_F_TSO | NETIF_F_TSO6 |
> +				      NETIF_F_GSO_UDP_TUNNEL |
>   				      NETIF_F_GSO_UDP_TUNNEL_CSUM |
>   				      NETIF_F_GSO_PARTIAL;
>   }
> @@ -2369,8 +2371,10 @@ static void mlx4_en_del_vxlan_offloads(struct work_struct *work)
>   	struct mlx4_en_priv *priv = container_of(work, struct mlx4_en_priv,
>   						 vxlan_del_task);
>   	/* unset offloads */
> -	priv->dev->hw_enc_features &= ~(NETIF_F_IP_CSUM | NETIF_F_RXCSUM |
> -					NETIF_F_TSO | NETIF_F_GSO_UDP_TUNNEL |
> +	priv->dev->hw_enc_features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> +					NETIF_F_RXCSUM |
> +					NETIF_F_TSO | NETIF_F_TSO6 |
> +					NETIF_F_GSO_UDP_TUNNEL |
>   					NETIF_F_GSO_UDP_TUNNEL_CSUM |
>   					NETIF_F_GSO_PARTIAL);
>   
> @@ -2431,7 +2435,18 @@ static netdev_features_t mlx4_en_features_check(struct sk_buff *skb,
>   						netdev_features_t features)
>   {
>   	features = vlan_features_check(skb, features);
> -	return vxlan_features_check(skb, features);
> +	features = vxlan_features_check(skb, features);
> +
> +	/* The ConnectX-3 doesn't support outer IPv6 checksums but it does
> +	 * support inner IPv6 checksums and segmentation so  we need to
> +	 * strip that feature if this is an IPv6 encapsulated frame.
> +	 */
> +	if (skb->encapsulation &&
> +	    (skb->ip_summed == CHECKSUM_PARTIAL) &&
> +	    (ip_hdr(skb)->version != 4))
> +		features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
Dejavu, didn't you fix this already in harmonize_features, in
i.e, it is enough to do here:

if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL))
             features &= ~NETIF_F_IPV6_CSUM;


> +
> +	return features;
>   }
>   #endif
>   
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> index c0d7b7296236..c9f5388ea22a 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> @@ -41,6 +41,7 @@
>   #include <linux/vmalloc.h>
>   #include <linux/tcp.h>
>   #include <linux/ip.h>
> +#include <linux/ipv6.h>
>   #include <linux/moduleparam.h>
>   
>   #include "mlx4_en.h"
> @@ -918,8 +919,18 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
>   				 tx_ind, fragptr);
>   
>   	if (skb->encapsulation) {
> -		struct iphdr *ipv4 = (struct iphdr *)skb_inner_network_header(skb);
> -		if (ipv4->protocol == IPPROTO_TCP || ipv4->protocol == IPPROTO_UDP)
> +		union {
> +			struct iphdr *v4;
> +			struct ipv6hdr *v6;
> +			unsigned char *hdr;
> +		} ip;
> +		u8 proto;
> +
> +		ip.hdr = skb_inner_network_header(skb);
> +		proto = (ip.v4->version == 4) ? ip.v4->protocol :
> +						ip.v6->nexthdr;
> +
> +		if (proto == IPPROTO_TCP || proto == IPPROTO_UDP)
>   			op_own |= cpu_to_be32(MLX4_WQE_CTRL_IIP | MLX4_WQE_CTRL_ILP);
>   		else
>   			op_own |= cpu_to_be32(MLX4_WQE_CTRL_IIP);

basically this is a bug fix, I don't know why the original author 
assumed it will be ipv4 !

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

* Re: [net-next PATCH 6/8] mlx4: Add support for inner IPv6 checksum offloads and TSO
  2016-04-26 14:37   ` Saeed Mahameed
@ 2016-04-26 15:50     ` Alex Duyck
  2016-04-26 20:23       ` Saeed Mahameed
  0 siblings, 1 reply; 18+ messages in thread
From: Alex Duyck @ 2016-04-26 15:50 UTC (permalink / raw)
  To: Saeed Mahameed
  Cc: talal, Linux Kernel Network Developers, David Miller, galp,
	ogerlitz, Eran Ben Elisha

On Tue, Apr 26, 2016 at 7:37 AM, Saeed Mahameed
<saeedm@dev.mellanox.co.il> wrote:
>
>
> On 4/25/2016 9:31 PM, Alexander Duyck wrote:
>>
>> >From what I can tell the ConnectX-3 will support an inner IPv6 checksum
>> and
>> segmentation offload, however it cannot support outer IPv6 headers.  For
>> this reason I am adding the feature to the hw_enc_features and adding an
>> extra check to the features_check call that will disable GSO and checksum
>> offload in the case that the encapsulated frame has an outer IP version of
>> that is not 4.
>
>
> Hi Alex,
>
> Can you share the testing commands of running vxlan over IPv6 and what
> exactly didn't work for you ?
> we would like to test this in house and understand what went wrong,
> theoretically there shouldn't be a difference between IPv6/IPv4 outer
> checksum offloading in ConnectX-3.

The setup is pretty straight forward.  Basically I left the first port
in the default namespace and moved the second int a secondary
namespace referred to below as $netns.  I then assigned the IPv6
addresses fec0::10:1 and fec0::10:2. After that I ran the following:

        VXLAN=vx$net
        echo $VXLAN ${test_options[$i]}
        ip link add $VXLAN type vxlan id $net \
                local fec0::10:1 remote $addr6 dev $PF0 \
                ${test_options[$i]} dstport `expr 8800 + $net`
        ip netns exec $netns ip link add $VXLAN type vxlan id $net \
                                  local $addr6 remote fec0::10:1 dev $port \
                                  ${test_options[$i]} dstport `expr 8800 + $net`
        ifconfig $VXLAN 192.168.${net}.1/24
        ip netns exec $netns ifconfig $VXLAN 192.168.${net}.2/24


> Anyway, I suspect it might be related to a driver bug most likely in
> get_real_size function @en_tx.c
> specifically in : *lso_header_size = (skb_inner_transport_header(skb) -
> skb->data) + inner_tcp_hdrlen(skb);
>
> will check this and get back to you.

I'm not entirely convinced.  What I was seeing is t hat the hardware
itself was performing Rx checksum offload only on tunnels with an
outer IPv4 header and ignoring tunnels with an outer IPv6 header.

> for the mlx5 patches I will also go through them later today.

Thanks.

>
>> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
>> ---
>>   drivers/net/ethernet/mellanox/mlx4/en_netdev.c |   25
>> +++++++++++++++++++-----
>>   drivers/net/ethernet/mellanox/mlx4/en_tx.c     |   15 ++++++++++++--
>>   2 files changed, 33 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
>> b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
>> index bce37cbfde24..6f28ac58251c 100644
>> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
>> @@ -2357,8 +2357,10 @@ out:
>>         }
>>         /* set offloads */
>> -       priv->dev->hw_enc_features |= NETIF_F_IP_CSUM | NETIF_F_RXCSUM |
>> -                                     NETIF_F_TSO | NETIF_F_GSO_UDP_TUNNEL
>> |
>> +       priv->dev->hw_enc_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM
>> |
>> +                                     NETIF_F_RXCSUM |
>> +                                     NETIF_F_TSO | NETIF_F_TSO6 |
>> +                                     NETIF_F_GSO_UDP_TUNNEL |
>>                                       NETIF_F_GSO_UDP_TUNNEL_CSUM |
>>                                       NETIF_F_GSO_PARTIAL;
>>   }
>> @@ -2369,8 +2371,10 @@ static void mlx4_en_del_vxlan_offloads(struct
>> work_struct *work)
>>         struct mlx4_en_priv *priv = container_of(work, struct
>> mlx4_en_priv,
>>                                                  vxlan_del_task);
>>         /* unset offloads */
>> -       priv->dev->hw_enc_features &= ~(NETIF_F_IP_CSUM | NETIF_F_RXCSUM |
>> -                                       NETIF_F_TSO |
>> NETIF_F_GSO_UDP_TUNNEL |
>> +       priv->dev->hw_enc_features &= ~(NETIF_F_IP_CSUM |
>> NETIF_F_IPV6_CSUM |
>> +                                       NETIF_F_RXCSUM |
>> +                                       NETIF_F_TSO | NETIF_F_TSO6 |
>> +                                       NETIF_F_GSO_UDP_TUNNEL |
>>                                         NETIF_F_GSO_UDP_TUNNEL_CSUM |
>>                                         NETIF_F_GSO_PARTIAL);
>>   @@ -2431,7 +2435,18 @@ static netdev_features_t
>> mlx4_en_features_check(struct sk_buff *skb,
>>                                                 netdev_features_t
>> features)
>>   {
>>         features = vlan_features_check(skb, features);
>> -       return vxlan_features_check(skb, features);
>> +       features = vxlan_features_check(skb, features);
>> +
>> +       /* The ConnectX-3 doesn't support outer IPv6 checksums but it does
>> +        * support inner IPv6 checksums and segmentation so  we need to
>> +        * strip that feature if this is an IPv6 encapsulated frame.
>> +        */
>> +       if (skb->encapsulation &&
>> +           (skb->ip_summed == CHECKSUM_PARTIAL) &&
>> +           (ip_hdr(skb)->version != 4))
>> +               features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
>
> Dejavu, didn't you fix this already in harmonize_features, in
> i.e, it is enough to do here:
>
> if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL))
>             features &= ~NETIF_F_IPV6_CSUM;
>

So what this patch is doing is enabling an inner IPv6 header offloads.
Up above we set the NETIF_F_IPV6_CSUM bit and we want it to stay set
unless we have an outer IPv6 header because the inner headers may
still need that bit set.  If I did what you suggest it strips IPv6
checksum support for inner headers and if we have to use GSO partial I
ended up encountering some of the other bugs that I have fixed for GSO
partial where either sg or csum are not defined.

>
>> +
>> +       return features;
>>   }
>>   #endif
>>   diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> index c0d7b7296236..c9f5388ea22a 100644
>> --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> @@ -41,6 +41,7 @@
>>   #include <linux/vmalloc.h>
>>   #include <linux/tcp.h>
>>   #include <linux/ip.h>
>> +#include <linux/ipv6.h>
>>   #include <linux/moduleparam.h>
>>     #include "mlx4_en.h"
>> @@ -918,8 +919,18 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct
>> net_device *dev)
>>                                  tx_ind, fragptr);
>>         if (skb->encapsulation) {
>> -               struct iphdr *ipv4 = (struct iphdr
>> *)skb_inner_network_header(skb);
>> -               if (ipv4->protocol == IPPROTO_TCP || ipv4->protocol ==
>> IPPROTO_UDP)
>> +               union {
>> +                       struct iphdr *v4;
>> +                       struct ipv6hdr *v6;
>> +                       unsigned char *hdr;
>> +               } ip;
>> +               u8 proto;
>> +
>> +               ip.hdr = skb_inner_network_header(skb);
>> +               proto = (ip.v4->version == 4) ? ip.v4->protocol :
>> +                                               ip.v6->nexthdr;
>> +
>> +               if (proto == IPPROTO_TCP || proto == IPPROTO_UDP)
>>                         op_own |= cpu_to_be32(MLX4_WQE_CTRL_IIP |
>> MLX4_WQE_CTRL_ILP);
>>                 else
>>                         op_own |= cpu_to_be32(MLX4_WQE_CTRL_IIP);
>
>
> basically this is a bug fix, I don't know why the original author assumed it
> will be ipv4 !

Because the feature flags didn't allow it any other way.  I am adding
the NETIF_F_TSO6 and NETIF_F_IPV6_CSUM flags in hw_enc_features and so
situations such as this couldn't be encountered until you start adding
those flags.

- Alex

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

* Re: [net-next PATCH 6/8] mlx4: Add support for inner IPv6 checksum offloads and TSO
  2016-04-26 15:50     ` Alex Duyck
@ 2016-04-26 20:23       ` Saeed Mahameed
  2016-04-26 21:01         ` Alexander Duyck
  0 siblings, 1 reply; 18+ messages in thread
From: Saeed Mahameed @ 2016-04-26 20:23 UTC (permalink / raw)
  To: Alex Duyck
  Cc: Tal Alon, Linux Kernel Network Developers, David Miller,
	Gal Pressman, Or Gerlitz, Eran Ben Elisha

On Tue, Apr 26, 2016 at 6:50 PM, Alex Duyck <aduyck@mirantis.com> wrote:
>
> The setup is pretty straight forward.  Basically I left the first port
> in the default namespace and moved the second int a secondary
> namespace referred to below as $netns.  I then assigned the IPv6
> addresses fec0::10:1 and fec0::10:2. After that I ran the following:
>
>         VXLAN=vx$net
>         echo $VXLAN ${test_options[$i]}
>         ip link add $VXLAN type vxlan id $net \
>                 local fec0::10:1 remote $addr6 dev $PF0 \
>                 ${test_options[$i]} dstport `expr 8800 + $net`
>         ip netns exec $netns ip link add $VXLAN type vxlan id $net \
>                                   local $addr6 remote fec0::10:1 dev $port \
>                                   ${test_options[$i]} dstport `expr 8800 + $net`
>         ifconfig $VXLAN 192.168.${net}.1/24
>         ip netns exec $netns ifconfig $VXLAN 192.168.${net}.2/24
>

Thanks, indeed i see that GSO is not working with vxlan over IPv6 over
mlx5 device.
We will test out those patches on both mlx4 and mlx5, and debug mlx4
IPv6 issue you see.

>
>> Anyway, I suspect it might be related to a driver bug most likely in
>> get_real_size function @en_tx.c
>> specifically in : *lso_header_size = (skb_inner_transport_header(skb) -
>> skb->data) + inner_tcp_hdrlen(skb);
>>
>> will check this and get back to you.
>
> I'm not entirely convinced.  What I was seeing is t hat the hardware
> itself was performing Rx checksum offload only on tunnels with an
> outer IPv4 header and ignoring tunnels with an outer IPv6 header.

I don't get it, are you trying to say that the issue is in the RX side ?
what do you mean by ignoring ? Dropping ? or just not validating checksum ?
if so why would you disable GSO and IPv6 checksumming on TX ?

>>>   @@ -2431,7 +2435,18 @@ static netdev_features_t
>>> mlx4_en_features_check(struct sk_buff *skb,
>>>                                                 netdev_features_t
>>> features)
>>>   {
>>>         features = vlan_features_check(skb, features);
>>> -       return vxlan_features_check(skb, features);
>>> +       features = vxlan_features_check(skb, features);
>>> +
>>> +       /* The ConnectX-3 doesn't support outer IPv6 checksums but it does
>>> +        * support inner IPv6 checksums and segmentation so  we need to
>>> +        * strip that feature if this is an IPv6 encapsulated frame.
>>> +        */
>>> +       if (skb->encapsulation &&
>>> +           (skb->ip_summed == CHECKSUM_PARTIAL) &&
>>> +           (ip_hdr(skb)->version != 4))
>>> +               features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
>>
>> Dejavu, didn't you fix this already in harmonize_features, in
>> i.e, it is enough to do here:
>>
>> if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL))
>>             features &= ~NETIF_F_IPV6_CSUM;
>>
>
> So what this patch is doing is enabling an inner IPv6 header offloads.
> Up above we set the NETIF_F_IPV6_CSUM bit and we want it to stay set
> unless we have an outer IPv6 header because the inner headers may
> still need that bit set.  If I did what you suggest it strips IPv6
> checksum support for inner headers and if we have to use GSO partial I
> ended up encountering some of the other bugs that I have fixed for GSO
> partial where either sg or csum are not defined.
>

I see, you mean that you want to disable checksumming and GSO only for
packets with Outer(IPv6):Inner(X) and keep it in case for
Outer(IPv4):Inner(IPv6)
but i think it is weird that the driver decides to disable features it
didn't declare in first place (NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK)

Retry:

if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL) &&
    (ip_hdr(skb)->version != 4))
            features &= ~NETIF_F_IPV6_CSUM;

will this work ?

Anyway i prefer to debug the mlx4 issue first before we discuss the
best approach to disable checksumming & GSO for outer IPv6 in mlx4.

>>
>>> +
>>> +       return features;
>>>   }
>>>   #endif
>>>   diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>>> b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>>> index c0d7b7296236..c9f5388ea22a 100644
>>> --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>>> @@ -41,6 +41,7 @@
>>>   #include <linux/vmalloc.h>
>>>   #include <linux/tcp.h>
>>>   #include <linux/ip.h>
>>> +#include <linux/ipv6.h>
>>>   #include <linux/moduleparam.h>
>>>     #include "mlx4_en.h"
>>> @@ -918,8 +919,18 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct
>>> net_device *dev)
>>>                                  tx_ind, fragptr);
>>>         if (skb->encapsulation) {
>>> -               struct iphdr *ipv4 = (struct iphdr
>>> *)skb_inner_network_header(skb);
>>> -               if (ipv4->protocol == IPPROTO_TCP || ipv4->protocol ==
>>> IPPROTO_UDP)
>>> +               union {
>>> +                       struct iphdr *v4;
>>> +                       struct ipv6hdr *v6;
>>> +                       unsigned char *hdr;
>>> +               } ip;
>>> +               u8 proto;
>>> +
>>> +               ip.hdr = skb_inner_network_header(skb);
>>> +               proto = (ip.v4->version == 4) ? ip.v4->protocol :
>>> +                                               ip.v6->nexthdr;
>>> +
>>> +               if (proto == IPPROTO_TCP || proto == IPPROTO_UDP)
>>>                         op_own |= cpu_to_be32(MLX4_WQE_CTRL_IIP |
>>> MLX4_WQE_CTRL_ILP);
>>>                 else
>>>                         op_own |= cpu_to_be32(MLX4_WQE_CTRL_IIP);
>>
>>
>> basically this is a bug fix, I don't know why the original author assumed it
>> will be ipv4 !
>
> Because the feature flags didn't allow it any other way.  I am adding
> the NETIF_F_TSO6 and NETIF_F_IPV6_CSUM flags in hw_enc_features and so
> situations such as this couldn't be encountered until you start adding
> those flags.
>

True.

Thanks,
- Saeed

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

* Re: [net-next PATCH 6/8] mlx4: Add support for inner IPv6 checksum offloads and TSO
  2016-04-26 20:23       ` Saeed Mahameed
@ 2016-04-26 21:01         ` Alexander Duyck
  2016-04-27 15:39           ` Tariq Toukan
  0 siblings, 1 reply; 18+ messages in thread
From: Alexander Duyck @ 2016-04-26 21:01 UTC (permalink / raw)
  To: Saeed Mahameed
  Cc: Alex Duyck, Tal Alon, Linux Kernel Network Developers,
	David Miller, Gal Pressman, Or Gerlitz, Eran Ben Elisha

On Tue, Apr 26, 2016 at 1:23 PM, Saeed Mahameed
<saeedm@dev.mellanox.co.il> wrote:
> On Tue, Apr 26, 2016 at 6:50 PM, Alex Duyck <aduyck@mirantis.com> wrote:
>>
>> The setup is pretty straight forward.  Basically I left the first port
>> in the default namespace and moved the second int a secondary
>> namespace referred to below as $netns.  I then assigned the IPv6
>> addresses fec0::10:1 and fec0::10:2. After that I ran the following:
>>
>>         VXLAN=vx$net
>>         echo $VXLAN ${test_options[$i]}
>>         ip link add $VXLAN type vxlan id $net \
>>                 local fec0::10:1 remote $addr6 dev $PF0 \
>>                 ${test_options[$i]} dstport `expr 8800 + $net`
>>         ip netns exec $netns ip link add $VXLAN type vxlan id $net \
>>                                   local $addr6 remote fec0::10:1 dev $port \
>>                                   ${test_options[$i]} dstport `expr 8800 + $net`
>>         ifconfig $VXLAN 192.168.${net}.1/24
>>         ip netns exec $netns ifconfig $VXLAN 192.168.${net}.2/24
>>
>
> Thanks, indeed i see that GSO is not working with vxlan over IPv6 over
> mlx5 device.
> We will test out those patches on both mlx4 and mlx5, and debug mlx4
> IPv6 issue you see.
>
>>
>>> Anyway, I suspect it might be related to a driver bug most likely in
>>> get_real_size function @en_tx.c
>>> specifically in : *lso_header_size = (skb_inner_transport_header(skb) -
>>> skb->data) + inner_tcp_hdrlen(skb);
>>>
>>> will check this and get back to you.
>>
>> I'm not entirely convinced.  What I was seeing is t hat the hardware
>> itself was performing Rx checksum offload only on tunnels with an
>> outer IPv4 header and ignoring tunnels with an outer IPv6 header.
>
> I don't get it, are you trying to say that the issue is in the RX side ?
> what do you mean by ignoring ? Dropping ? or just not validating checksum ?
> if so why would you disable GSO and IPv6 checksumming on TX ?

I'm suspecting that whatever parsing logic exists in either the
hardware or firmware may not be configured to parse tunnels with outer
IPv6 headers.  The tell-tale sign is what occurs with an IPv6 based
tunnel with no outer checksum.  The hardware is not performing a
checksum on the inner headers so it reports it as a UDP frame with no
checksum to the stack which ends up preventing us from doing GRO.
That tells me that the hardware is not parsing IPv6 based tunnels on
Rx.  I am assuming that if the Rx side doesn't work then there is a
good chance that the Tx won't.

>>>>   @@ -2431,7 +2435,18 @@ static netdev_features_t
>>>> mlx4_en_features_check(struct sk_buff *skb,
>>>>                                                 netdev_features_t
>>>> features)
>>>>   {
>>>>         features = vlan_features_check(skb, features);
>>>> -       return vxlan_features_check(skb, features);
>>>> +       features = vxlan_features_check(skb, features);
>>>> +
>>>> +       /* The ConnectX-3 doesn't support outer IPv6 checksums but it does
>>>> +        * support inner IPv6 checksums and segmentation so  we need to
>>>> +        * strip that feature if this is an IPv6 encapsulated frame.
>>>> +        */
>>>> +       if (skb->encapsulation &&
>>>> +           (skb->ip_summed == CHECKSUM_PARTIAL) &&
>>>> +           (ip_hdr(skb)->version != 4))
>>>> +               features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
>>>
>>> Dejavu, didn't you fix this already in harmonize_features, in
>>> i.e, it is enough to do here:
>>>
>>> if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL))
>>>             features &= ~NETIF_F_IPV6_CSUM;
>>>
>>
>> So what this patch is doing is enabling an inner IPv6 header offloads.
>> Up above we set the NETIF_F_IPV6_CSUM bit and we want it to stay set
>> unless we have an outer IPv6 header because the inner headers may
>> still need that bit set.  If I did what you suggest it strips IPv6
>> checksum support for inner headers and if we have to use GSO partial I
>> ended up encountering some of the other bugs that I have fixed for GSO
>> partial where either sg or csum are not defined.
>>
>
> I see, you mean that you want to disable checksumming and GSO only for
> packets with Outer(IPv6):Inner(X) and keep it in case for
> Outer(IPv4):Inner(IPv6)
> but i think it is weird that the driver decides to disable features it
> didn't declare in first place (NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK)
>
> Retry:
>
> if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL) &&
>     (ip_hdr(skb)->version != 4))
>             features &= ~NETIF_F_IPV6_CSUM;
>
> will this work ?

Sort of.  All that would happen is that you would fall through to
harmonize_features where NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK gets
cleared.  I just figured I would short-cut things since we cannot
support inner checksum or any GSO offloads if the tunnel has an outer
IPv6 header.  In addition this happens to effectively be the same code
I am using in vxlan_features_check to disable things if we cannot
checksum a protocol so it should help to keep the code size smaller
for the function if the compiler is smart enough to coalesce similar
code.

> Anyway i prefer to debug the mlx4 issue first before we discuss the
> best approach to disable checksumming & GSO for outer IPv6 in mlx4.

The current code as-is already has it disabled.  All I am doing is
enabling IPv6 checksums for inner headers as it seems like it doesn't
work for outer headers.

>>>
>>>> +
>>>> +       return features;
>>>>   }
>>>>   #endif
>>>>   diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>>>> b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>>>> index c0d7b7296236..c9f5388ea22a 100644
>>>> --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>>>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>>>> @@ -41,6 +41,7 @@
>>>>   #include <linux/vmalloc.h>
>>>>   #include <linux/tcp.h>
>>>>   #include <linux/ip.h>
>>>> +#include <linux/ipv6.h>
>>>>   #include <linux/moduleparam.h>
>>>>     #include "mlx4_en.h"
>>>> @@ -918,8 +919,18 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct
>>>> net_device *dev)
>>>>                                  tx_ind, fragptr);
>>>>         if (skb->encapsulation) {
>>>> -               struct iphdr *ipv4 = (struct iphdr
>>>> *)skb_inner_network_header(skb);
>>>> -               if (ipv4->protocol == IPPROTO_TCP || ipv4->protocol ==
>>>> IPPROTO_UDP)
>>>> +               union {
>>>> +                       struct iphdr *v4;
>>>> +                       struct ipv6hdr *v6;
>>>> +                       unsigned char *hdr;
>>>> +               } ip;
>>>> +               u8 proto;
>>>> +
>>>> +               ip.hdr = skb_inner_network_header(skb);
>>>> +               proto = (ip.v4->version == 4) ? ip.v4->protocol :
>>>> +                                               ip.v6->nexthdr;
>>>> +
>>>> +               if (proto == IPPROTO_TCP || proto == IPPROTO_UDP)
>>>>                         op_own |= cpu_to_be32(MLX4_WQE_CTRL_IIP |
>>>> MLX4_WQE_CTRL_ILP);
>>>>                 else
>>>>                         op_own |= cpu_to_be32(MLX4_WQE_CTRL_IIP);
>>>
>>>
>>> basically this is a bug fix, I don't know why the original author assumed it
>>> will be ipv4 !
>>
>> Because the feature flags didn't allow it any other way.  I am adding
>> the NETIF_F_TSO6 and NETIF_F_IPV6_CSUM flags in hw_enc_features and so
>> situations such as this couldn't be encountered until you start adding
>> those flags.
>>
>
> True.
>
> Thanks,
> - Saeed

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

* Re: [net-next PATCH 6/8] mlx4: Add support for inner IPv6 checksum offloads and TSO
  2016-04-26 21:01         ` Alexander Duyck
@ 2016-04-27 15:39           ` Tariq Toukan
  2016-04-27 18:05             ` Alexander Duyck
  0 siblings, 1 reply; 18+ messages in thread
From: Tariq Toukan @ 2016-04-27 15:39 UTC (permalink / raw)
  To: Alexander Duyck, Saeed Mahameed
  Cc: Alex Duyck, Tal Alon, Linux Kernel Network Developers,
	David Miller, Gal Pressman, Or Gerlitz, Eran Ben Elisha



On 27/04/2016 12:01 AM, Alexander Duyck wrote:
> On Tue, Apr 26, 2016 at 1:23 PM, Saeed Mahameed
> <saeedm@dev.mellanox.co.il> wrote:
>> On Tue, Apr 26, 2016 at 6:50 PM, Alex Duyck <aduyck@mirantis.com> wrote:
>>> The setup is pretty straight forward.  Basically I left the first port
>>> in the default namespace and moved the second int a secondary
>>> namespace referred to below as $netns.  I then assigned the IPv6
>>> addresses fec0::10:1 and fec0::10:2. After that I ran the following:
>>>
>>>          VXLAN=vx$net
>>>          echo $VXLAN ${test_options[$i]}
>>>          ip link add $VXLAN type vxlan id $net \
>>>                  local fec0::10:1 remote $addr6 dev $PF0 \
>>>                  ${test_options[$i]} dstport `expr 8800 + $net`
>>>          ip netns exec $netns ip link add $VXLAN type vxlan id $net \
>>>                                    local $addr6 remote fec0::10:1 dev $port \
>>>                                    ${test_options[$i]} dstport `expr 8800 + $net`
>>>          ifconfig $VXLAN 192.168.${net}.1/24
>>>          ip netns exec $netns ifconfig $VXLAN 192.168.${net}.2/24
>>>
>> Thanks, indeed i see that GSO is not working with vxlan over IPv6 over
>> mlx5 device.
>> We will test out those patches on both mlx4 and mlx5, and debug mlx4
>> IPv6 issue you see.
>>
>>>> Anyway, I suspect it might be related to a driver bug most likely in
>>>> get_real_size function @en_tx.c
>>>> specifically in : *lso_header_size = (skb_inner_transport_header(skb) -
>>>> skb->data) + inner_tcp_hdrlen(skb);
>>>>
>>>> will check this and get back to you.
>>> I'm not entirely convinced.  What I was seeing is t hat the hardware
>>> itself was performing Rx checksum offload only on tunnels with an
>>> outer IPv4 header and ignoring tunnels with an outer IPv6 header.
>> I don't get it, are you trying to say that the issue is in the RX side ?
>> what do you mean by ignoring ? Dropping ? or just not validating checksum ?
>> if so why would you disable GSO and IPv6 checksumming on TX ?
> I'm suspecting that whatever parsing logic exists in either the
> hardware or firmware may not be configured to parse tunnels with outer
> IPv6 headers.  The tell-tale sign is what occurs with an IPv6 based
> tunnel with no outer checksum.  The hardware is not performing a
> checksum on the inner headers so it reports it as a UDP frame with no
> checksum to the stack which ends up preventing us from doing GRO.
> That tells me that the hardware is not parsing IPv6 based tunnels on
> Rx.  I am assuming that if the Rx side doesn't work then there is a
> good chance that the Tx won't.
>
>>>>>    @@ -2431,7 +2435,18 @@ static netdev_features_t
>>>>> mlx4_en_features_check(struct sk_buff *skb,
>>>>>                                                  netdev_features_t
>>>>> features)
>>>>>    {
>>>>>          features = vlan_features_check(skb, features);
>>>>> -       return vxlan_features_check(skb, features);
>>>>> +       features = vxlan_features_check(skb, features);
>>>>> +
>>>>> +       /* The ConnectX-3 doesn't support outer IPv6 checksums but it does
>>>>> +        * support inner IPv6 checksums and segmentation so  we need to
>>>>> +        * strip that feature if this is an IPv6 encapsulated frame.
>>>>> +        */
>>>>> +       if (skb->encapsulation &&
>>>>> +           (skb->ip_summed == CHECKSUM_PARTIAL) &&
>>>>> +           (ip_hdr(skb)->version != 4))
>>>>> +               features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
>>>> Dejavu, didn't you fix this already in harmonize_features, in
>>>> i.e, it is enough to do here:
>>>>
>>>> if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL))
>>>>              features &= ~NETIF_F_IPV6_CSUM;
>>>>
>>> So what this patch is doing is enabling an inner IPv6 header offloads.
>>> Up above we set the NETIF_F_IPV6_CSUM bit and we want it to stay set
>>> unless we have an outer IPv6 header because the inner headers may
>>> still need that bit set.  If I did what you suggest it strips IPv6
>>> checksum support for inner headers and if we have to use GSO partial I
>>> ended up encountering some of the other bugs that I have fixed for GSO
>>> partial where either sg or csum are not defined.
>>>
>> I see, you mean that you want to disable checksumming and GSO only for
>> packets with Outer(IPv6):Inner(X) and keep it in case for
>> Outer(IPv4):Inner(IPv6)
>> but i think it is weird that the driver decides to disable features it
>> didn't declare in first place (NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK)
>>
>> Retry:
>>
>> if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL) &&
>>      (ip_hdr(skb)->version != 4))
>>              features &= ~NETIF_F_IPV6_CSUM;
>>
>> will this work ?
> Sort of.  All that would happen is that you would fall through to
> harmonize_features where NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK gets
> cleared.  I just figured I would short-cut things since we cannot
> support inner checksum or any GSO offloads if the tunnel has an outer
> IPv6 header.  In addition this happens to effectively be the same code
> I am using in vxlan_features_check to disable things if we cannot
> checksum a protocol so it should help to keep the code size smaller
> for the function if the compiler is smart enough to coalesce similar
> code.
>
>> Anyway i prefer to debug the mlx4 issue first before we discuss the
>> best approach to disable checksumming & GSO for outer IPv6 in mlx4.
> The current code as-is already has it disabled.  All I am doing is
> enabling IPv6 checksums for inner headers as it seems like it doesn't
> work for outer headers.

Hi Alex,
I will be working on the mlx4 issue next week after the holidays.
I will check this offline in-house, without blocking the series.

Regards,
Tariq

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

* Re: [net-next PATCH 6/8] mlx4: Add support for inner IPv6 checksum offloads and TSO
  2016-04-27 15:39           ` Tariq Toukan
@ 2016-04-27 18:05             ` Alexander Duyck
  2016-04-28 13:26               ` Tariq Toukan
  0 siblings, 1 reply; 18+ messages in thread
From: Alexander Duyck @ 2016-04-27 18:05 UTC (permalink / raw)
  To: Tariq Toukan, Saeed Mahameed
  Cc: Alex Duyck, Tal Alon, Linux Kernel Network Developers,
	David Miller, Gal Pressman, Or Gerlitz, Eran Ben Elisha

On 04/27/2016 08:39 AM, Tariq Toukan wrote:
>
>
> On 27/04/2016 12:01 AM, Alexander Duyck wrote:
>> On Tue, Apr 26, 2016 at 1:23 PM, Saeed Mahameed
>> <saeedm@dev.mellanox.co.il> wrote:
>>> On Tue, Apr 26, 2016 at 6:50 PM, Alex Duyck <aduyck@mirantis.com> wrote:
>>>> The setup is pretty straight forward.  Basically I left the first port
>>>> in the default namespace and moved the second int a secondary
>>>> namespace referred to below as $netns.  I then assigned the IPv6
>>>> addresses fec0::10:1 and fec0::10:2. After that I ran the following:
>>>>
>>>>          VXLAN=vx$net
>>>>          echo $VXLAN ${test_options[$i]}
>>>>          ip link add $VXLAN type vxlan id $net \
>>>>                  local fec0::10:1 remote $addr6 dev $PF0 \
>>>>                  ${test_options[$i]} dstport `expr 8800 + $net`
>>>>          ip netns exec $netns ip link add $VXLAN type vxlan id $net \
>>>>                                    local $addr6 remote fec0::10:1
>>>> dev $port \
>>>>                                    ${test_options[$i]} dstport `expr
>>>> 8800 + $net`
>>>>          ifconfig $VXLAN 192.168.${net}.1/24
>>>>          ip netns exec $netns ifconfig $VXLAN 192.168.${net}.2/24
>>>>
>>> Thanks, indeed i see that GSO is not working with vxlan over IPv6 over
>>> mlx5 device.
>>> We will test out those patches on both mlx4 and mlx5, and debug mlx4
>>> IPv6 issue you see.
>>>
>>>>> Anyway, I suspect it might be related to a driver bug most likely in
>>>>> get_real_size function @en_tx.c
>>>>> specifically in : *lso_header_size =
>>>>> (skb_inner_transport_header(skb) -
>>>>> skb->data) + inner_tcp_hdrlen(skb);
>>>>>
>>>>> will check this and get back to you.
>>>> I'm not entirely convinced.  What I was seeing is t hat the hardware
>>>> itself was performing Rx checksum offload only on tunnels with an
>>>> outer IPv4 header and ignoring tunnels with an outer IPv6 header.
>>> I don't get it, are you trying to say that the issue is in the RX side ?
>>> what do you mean by ignoring ? Dropping ? or just not validating
>>> checksum ?
>>> if so why would you disable GSO and IPv6 checksumming on TX ?
>> I'm suspecting that whatever parsing logic exists in either the
>> hardware or firmware may not be configured to parse tunnels with outer
>> IPv6 headers.  The tell-tale sign is what occurs with an IPv6 based
>> tunnel with no outer checksum.  The hardware is not performing a
>> checksum on the inner headers so it reports it as a UDP frame with no
>> checksum to the stack which ends up preventing us from doing GRO.
>> That tells me that the hardware is not parsing IPv6 based tunnels on
>> Rx.  I am assuming that if the Rx side doesn't work then there is a
>> good chance that the Tx won't.
>>
>>>>>>    @@ -2431,7 +2435,18 @@ static netdev_features_t
>>>>>> mlx4_en_features_check(struct sk_buff *skb,
>>>>>>                                                  netdev_features_t
>>>>>> features)
>>>>>>    {
>>>>>>          features = vlan_features_check(skb, features);
>>>>>> -       return vxlan_features_check(skb, features);
>>>>>> +       features = vxlan_features_check(skb, features);
>>>>>> +
>>>>>> +       /* The ConnectX-3 doesn't support outer IPv6 checksums but
>>>>>> it does
>>>>>> +        * support inner IPv6 checksums and segmentation so  we
>>>>>> need to
>>>>>> +        * strip that feature if this is an IPv6 encapsulated frame.
>>>>>> +        */
>>>>>> +       if (skb->encapsulation &&
>>>>>> +           (skb->ip_summed == CHECKSUM_PARTIAL) &&
>>>>>> +           (ip_hdr(skb)->version != 4))
>>>>>> +               features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
>>>>> Dejavu, didn't you fix this already in harmonize_features, in
>>>>> i.e, it is enough to do here:
>>>>>
>>>>> if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL))
>>>>>              features &= ~NETIF_F_IPV6_CSUM;
>>>>>
>>>> So what this patch is doing is enabling an inner IPv6 header offloads.
>>>> Up above we set the NETIF_F_IPV6_CSUM bit and we want it to stay set
>>>> unless we have an outer IPv6 header because the inner headers may
>>>> still need that bit set.  If I did what you suggest it strips IPv6
>>>> checksum support for inner headers and if we have to use GSO partial I
>>>> ended up encountering some of the other bugs that I have fixed for GSO
>>>> partial where either sg or csum are not defined.
>>>>
>>> I see, you mean that you want to disable checksumming and GSO only for
>>> packets with Outer(IPv6):Inner(X) and keep it in case for
>>> Outer(IPv4):Inner(IPv6)
>>> but i think it is weird that the driver decides to disable features it
>>> didn't declare in first place (NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK)
>>>
>>> Retry:
>>>
>>> if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL) &&
>>>      (ip_hdr(skb)->version != 4))
>>>              features &= ~NETIF_F_IPV6_CSUM;
>>>
>>> will this work ?
>> Sort of.  All that would happen is that you would fall through to
>> harmonize_features where NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK gets
>> cleared.  I just figured I would short-cut things since we cannot
>> support inner checksum or any GSO offloads if the tunnel has an outer
>> IPv6 header.  In addition this happens to effectively be the same code
>> I am using in vxlan_features_check to disable things if we cannot
>> checksum a protocol so it should help to keep the code size smaller
>> for the function if the compiler is smart enough to coalesce similar
>> code.
>>
>>> Anyway i prefer to debug the mlx4 issue first before we discuss the
>>> best approach to disable checksumming & GSO for outer IPv6 in mlx4.
>> The current code as-is already has it disabled.  All I am doing is
>> enabling IPv6 checksums for inner headers as it seems like it doesn't
>> work for outer headers.
>
> Hi Alex,
> I will be working on the mlx4 issue next week after the holidays.
> I will check this offline in-house, without blocking the series.
>
> Regards,
> Tariq
>

If it helps below are the kind of results I am seeing with the patch 
series applied versus what is currently there.  The big problem areas 
are IPv6 over IPv4 tunnels, and IPv4 over IPv6 tunnels without checksums.

The breakdown below is bare-ip-version without tunnel, outer-inner with 
tunnel, or outer-csum-inner if a checksum is present on the outer UDP 
header.

After the series is applied you can see the v6 over v4 issues are 
addressed, and GSO partial has improved the performance for traffic over 
v4 tunnels with outer UDP checksums.

Throughput Throughput  Local Local   Result
            Units       CPU   Service Tag
                        Util  Demand
                        %
mlx4 - Before
-------------------------------------------------
26616.45   10^6bits/s  3.62  0.357   "v4"
26101.18   10^6bits/s  6.72  0.675   "v6"
22289.41   10^6bits/s  6.49  0.764   "v4-v4"
N/A - could not connect              "v4-v6"
12743.91   10^6bits/s  4.25  0.874   "v4-csum-v4"
N/A - could not connect              "v4-csum-v6"
0.69       10^6bits/s  0.66  2519.1  "v6-v4"
5924.47    10^6bits/s  4.23  1.871   "v6-v6"
10369.95   10^6bits/s  4.33  1.096   "v6-csum-v4"
10648.51   10^6bits/s  4.10  1.010   "v6-csum-v6"

mlx4 - After
-------------------------------------------------
26585.36   10^6bits/s  3.60  0.355   "v4"
26342.86   10^6bits/s  6.67  0.664   "v6"
22295.93   10^6bits/s  6.34  0.746   "v4-v4"
19977.24   10^6bits/s  6.04  0.793   "v4-v6"
22164.71   10^6bits/s  6.46  0.763   "v4-csum-v4"
19685.22   10^6bits/s  6.12  0.815   "v4-csum-v6"
6126.80    10^6bits/s  4.29  1.835   "v6-v4"
5793.53    10^6bits/s  4.24  1.917   "v6-v6"
10278.52   10^6bits/s  4.07  1.039   "v6-csum-v4"
10526.68   10^6bits/s  4.11  1.024   "v6-csum-v6"

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

* Re: [net-next PATCH 6/8] mlx4: Add support for inner IPv6 checksum offloads and TSO
  2016-04-27 18:05             ` Alexander Duyck
@ 2016-04-28 13:26               ` Tariq Toukan
  0 siblings, 0 replies; 18+ messages in thread
From: Tariq Toukan @ 2016-04-28 13:26 UTC (permalink / raw)
  To: Alexander Duyck, Saeed Mahameed
  Cc: Alex Duyck, Tal Alon, Linux Kernel Network Developers,
	David Miller, Gal Pressman, Or Gerlitz, Eran Ben Elisha



On 27/04/2016 9:05 PM, Alexander Duyck wrote:
> On 04/27/2016 08:39 AM, Tariq Toukan wrote:
>>
>>
>> On 27/04/2016 12:01 AM, Alexander Duyck wrote:
>>> On Tue, Apr 26, 2016 at 1:23 PM, Saeed Mahameed
>>> <saeedm@dev.mellanox.co.il> wrote:
>>>> On Tue, Apr 26, 2016 at 6:50 PM, Alex Duyck <aduyck@mirantis.com> 
>>>> wrote:
>>>>> The setup is pretty straight forward.  Basically I left the first 
>>>>> port
>>>>> in the default namespace and moved the second int a secondary
>>>>> namespace referred to below as $netns.  I then assigned the IPv6
>>>>> addresses fec0::10:1 and fec0::10:2. After that I ran the following:
>>>>>
>>>>>          VXLAN=vx$net
>>>>>          echo $VXLAN ${test_options[$i]}
>>>>>          ip link add $VXLAN type vxlan id $net \
>>>>>                  local fec0::10:1 remote $addr6 dev $PF0 \
>>>>>                  ${test_options[$i]} dstport `expr 8800 + $net`
>>>>>          ip netns exec $netns ip link add $VXLAN type vxlan id $net \
>>>>>                                    local $addr6 remote fec0::10:1
>>>>> dev $port \
>>>>>                                    ${test_options[$i]} dstport `expr
>>>>> 8800 + $net`
>>>>>          ifconfig $VXLAN 192.168.${net}.1/24
>>>>>          ip netns exec $netns ifconfig $VXLAN 192.168.${net}.2/24
>>>>>
>>>> Thanks, indeed i see that GSO is not working with vxlan over IPv6 over
>>>> mlx5 device.
>>>> We will test out those patches on both mlx4 and mlx5, and debug mlx4
>>>> IPv6 issue you see.
>>>>
>>>>>> Anyway, I suspect it might be related to a driver bug most likely in
>>>>>> get_real_size function @en_tx.c
>>>>>> specifically in : *lso_header_size =
>>>>>> (skb_inner_transport_header(skb) -
>>>>>> skb->data) + inner_tcp_hdrlen(skb);
>>>>>>
>>>>>> will check this and get back to you.
>>>>> I'm not entirely convinced.  What I was seeing is t hat the hardware
>>>>> itself was performing Rx checksum offload only on tunnels with an
>>>>> outer IPv4 header and ignoring tunnels with an outer IPv6 header.
>>>> I don't get it, are you trying to say that the issue is in the RX 
>>>> side ?
>>>> what do you mean by ignoring ? Dropping ? or just not validating
>>>> checksum ?
>>>> if so why would you disable GSO and IPv6 checksumming on TX ?
>>> I'm suspecting that whatever parsing logic exists in either the
>>> hardware or firmware may not be configured to parse tunnels with outer
>>> IPv6 headers.  The tell-tale sign is what occurs with an IPv6 based
>>> tunnel with no outer checksum.  The hardware is not performing a
>>> checksum on the inner headers so it reports it as a UDP frame with no
>>> checksum to the stack which ends up preventing us from doing GRO.
>>> That tells me that the hardware is not parsing IPv6 based tunnels on
>>> Rx.  I am assuming that if the Rx side doesn't work then there is a
>>> good chance that the Tx won't.
>>>
>>>>>>>    @@ -2431,7 +2435,18 @@ static netdev_features_t
>>>>>>> mlx4_en_features_check(struct sk_buff *skb,
>>>>>>> netdev_features_t
>>>>>>> features)
>>>>>>>    {
>>>>>>>          features = vlan_features_check(skb, features);
>>>>>>> -       return vxlan_features_check(skb, features);
>>>>>>> +       features = vxlan_features_check(skb, features);
>>>>>>> +
>>>>>>> +       /* The ConnectX-3 doesn't support outer IPv6 checksums but
>>>>>>> it does
>>>>>>> +        * support inner IPv6 checksums and segmentation so  we
>>>>>>> need to
>>>>>>> +        * strip that feature if this is an IPv6 encapsulated 
>>>>>>> frame.
>>>>>>> +        */
>>>>>>> +       if (skb->encapsulation &&
>>>>>>> +           (skb->ip_summed == CHECKSUM_PARTIAL) &&
>>>>>>> +           (ip_hdr(skb)->version != 4))
>>>>>>> +               features &= ~(NETIF_F_CSUM_MASK | 
>>>>>>> NETIF_F_GSO_MASK);
>>>>>> Dejavu, didn't you fix this already in harmonize_features, in
>>>>>> i.e, it is enough to do here:
>>>>>>
>>>>>> if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL))
>>>>>>              features &= ~NETIF_F_IPV6_CSUM;
>>>>>>
>>>>> So what this patch is doing is enabling an inner IPv6 header 
>>>>> offloads.
>>>>> Up above we set the NETIF_F_IPV6_CSUM bit and we want it to stay set
>>>>> unless we have an outer IPv6 header because the inner headers may
>>>>> still need that bit set.  If I did what you suggest it strips IPv6
>>>>> checksum support for inner headers and if we have to use GSO 
>>>>> partial I
>>>>> ended up encountering some of the other bugs that I have fixed for 
>>>>> GSO
>>>>> partial where either sg or csum are not defined.
>>>>>
>>>> I see, you mean that you want to disable checksumming and GSO only for
>>>> packets with Outer(IPv6):Inner(X) and keep it in case for
>>>> Outer(IPv4):Inner(IPv6)
>>>> but i think it is weird that the driver decides to disable features it
>>>> didn't declare in first place (NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK)
>>>>
>>>> Retry:
>>>>
>>>> if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL) &&
>>>>      (ip_hdr(skb)->version != 4))
>>>>              features &= ~NETIF_F_IPV6_CSUM;
>>>>
>>>> will this work ?
>>> Sort of.  All that would happen is that you would fall through to
>>> harmonize_features where NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK gets
>>> cleared.  I just figured I would short-cut things since we cannot
>>> support inner checksum or any GSO offloads if the tunnel has an outer
>>> IPv6 header.  In addition this happens to effectively be the same code
>>> I am using in vxlan_features_check to disable things if we cannot
>>> checksum a protocol so it should help to keep the code size smaller
>>> for the function if the compiler is smart enough to coalesce similar
>>> code.
>>>
>>>> Anyway i prefer to debug the mlx4 issue first before we discuss the
>>>> best approach to disable checksumming & GSO for outer IPv6 in mlx4.
>>> The current code as-is already has it disabled.  All I am doing is
>>> enabling IPv6 checksums for inner headers as it seems like it doesn't
>>> work for outer headers.
>>
>> Hi Alex,
>> I will be working on the mlx4 issue next week after the holidays.
>> I will check this offline in-house, without blocking the series.
>>
>> Regards,
>> Tariq
>>
>
> If it helps below are the kind of results I am seeing with the patch 
> series applied versus what is currently there.  The big problem areas 
> are IPv6 over IPv4 tunnels, and IPv4 over IPv6 tunnels without checksums.
>
> The breakdown below is bare-ip-version without tunnel, outer-inner 
> with tunnel, or outer-csum-inner if a checksum is present on the outer 
> UDP header.
>
> After the series is applied you can see the v6 over v4 issues are 
> addressed, and GSO partial has improved the performance for traffic 
> over v4 tunnels with outer UDP checksums.
>
> Throughput Throughput  Local Local   Result
>            Units       CPU   Service Tag
>                        Util  Demand
>                        %
> mlx4 - Before
> -------------------------------------------------
> 26616.45   10^6bits/s  3.62  0.357   "v4"
> 26101.18   10^6bits/s  6.72  0.675   "v6"
> 22289.41   10^6bits/s  6.49  0.764   "v4-v4"
> N/A - could not connect              "v4-v6"
> 12743.91   10^6bits/s  4.25  0.874   "v4-csum-v4"
> N/A - could not connect              "v4-csum-v6"
> 0.69       10^6bits/s  0.66  2519.1  "v6-v4"
> 5924.47    10^6bits/s  4.23  1.871   "v6-v6"
> 10369.95   10^6bits/s  4.33  1.096   "v6-csum-v4"
> 10648.51   10^6bits/s  4.10  1.010   "v6-csum-v6"
>
> mlx4 - After
> -------------------------------------------------
> 26585.36   10^6bits/s  3.60  0.355   "v4"
> 26342.86   10^6bits/s  6.67  0.664   "v6"
> 22295.93   10^6bits/s  6.34  0.746   "v4-v4"
> 19977.24   10^6bits/s  6.04  0.793   "v4-v6"
> 22164.71   10^6bits/s  6.46  0.763   "v4-csum-v4"
> 19685.22   10^6bits/s  6.12  0.815   "v4-csum-v6"
> 6126.80    10^6bits/s  4.29  1.835   "v6-v4"
> 5793.53    10^6bits/s  4.24  1.917   "v6-v6"
> 10278.52   10^6bits/s  4.07  1.039   "v6-csum-v4"
> 10526.68   10^6bits/s  4.11  1.024   "v6-csum-v6"

Yeah that can help in comparing our results and getting aligned to 
address the issues.
Thanks for sharing.

Regards,
Tariq

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

* Re: [net-next PATCH 0/8] Fix Tunnel features and enable GSO partial for Mellanox adapters
  2016-04-25 18:30 [net-next PATCH 0/8] Fix Tunnel features and enable GSO partial for Mellanox adapters Alexander Duyck
                   ` (7 preceding siblings ...)
  2016-04-25 18:31 ` [net-next PATCH 8/8] mlx5e: Fix IPv6 tunnel " Alexander Duyck
@ 2016-04-29 17:31 ` David Miller
  2016-04-29 17:32   ` Alex Duyck
  8 siblings, 1 reply; 18+ messages in thread
From: David Miller @ 2016-04-29 17:31 UTC (permalink / raw)
  To: aduyck; +Cc: talal, netdev, galp, ogerlitz, eranbe

From: Alexander Duyck <aduyck@mirantis.com>
Date: Mon, 25 Apr 2016 11:30:54 -0700

> This patch series is meant to allow us to get the best performance possible
> for Mellanox ConnectX-3/4 adapters in terms of VXLAN tunnels.

I'm going to mark this as "deferred" in patchwork, so Alex why don't you just
respin these and repost next week when you get final feedback from the Mellanox
folks?

THanks.

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

* Re: [net-next PATCH 0/8] Fix Tunnel features and enable GSO partial for Mellanox adapters
  2016-04-29 17:31 ` [net-next PATCH 0/8] Fix Tunnel features and enable GSO partial for Mellanox adapters David Miller
@ 2016-04-29 17:32   ` Alex Duyck
  0 siblings, 0 replies; 18+ messages in thread
From: Alex Duyck @ 2016-04-29 17:32 UTC (permalink / raw)
  To: David Miller
  Cc: talal, Linux Kernel Network Developers, galp, ogerlitz, Eran Ben Elisha

On Fri, Apr 29, 2016 at 10:31 AM, David Miller <davem@davemloft.net> wrote:
> From: Alexander Duyck <aduyck@mirantis.com>
> Date: Mon, 25 Apr 2016 11:30:54 -0700
>
>> This patch series is meant to allow us to get the best performance possible
>> for Mellanox ConnectX-3/4 adapters in terms of VXLAN tunnels.
>
> I'm going to mark this as "deferred" in patchwork, so Alex why don't you just
> respin these and repost next week when you get final feedback from the Mellanox
> folks?
>
> THanks.

Okay.  Will do.

Thanks.

- Alex

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

end of thread, other threads:[~2016-04-29 17:32 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-25 18:30 [net-next PATCH 0/8] Fix Tunnel features and enable GSO partial for Mellanox adapters Alexander Duyck
2016-04-25 18:31 ` [net-next PATCH 1/8] net: Disable segmentation if checksumming is not supported Alexander Duyck
2016-04-25 18:31 ` [net-next PATCH 2/8] gso: Only allow GSO_PARTIAL if we can checksum the inner protocol Alexander Duyck
2016-04-25 18:31 ` [net-next PATCH 3/8] net: Fix netdev_fix_features so that TSO_MANGLEID is only available with TSO Alexander Duyck
2016-04-25 18:31 ` [net-next PATCH 4/8] vxlan: Add checksum check to the features check function Alexander Duyck
2016-04-25 18:31 ` [net-next PATCH 5/8] mlx4: Add support for UDP tunnel segmentation with outer checksum offload Alexander Duyck
2016-04-25 18:31 ` [net-next PATCH 6/8] mlx4: Add support for inner IPv6 checksum offloads and TSO Alexander Duyck
2016-04-26 14:37   ` Saeed Mahameed
2016-04-26 15:50     ` Alex Duyck
2016-04-26 20:23       ` Saeed Mahameed
2016-04-26 21:01         ` Alexander Duyck
2016-04-27 15:39           ` Tariq Toukan
2016-04-27 18:05             ` Alexander Duyck
2016-04-28 13:26               ` Tariq Toukan
2016-04-25 18:31 ` [net-next PATCH 7/8] mlx5e: Add support for UDP tunnel segmentation with outer checksum offload Alexander Duyck
2016-04-25 18:31 ` [net-next PATCH 8/8] mlx5e: Fix IPv6 tunnel " Alexander Duyck
2016-04-29 17:31 ` [net-next PATCH 0/8] Fix Tunnel features and enable GSO partial for Mellanox adapters David Miller
2016-04-29 17:32   ` Alex Duyck

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.