[PATCH 00/10] Split UFO into v4 and v6 versions.

[PATCH 00/10] Split UFO into v4 and v6 versions.

[Vladislav Yasevich]

UFO support in the kernel applies to both IPv4 and IPv6 protocols
with the same device feature.  However some devices may not be able
to support one of the offloads.  For this we split the UFO offload
feature into 2 pieces.  NETIF_F_UFO now controlls the IPv4 part and
this series introduces NETIF_F_UFO6.

As a result of this work, we can now re-enable NETIF_F_UFO on
virtio_net devices and restore UDP over IPv4 performance for guests.
We also continue to support legacy guests that assume that UFO6
support included into UFO(4).

Without this work, migrating a guest to a 3.18 kernel fails.

Vladislav Yasevich (10):
  core: Split out UFO6 support
  net:  Correctly mark IPv6 UFO offload type.
  ovs: Enable handling of UFO6 packets.
  loopback: Turn on UFO6 support.
  veth: Enable UFO6 support.
  macvlan: Enable UFO6 support.
  s2io: Enable UFO6 support.
  tun: Re-uanble UFO support.
  macvtap: Re-enable UFO support
  Revert "drivers/net: Disable UFO through virtio"

 drivers/net/ethernet/neterion/s2io.c |  6 +++---
 drivers/net/loopback.c               |  4 ++--
 drivers/net/macvlan.c                |  2 +-
 drivers/net/macvtap.c                | 20 ++++++++++++++------
 drivers/net/tun.c                    | 26 ++++++++++++++------------
 drivers/net/veth.c                   |  2 +-
 drivers/net/virtio_net.c             | 24 ++++++++++--------------
 include/linux/netdev_features.h      |  7 +++++--
 include/linux/netdevice.h            |  1 +
 include/linux/skbuff.h               |  1 +
 net/core/dev.c                       | 35 +++++++++++++++++++----------------
 net/core/ethtool.c                   |  2 +-
 net/ipv6/ip6_offload.c               |  1 +
 net/ipv6/ip6_output.c                |  4 ++--
 net/ipv6/udp_offload.c               |  3 ++-
 net/mpls/mpls_gso.c                  |  1 +
 net/openvswitch/datapath.c           |  3 ++-
 net/openvswitch/flow.c               |  2 +-
 18 files changed, 81 insertions(+), 63 deletions(-)

-- 
1.9.3

[PATCH 01/10] core: Split out UFO6 support

[Vladislav Yasevich]

Split IPv6 support for UFO into its own feature similiar to TSO.
This will later allow us to re-enable UFO support for virtio-net
devices.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 include/linux/netdev_features.h |  7 +++++--
 include/linux/netdevice.h       |  1 +
 include/linux/skbuff.h          |  1 +
 net/core/dev.c                  | 35 +++++++++++++++++++----------------
 net/core/ethtool.c              |  2 +-
 5 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index dcfdecb..a078945 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -48,8 +48,9 @@ enum {
 	NETIF_F_GSO_UDP_TUNNEL_BIT,	/* ... UDP TUNNEL with TSO */
 	NETIF_F_GSO_UDP_TUNNEL_CSUM_BIT,/* ... UDP TUNNEL with TSO & CSUM */
 	NETIF_F_GSO_MPLS_BIT,		/* ... MPLS segmentation */
+	NETIF_F_UFO6_BIT,		/* ... UDPv6 fragmentation */
 	/**/NETIF_F_GSO_LAST =		/* last bit, see GSO_MASK */
-		NETIF_F_GSO_MPLS_BIT,
+		NETIF_F_UFO6_BIT,
 
 	NETIF_F_FCOE_CRC_BIT,		/* FCoE CRC32 */
 	NETIF_F_SCTP_CSUM_BIT,		/* SCTP checksum offload */
@@ -109,6 +110,7 @@ enum {
 #define NETIF_F_TSO_ECN		__NETIF_F(TSO_ECN)
 #define NETIF_F_TSO		__NETIF_F(TSO)
 #define NETIF_F_UFO		__NETIF_F(UFO)
+#define NETIF_F_UFO6		__NETIF_F(UFO6)
 #define NETIF_F_VLAN_CHALLENGED	__NETIF_F(VLAN_CHALLENGED)
 #define NETIF_F_RXFCS		__NETIF_F(RXFCS)
 #define NETIF_F_RXALL		__NETIF_F(RXALL)
@@ -141,7 +143,7 @@ enum {
 
 /* List of features with software fallbacks. */
 #define NETIF_F_GSO_SOFTWARE	(NETIF_F_TSO | NETIF_F_TSO_ECN | \
-				 NETIF_F_TSO6 | NETIF_F_UFO)
+				 NETIF_F_TSO6 | NETIF_F_UFO | NETIF_F_UFO6)
 
 #define NETIF_F_GEN_CSUM	NETIF_F_HW_CSUM
 #define NETIF_F_V4_CSUM		(NETIF_F_GEN_CSUM | NETIF_F_IP_CSUM)
@@ -149,6 +151,7 @@ enum {
 #define NETIF_F_ALL_CSUM	(NETIF_F_V4_CSUM | NETIF_F_V6_CSUM)
 
 #define NETIF_F_ALL_TSO 	(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_TSO_ECN)
+#define NETIF_F_ALL_UFO		(NETIF_F_UFO | NETIF_F_UFO6)
 
 #define NETIF_F_ALL_FCOE	(NETIF_F_FCOE_CRC | NETIF_F_FCOE_MTU | \
 				 NETIF_F_FSO)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 74fd5d3..86af10a 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3559,6 +3559,7 @@ static inline bool net_gso_ok(netdev_features_t features, int gso_type)
 	/* check flags correspondence */
 	BUILD_BUG_ON(SKB_GSO_TCPV4   != (NETIF_F_TSO >> NETIF_F_GSO_SHIFT));
 	BUILD_BUG_ON(SKB_GSO_UDP     != (NETIF_F_UFO >> NETIF_F_GSO_SHIFT));
+	BUILD_BUG_ON(SKB_GSO_UDP6    != (NETIF_F_UFO6 >> NETIF_F_GSO_SHIFT));
 	BUILD_BUG_ON(SKB_GSO_DODGY   != (NETIF_F_GSO_ROBUST >> NETIF_F_GSO_SHIFT));
 	BUILD_BUG_ON(SKB_GSO_TCP_ECN != (NETIF_F_TSO_ECN >> NETIF_F_GSO_SHIFT));
 	BUILD_BUG_ON(SKB_GSO_TCPV6   != (NETIF_F_TSO6 >> NETIF_F_GSO_SHIFT));
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 6c8b6f6..8538b67 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -372,6 +372,7 @@ enum {
 
 	SKB_GSO_MPLS = 1 << 12,
 
+	SKB_GSO_UDP6 = 1 << 13
 };
 
 #if BITS_PER_LONG > 32
diff --git a/net/core/dev.c b/net/core/dev.c
index 945bbd0..fa4d2ee 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5929,6 +5929,12 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
 		features &= ~NETIF_F_ALL_TSO;
 	}
 
+	/* UFO requires that SG is present as well */
+	if ((features & NETIF_F_ALL_UFO) && !(features & NETIF_F_SG)) {
+		netdev_dbg(dev, "Dropping UFO features since no SG feature.\n");
+		features &= ~NETIF_F_ALL_UFO;
+	}
+
 	if ((features & NETIF_F_TSO) && !(features & NETIF_F_HW_CSUM) &&
 					!(features & NETIF_F_IP_CSUM)) {
 		netdev_dbg(dev, "Dropping TSO features since no CSUM feature.\n");
@@ -5952,24 +5958,21 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
 		features &= ~NETIF_F_GSO;
 	}
 
-	/* UFO needs SG and checksumming */
-	if (features & NETIF_F_UFO) {
-		/* maybe split UFO into V4 and V6? */
-		if (!((features & NETIF_F_GEN_CSUM) ||
-		    (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
-			    == (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
-			netdev_dbg(dev,
-				"Dropping NETIF_F_UFO since no checksum offload features.\n");
-			features &= ~NETIF_F_UFO;
-		}
-
-		if (!(features & NETIF_F_SG)) {
-			netdev_dbg(dev,
-				"Dropping NETIF_F_UFO since no NETIF_F_SG feature.\n");
-			features &= ~NETIF_F_UFO;
-		}
+	/* UFO also needs checksumming */
+	if ((features & NETIF_F_UFO) && !(features & NETIF_F_GEN_CSUM) &&
+					!(features & NETIF_F_IP_CSUM)) {
+		netdev_dbg(dev,
+			   "Dropping NETIF_F_UFO since no checksum offload features.\n");
+		features &= ~NETIF_F_UFO;
+	}
+	if ((features & NETIF_F_UFO6) && !(features & NETIF_F_GEN_CSUM) &&
+					 !(features & NETIF_F_IPV6_CSUM)) {
+		netdev_dbg(dev,
+			   "Dropping NETIF_F_UFO6 since no checksum offload features.\n");
+		features &= ~NETIF_F_UFO6;
 	}
 
+
 #ifdef CONFIG_NET_RX_BUSY_POLL
 	if (dev->netdev_ops->ndo_busy_poll)
 		features |= NETIF_F_BUSY_POLL;
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 06dfb29..93eff41 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -223,7 +223,7 @@ static netdev_features_t ethtool_get_feature_mask(u32 eth_cmd)
 		return NETIF_F_ALL_TSO;
 	case ETHTOOL_GUFO:
 	case ETHTOOL_SUFO:
-		return NETIF_F_UFO;
+		return NETIF_F_ALL_UFO;
 	case ETHTOOL_GGSO:
 	case ETHTOOL_SGSO:
 		return NETIF_F_GSO;
-- 
1.9.3

[PATCH 01/10] core: Split out UFO6 support

[Vladislav Yasevich]

Split IPv6 support for UFO into its own feature similiar to TSO.
This will later allow us to re-enable UFO support for virtio-net
devices.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 include/linux/netdev_features.h |  7 +++++--
 include/linux/netdevice.h       |  1 +
 include/linux/skbuff.h          |  1 +
 net/core/dev.c                  | 35 +++++++++++++++++++----------------
 net/core/ethtool.c              |  2 +-
 5 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index dcfdecb..a078945 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -48,8 +48,9 @@ enum {
 	NETIF_F_GSO_UDP_TUNNEL_BIT,	/* ... UDP TUNNEL with TSO */
 	NETIF_F_GSO_UDP_TUNNEL_CSUM_BIT,/* ... UDP TUNNEL with TSO & CSUM */
 	NETIF_F_GSO_MPLS_BIT,		/* ... MPLS segmentation */
+	NETIF_F_UFO6_BIT,		/* ... UDPv6 fragmentation */
 	/**/NETIF_F_GSO_LAST =		/* last bit, see GSO_MASK */
-		NETIF_F_GSO_MPLS_BIT,
+		NETIF_F_UFO6_BIT,
 
 	NETIF_F_FCOE_CRC_BIT,		/* FCoE CRC32 */
 	NETIF_F_SCTP_CSUM_BIT,		/* SCTP checksum offload */
@@ -109,6 +110,7 @@ enum {
 #define NETIF_F_TSO_ECN		__NETIF_F(TSO_ECN)
 #define NETIF_F_TSO		__NETIF_F(TSO)
 #define NETIF_F_UFO		__NETIF_F(UFO)
+#define NETIF_F_UFO6		__NETIF_F(UFO6)
 #define NETIF_F_VLAN_CHALLENGED	__NETIF_F(VLAN_CHALLENGED)
 #define NETIF_F_RXFCS		__NETIF_F(RXFCS)
 #define NETIF_F_RXALL		__NETIF_F(RXALL)
@@ -141,7 +143,7 @@ enum {
 
 /* List of features with software fallbacks. */
 #define NETIF_F_GSO_SOFTWARE	(NETIF_F_TSO | NETIF_F_TSO_ECN | \
-				 NETIF_F_TSO6 | NETIF_F_UFO)
+				 NETIF_F_TSO6 | NETIF_F_UFO | NETIF_F_UFO6)
 
 #define NETIF_F_GEN_CSUM	NETIF_F_HW_CSUM
 #define NETIF_F_V4_CSUM		(NETIF_F_GEN_CSUM | NETIF_F_IP_CSUM)
@@ -149,6 +151,7 @@ enum {
 #define NETIF_F_ALL_CSUM	(NETIF_F_V4_CSUM | NETIF_F_V6_CSUM)
 
 #define NETIF_F_ALL_TSO 	(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_TSO_ECN)
+#define NETIF_F_ALL_UFO		(NETIF_F_UFO | NETIF_F_UFO6)
 
 #define NETIF_F_ALL_FCOE	(NETIF_F_FCOE_CRC | NETIF_F_FCOE_MTU | \
 				 NETIF_F_FSO)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 74fd5d3..86af10a 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3559,6 +3559,7 @@ static inline bool net_gso_ok(netdev_features_t features, int gso_type)
 	/* check flags correspondence */
 	BUILD_BUG_ON(SKB_GSO_TCPV4   != (NETIF_F_TSO >> NETIF_F_GSO_SHIFT));
 	BUILD_BUG_ON(SKB_GSO_UDP     != (NETIF_F_UFO >> NETIF_F_GSO_SHIFT));
+	BUILD_BUG_ON(SKB_GSO_UDP6    != (NETIF_F_UFO6 >> NETIF_F_GSO_SHIFT));
 	BUILD_BUG_ON(SKB_GSO_DODGY   != (NETIF_F_GSO_ROBUST >> NETIF_F_GSO_SHIFT));
 	BUILD_BUG_ON(SKB_GSO_TCP_ECN != (NETIF_F_TSO_ECN >> NETIF_F_GSO_SHIFT));
 	BUILD_BUG_ON(SKB_GSO_TCPV6   != (NETIF_F_TSO6 >> NETIF_F_GSO_SHIFT));
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 6c8b6f6..8538b67 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -372,6 +372,7 @@ enum {
 
 	SKB_GSO_MPLS = 1 << 12,
 
+	SKB_GSO_UDP6 = 1 << 13
 };
 
 #if BITS_PER_LONG > 32
diff --git a/net/core/dev.c b/net/core/dev.c
index 945bbd0..fa4d2ee 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5929,6 +5929,12 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
 		features &= ~NETIF_F_ALL_TSO;
 	}
 
+	/* UFO requires that SG is present as well */
+	if ((features & NETIF_F_ALL_UFO) && !(features & NETIF_F_SG)) {
+		netdev_dbg(dev, "Dropping UFO features since no SG feature.\n");
+		features &= ~NETIF_F_ALL_UFO;
+	}
+
 	if ((features & NETIF_F_TSO) && !(features & NETIF_F_HW_CSUM) &&
 					!(features & NETIF_F_IP_CSUM)) {
 		netdev_dbg(dev, "Dropping TSO features since no CSUM feature.\n");
@@ -5952,24 +5958,21 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
 		features &= ~NETIF_F_GSO;
 	}
 
-	/* UFO needs SG and checksumming */
-	if (features & NETIF_F_UFO) {
-		/* maybe split UFO into V4 and V6? */
-		if (!((features & NETIF_F_GEN_CSUM) ||
-		    (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
-			    == (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
-			netdev_dbg(dev,
-				"Dropping NETIF_F_UFO since no checksum offload features.\n");
-			features &= ~NETIF_F_UFO;
-		}
-
-		if (!(features & NETIF_F_SG)) {
-			netdev_dbg(dev,
-				"Dropping NETIF_F_UFO since no NETIF_F_SG feature.\n");
-			features &= ~NETIF_F_UFO;
-		}
+	/* UFO also needs checksumming */
+	if ((features & NETIF_F_UFO) && !(features & NETIF_F_GEN_CSUM) &&
+					!(features & NETIF_F_IP_CSUM)) {
+		netdev_dbg(dev,
+			   "Dropping NETIF_F_UFO since no checksum offload features.\n");
+		features &= ~NETIF_F_UFO;
+	}
+	if ((features & NETIF_F_UFO6) && !(features & NETIF_F_GEN_CSUM) &&
+					 !(features & NETIF_F_IPV6_CSUM)) {
+		netdev_dbg(dev,
+			   "Dropping NETIF_F_UFO6 since no checksum offload features.\n");
+		features &= ~NETIF_F_UFO6;
 	}
 
+
 #ifdef CONFIG_NET_RX_BUSY_POLL
 	if (dev->netdev_ops->ndo_busy_poll)
 		features |= NETIF_F_BUSY_POLL;
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 06dfb29..93eff41 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -223,7 +223,7 @@ static netdev_features_t ethtool_get_feature_mask(u32 eth_cmd)
 		return NETIF_F_ALL_TSO;
 	case ETHTOOL_GUFO:
 	case ETHTOOL_SUFO:
-		return NETIF_F_UFO;
+		return NETIF_F_ALL_UFO;
 	case ETHTOOL_GGSO:
 	case ETHTOOL_SGSO:
 		return NETIF_F_GSO;
-- 
1.9.3

[PATCH 02/10] net: Correctly mark IPv6 UFO offload type.

[Vladislav Yasevich]

If the device supports UFO6 features, mark the offloaded ipv6 udp
traffic appropriately.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 net/ipv6/ip6_offload.c | 1 +
 net/ipv6/ip6_output.c  | 4 ++--
 net/ipv6/udp_offload.c | 3 ++-
 net/mpls/mpls_gso.c    | 1 +
 4 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c
index 01e12d0..bd985d5 100644
--- a/net/ipv6/ip6_offload.c
+++ b/net/ipv6/ip6_offload.c
@@ -81,6 +81,7 @@ static struct sk_buff *ipv6_gso_segment(struct sk_buff *skb,
 		       SKB_GSO_UDP_TUNNEL_CSUM |
 		       SKB_GSO_MPLS |
 		       SKB_GSO_TCPV6 |
+		       SKB_GSO_UDP6 |
 		       0)))
 		goto out;
 
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 8e950c2..83f5c04 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1089,7 +1089,7 @@ static inline int ip6_ufo_append_data(struct sock *sk,
 	 */
 	skb_shinfo(skb)->gso_size = (mtu - fragheaderlen -
 				     sizeof(struct frag_hdr)) & ~7;
-	skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
+	skb_shinfo(skb)->gso_type = SKB_GSO_UDP6;
 	ipv6_select_ident(&fhdr, rt);
 	skb_shinfo(skb)->ip6_frag_id = fhdr.identification;
 
@@ -1296,7 +1296,7 @@ emsgsize:
 	if (((length > mtu) ||
 	     (skb && skb_is_gso(skb))) &&
 	    (sk->sk_protocol == IPPROTO_UDP) &&
-	    (rt->dst.dev->features & NETIF_F_UFO)) {
+	    (rt->dst.dev->features & NETIF_F_UFO6)) {
 		err = ip6_ufo_append_data(sk, getfrag, from, length,
 					  hh_len, fragheaderlen,
 					  transhdrlen, mtu, flags, rt);
diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
index 6b8f543..00d723e 100644
--- a/net/ipv6/udp_offload.c
+++ b/net/ipv6/udp_offload.c
@@ -39,6 +39,7 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb,
 		int type = skb_shinfo(skb)->gso_type;
 
 		if (unlikely(type & ~(SKB_GSO_UDP |
+				      SKB_GSO_UDP6 |
 				      SKB_GSO_DODGY |
 				      SKB_GSO_UDP_TUNNEL |
 				      SKB_GSO_UDP_TUNNEL_CSUM |
@@ -47,7 +48,7 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb,
 				      SKB_GSO_IPIP |
 				      SKB_GSO_SIT |
 				      SKB_GSO_MPLS) ||
-			     !(type & (SKB_GSO_UDP))))
+			     !(type & (SKB_GSO_UDP | SKB_GSO_UDP6))))
 			goto out;
 
 		skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len, mss);
diff --git a/net/mpls/mpls_gso.c b/net/mpls/mpls_gso.c
index e3545f2..27343f0 100644
--- a/net/mpls/mpls_gso.c
+++ b/net/mpls/mpls_gso.c
@@ -30,6 +30,7 @@ static struct sk_buff *mpls_gso_segment(struct sk_buff *skb,
 				~(SKB_GSO_TCPV4 |
 				  SKB_GSO_TCPV6 |
 				  SKB_GSO_UDP |
+				  SKB_GSO_UDP6 |
 				  SKB_GSO_DODGY |
 				  SKB_GSO_TCP_ECN |
 				  SKB_GSO_GRE |
-- 
1.9.3

[PATCH 02/10] net: Correctly mark IPv6 UFO offload type.

[Vladislav Yasevich]

If the device supports UFO6 features, mark the offloaded ipv6 udp
traffic appropriately.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 net/ipv6/ip6_offload.c | 1 +
 net/ipv6/ip6_output.c  | 4 ++--
 net/ipv6/udp_offload.c | 3 ++-
 net/mpls/mpls_gso.c    | 1 +
 4 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c
index 01e12d0..bd985d5 100644
--- a/net/ipv6/ip6_offload.c
+++ b/net/ipv6/ip6_offload.c
@@ -81,6 +81,7 @@ static struct sk_buff *ipv6_gso_segment(struct sk_buff *skb,
 		       SKB_GSO_UDP_TUNNEL_CSUM |
 		       SKB_GSO_MPLS |
 		       SKB_GSO_TCPV6 |
+		       SKB_GSO_UDP6 |
 		       0)))
 		goto out;
 
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 8e950c2..83f5c04 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1089,7 +1089,7 @@ static inline int ip6_ufo_append_data(struct sock *sk,
 	 */
 	skb_shinfo(skb)->gso_size = (mtu - fragheaderlen -
 				     sizeof(struct frag_hdr)) & ~7;
-	skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
+	skb_shinfo(skb)->gso_type = SKB_GSO_UDP6;
 	ipv6_select_ident(&fhdr, rt);
 	skb_shinfo(skb)->ip6_frag_id = fhdr.identification;
 
@@ -1296,7 +1296,7 @@ emsgsize:
 	if (((length > mtu) ||
 	     (skb && skb_is_gso(skb))) &&
 	    (sk->sk_protocol == IPPROTO_UDP) &&
-	    (rt->dst.dev->features & NETIF_F_UFO)) {
+	    (rt->dst.dev->features & NETIF_F_UFO6)) {
 		err = ip6_ufo_append_data(sk, getfrag, from, length,
 					  hh_len, fragheaderlen,
 					  transhdrlen, mtu, flags, rt);
diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
index 6b8f543..00d723e 100644
--- a/net/ipv6/udp_offload.c
+++ b/net/ipv6/udp_offload.c
@@ -39,6 +39,7 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb,
 		int type = skb_shinfo(skb)->gso_type;
 
 		if (unlikely(type & ~(SKB_GSO_UDP |
+				      SKB_GSO_UDP6 |
 				      SKB_GSO_DODGY |
 				      SKB_GSO_UDP_TUNNEL |
 				      SKB_GSO_UDP_TUNNEL_CSUM |
@@ -47,7 +48,7 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb,
 				      SKB_GSO_IPIP |
 				      SKB_GSO_SIT |
 				      SKB_GSO_MPLS) ||
-			     !(type & (SKB_GSO_UDP))))
+			     !(type & (SKB_GSO_UDP | SKB_GSO_UDP6))))
 			goto out;
 
 		skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len, mss);
diff --git a/net/mpls/mpls_gso.c b/net/mpls/mpls_gso.c
index e3545f2..27343f0 100644
--- a/net/mpls/mpls_gso.c
+++ b/net/mpls/mpls_gso.c
@@ -30,6 +30,7 @@ static struct sk_buff *mpls_gso_segment(struct sk_buff *skb,
 				~(SKB_GSO_TCPV4 |
 				  SKB_GSO_TCPV6 |
 				  SKB_GSO_UDP |
+				  SKB_GSO_UDP6 |
 				  SKB_GSO_DODGY |
 				  SKB_GSO_TCP_ECN |
 				  SKB_GSO_GRE |
-- 
1.9.3

[PATCH 03/10] ovs: Enable handling of UFO6 packets.

[Vladislav Yasevich]

Since UFO6 packets can now be identified by SKB_GSO_UDP6, add proper checks
to handel UFO6 flows.
Legacy applications may still have UFO6 packets identified by SKB_GSO_UDP,
so we need to continue to handle them correclty.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 net/openvswitch/datapath.c | 3 ++-
 net/openvswitch/flow.c     | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index f9e556b..b43fc60 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -334,7 +334,8 @@ static int queue_gso_packets(struct datapath *dp, struct sk_buff *skb,
 		if (err)
 			break;
 
-		if (skb == segs && gso_type & SKB_GSO_UDP) {
+		if (skb == segs &&
+		    ((gso_type & SKB_GSO_UDP) || (gso_type & SKB_GSO_UDP6))) {
 			/* The initial flow key extracted by ovs_flow_extract()
 			 * in this case is for a first fragment, so we need to
 			 * properly mark later fragments.
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 2b78789..d03adf4 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -602,7 +602,7 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
 
 		if (key->ip.frag == OVS_FRAG_TYPE_LATER)
 			return 0;
-		if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
+		if (skb_shinfo(skb)->gso_type & (SKB_GSO_UDP | SKB_GSO_UDP6))
 			key->ip.frag = OVS_FRAG_TYPE_FIRST;
 
 		/* Transport layer. */
-- 
1.9.3

[PATCH 04/10] loopback: Turn on UFO6 support.

[Vladislav Yasevich]

Turn on UFO6 support.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 drivers/net/loopback.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index c76283c..762c28a 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -170,10 +170,10 @@ static void loopback_setup(struct net_device *dev)
 	dev->flags		= IFF_LOOPBACK;
 	dev->priv_flags		|= IFF_LIVE_ADDR_CHANGE;
 	netif_keep_dst(dev);
-	dev->hw_features	= NETIF_F_ALL_TSO | NETIF_F_UFO;
+	dev->hw_features	= NETIF_F_ALL_TSO | NETIF_F_ALL_UFO;
 	dev->features 		= NETIF_F_SG | NETIF_F_FRAGLIST
 		| NETIF_F_ALL_TSO
-		| NETIF_F_UFO
+		| NETIF_F_ALL_UFO
 		| NETIF_F_HW_CSUM
 		| NETIF_F_RXCSUM
 		| NETIF_F_SCTP_CSUM
-- 
1.9.3

[PATCH 04/10] loopback: Turn on UFO6 support.

[Vladislav Yasevich]

Turn on UFO6 support.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 drivers/net/loopback.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index c76283c..762c28a 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -170,10 +170,10 @@ static void loopback_setup(struct net_device *dev)
 	dev->flags		= IFF_LOOPBACK;
 	dev->priv_flags		|= IFF_LIVE_ADDR_CHANGE;
 	netif_keep_dst(dev);
-	dev->hw_features	= NETIF_F_ALL_TSO | NETIF_F_UFO;
+	dev->hw_features	= NETIF_F_ALL_TSO | NETIF_F_ALL_UFO;
 	dev->features 		= NETIF_F_SG | NETIF_F_FRAGLIST
 		| NETIF_F_ALL_TSO
-		| NETIF_F_UFO
+		| NETIF_F_ALL_UFO
 		| NETIF_F_HW_CSUM
 		| NETIF_F_RXCSUM
 		| NETIF_F_SCTP_CSUM
-- 
1.9.3

[PATCH 05/10] veth: Enable UFO6 support.

[Vladislav Yasevich]

Turn on UFO6 feature.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 drivers/net/veth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 8ad5965..0052db5 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -280,7 +280,7 @@ static const struct net_device_ops veth_netdev_ops = {
 #define VETH_FEATURES (NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_ALL_TSO |    \
 		       NETIF_F_HW_CSUM | NETIF_F_RXCSUM | NETIF_F_HIGHDMA | \
 		       NETIF_F_GSO_GRE | NETIF_F_GSO_UDP_TUNNEL |	    \
-		       NETIF_F_GSO_IPIP | NETIF_F_GSO_SIT | NETIF_F_UFO	|   \
+		       NETIF_F_GSO_IPIP | NETIF_F_GSO_SIT | NETIF_F_ALL_UFO | \
 		       NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX | \
 		       NETIF_F_HW_VLAN_STAG_TX | NETIF_F_HW_VLAN_STAG_RX )
 
-- 
1.9.3

[PATCH 05/10] veth: Enable UFO6 support.

[Vladislav Yasevich]

Turn on UFO6 feature.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 drivers/net/veth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 8ad5965..0052db5 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -280,7 +280,7 @@ static const struct net_device_ops veth_netdev_ops = {
 #define VETH_FEATURES (NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_ALL_TSO |    \
 		       NETIF_F_HW_CSUM | NETIF_F_RXCSUM | NETIF_F_HIGHDMA | \
 		       NETIF_F_GSO_GRE | NETIF_F_GSO_UDP_TUNNEL |	    \
-		       NETIF_F_GSO_IPIP | NETIF_F_GSO_SIT | NETIF_F_UFO	|   \
+		       NETIF_F_GSO_IPIP | NETIF_F_GSO_SIT | NETIF_F_ALL_UFO | \
 		       NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX | \
 		       NETIF_F_HW_VLAN_STAG_TX | NETIF_F_HW_VLAN_STAG_RX )
 
-- 
1.9.3

[PATCH 06/10] macvlan: Enable UFO6 support.

[Vladislav Yasevich]

Turn on UFO6 feature.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 drivers/net/macvlan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index bfb0b6e..807b98d 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -746,7 +746,7 @@ static struct lock_class_key macvlan_netdev_addr_lock_key;
 
 #define MACVLAN_FEATURES \
 	(NETIF_F_SG | NETIF_F_ALL_CSUM | NETIF_F_HIGHDMA | NETIF_F_FRAGLIST | \
-	 NETIF_F_GSO | NETIF_F_TSO | NETIF_F_UFO | NETIF_F_GSO_ROBUST | \
+	 NETIF_F_GSO | NETIF_F_TSO | NETIF_F_ALL_UFO | NETIF_F_GSO_ROBUST | \
 	 NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_GRO | NETIF_F_RXCSUM | \
 	 NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_VLAN_STAG_FILTER)
 
-- 
1.9.3

[PATCH 06/10] macvlan: Enable UFO6 support.

[Vladislav Yasevich]

Turn on UFO6 feature.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 drivers/net/macvlan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index bfb0b6e..807b98d 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -746,7 +746,7 @@ static struct lock_class_key macvlan_netdev_addr_lock_key;
 
 #define MACVLAN_FEATURES \
 	(NETIF_F_SG | NETIF_F_ALL_CSUM | NETIF_F_HIGHDMA | NETIF_F_FRAGLIST | \
-	 NETIF_F_GSO | NETIF_F_TSO | NETIF_F_UFO | NETIF_F_GSO_ROBUST | \
+	 NETIF_F_GSO | NETIF_F_TSO | NETIF_F_ALL_UFO | NETIF_F_GSO_ROBUST | \
 	 NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_GRO | NETIF_F_RXCSUM | \
 	 NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_VLAN_STAG_FILTER)
 
-- 
1.9.3

[PATCH 07/10] s2io: Enable UFO6 support.

[Vladislav Yasevich]

CC: Jon Mason <jdmason@kudzu.us>
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 drivers/net/ethernet/neterion/s2io.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/neterion/s2io.c b/drivers/net/ethernet/neterion/s2io.c
index f5e4b82..d823bb7 100644
--- a/drivers/net/ethernet/neterion/s2io.c
+++ b/drivers/net/ethernet/neterion/s2io.c
@@ -4140,7 +4140,7 @@ static netdev_tx_t s2io_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	frg_len = skb_headlen(skb);
-	if (offload_type == SKB_GSO_UDP) {
+	if (offload_type == SKB_GSO_UDP || offload_type == SKB_GSO_UDP6) {
 		int ufo_size;
 
 		ufo_size = s2io_udp_mss(skb);
@@ -7917,9 +7917,9 @@ s2io_init_nic(struct pci_dev *pdev, const struct pci_device_id *pre)
 	dev->features |= dev->hw_features |
 		NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX;
 	if (sp->device_type & XFRAME_II_DEVICE) {
-		dev->hw_features |= NETIF_F_UFO;
+		dev->hw_features |= NETIF_F_ALL_UFO;
 		if (ufo)
-			dev->features |= NETIF_F_UFO;
+			dev->features |= NETIF_F_ALL_UFO;
 	}
 	if (sp->high_dma_flag == true)
 		dev->features |= NETIF_F_HIGHDMA;
-- 
1.9.3

[PATCH 07/10] s2io: Enable UFO6 support.

[Vladislav Yasevich]

CC: Jon Mason <jdmason@kudzu.us>
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 drivers/net/ethernet/neterion/s2io.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/neterion/s2io.c b/drivers/net/ethernet/neterion/s2io.c
index f5e4b82..d823bb7 100644
--- a/drivers/net/ethernet/neterion/s2io.c
+++ b/drivers/net/ethernet/neterion/s2io.c
@@ -4140,7 +4140,7 @@ static netdev_tx_t s2io_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	frg_len = skb_headlen(skb);
-	if (offload_type == SKB_GSO_UDP) {
+	if (offload_type == SKB_GSO_UDP || offload_type == SKB_GSO_UDP6) {
 		int ufo_size;
 
 		ufo_size = s2io_udp_mss(skb);
@@ -7917,9 +7917,9 @@ s2io_init_nic(struct pci_dev *pdev, const struct pci_device_id *pre)
 	dev->features |= dev->hw_features |
 		NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX;
 	if (sp->device_type & XFRAME_II_DEVICE) {
-		dev->hw_features |= NETIF_F_UFO;
+		dev->hw_features |= NETIF_F_ALL_UFO;
 		if (ufo)
-			dev->features |= NETIF_F_UFO;
+			dev->features |= NETIF_F_ALL_UFO;
 	}
 	if (sp->high_dma_flag == true)
 		dev->features |= NETIF_F_HIGHDMA;
-- 
1.9.3

[PATCH 08/10] tun: Re-uanble UFO support.

[Vladislav Yasevich]

Now that UFO is split into v4 and v6 parts, we can bring
back v4 support without any trouble.

Continue to handle legacy applications by selecting the
IPv6 fragment id but do not change the gso type.  Thist
makes sure that two legacy VMs may still communicate.

Based on original work from Ben Hutchings.

Fixes: 88e0e0e5aa7a ("drivers/net: Disable UFO through virtio")
CC: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 drivers/net/tun.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 9dd3746..8c32fca 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -175,7 +175,7 @@ struct tun_struct {
 	struct net_device	*dev;
 	netdev_features_t	set_features;
 #define TUN_USER_FEATURES (NETIF_F_HW_CSUM|NETIF_F_TSO_ECN|NETIF_F_TSO| \
-			  NETIF_F_TSO6)
+			  NETIF_F_TSO6|NETIF_F_UFO)
 
 	int			vnet_hdr_sz;
 	int			sndbuf;
@@ -1152,20 +1152,15 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
 			break;
 		case VIRTIO_NET_HDR_GSO_UDP:
-		{
-			static bool warned;
-
-			if (!warned) {
-				warned = true;
-				netdev_warn(tun->dev,
-					    "%s: using disabled UFO feature; please fix this program\n",
-					    current->comm);
-			}
 			skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
-			if (skb->protocol == htons(ETH_P_IPV6))
+			if (vlan_get_protocol(skb) == htons(ETH_P_IPV6)) {
+				/* This allows legacy application to work.
+				 * Do not change the gso_type as it may
+				 * not be upderstood by legacy applications.
+				 */
 				ipv6_proxy_select_ident(skb);
+			}
 			break;
-		}
 		default:
 			tun->dev->stats.rx_frame_errors++;
 			kfree_skb(skb);
@@ -1273,6 +1268,8 @@ static ssize_t tun_put_user(struct tun_struct *tun,
 				gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
 			else if (sinfo->gso_type & SKB_GSO_TCPV6)
 				gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
+			else if (sinfo->gso_type & SKB_GSO_UDP)
+				gso.gso_type = VIRTIO_NET_HDR_GSO_UDP;
 			else {
 				pr_err("unexpected GSO type: "
 				       "0x%x, gso_size %d, hdr_len %d\n",
@@ -1780,6 +1777,11 @@ static int set_offload(struct tun_struct *tun, unsigned long arg)
 				features |= NETIF_F_TSO6;
 			arg &= ~(TUN_F_TSO4|TUN_F_TSO6);
 		}
+
+		if (arg & TUN_F_UFO) {
+			features |= NETIF_F_UFO;
+			arg &= ~TUN_F_UFO;
+		}
 	}
 
 	/* This gives the user a way to test for new features in future by
-- 
1.9.3

[PATCH 08/10] tun: Re-uanble UFO support.

[Vladislav Yasevich]

Now that UFO is split into v4 and v6 parts, we can bring
back v4 support without any trouble.

Continue to handle legacy applications by selecting the
IPv6 fragment id but do not change the gso type.  Thist
makes sure that two legacy VMs may still communicate.

Based on original work from Ben Hutchings.

Fixes: 88e0e0e5aa7a ("drivers/net: Disable UFO through virtio")
CC: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 drivers/net/tun.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 9dd3746..8c32fca 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -175,7 +175,7 @@ struct tun_struct {
 	struct net_device	*dev;
 	netdev_features_t	set_features;
 #define TUN_USER_FEATURES (NETIF_F_HW_CSUM|NETIF_F_TSO_ECN|NETIF_F_TSO| \
-			  NETIF_F_TSO6)
+			  NETIF_F_TSO6|NETIF_F_UFO)
 
 	int			vnet_hdr_sz;
 	int			sndbuf;
@@ -1152,20 +1152,15 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
 			break;
 		case VIRTIO_NET_HDR_GSO_UDP:
-		{
-			static bool warned;
-
-			if (!warned) {
-				warned = true;
-				netdev_warn(tun->dev,
-					    "%s: using disabled UFO feature; please fix this program\n",
-					    current->comm);
-			}
 			skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
-			if (skb->protocol == htons(ETH_P_IPV6))
+			if (vlan_get_protocol(skb) == htons(ETH_P_IPV6)) {
+				/* This allows legacy application to work.
+				 * Do not change the gso_type as it may
+				 * not be upderstood by legacy applications.
+				 */
 				ipv6_proxy_select_ident(skb);
+			}
 			break;
-		}
 		default:
 			tun->dev->stats.rx_frame_errors++;
 			kfree_skb(skb);
@@ -1273,6 +1268,8 @@ static ssize_t tun_put_user(struct tun_struct *tun,
 				gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
 			else if (sinfo->gso_type & SKB_GSO_TCPV6)
 				gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
+			else if (sinfo->gso_type & SKB_GSO_UDP)
+				gso.gso_type = VIRTIO_NET_HDR_GSO_UDP;
 			else {
 				pr_err("unexpected GSO type: "
 				       "0x%x, gso_size %d, hdr_len %d\n",
@@ -1780,6 +1777,11 @@ static int set_offload(struct tun_struct *tun, unsigned long arg)
 				features |= NETIF_F_TSO6;
 			arg &= ~(TUN_F_TSO4|TUN_F_TSO6);
 		}
+
+		if (arg & TUN_F_UFO) {
+			features |= NETIF_F_UFO;
+			arg &= ~TUN_F_UFO;
+		}
 	}
 
 	/* This gives the user a way to test for new features in future by
-- 
1.9.3

[PATCH 09/10] macvtap: Re-enable UFO support

[Vladislav Yasevich]

Now that UFO is split into v4 and v6 parts, we can bring
back v4 support.  Continue to handle legacy applications
by selecting the ipv6 fagment id but do not change the
gso type.  This allows 2 legacy VMs to continue to communicate.

Based on original work from Ben Hutchings.

Fixes: 88e0e0e5aa7a ("drivers/net: Disable UFO through virtio")
CC: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 drivers/net/macvtap.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 880cc09..75febd4 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -66,7 +66,7 @@ static struct cdev macvtap_cdev;
 static const struct proto_ops macvtap_socket_ops;
 
 #define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
-		      NETIF_F_TSO6)
+		      NETIF_F_TSO6 | NETIF_F_UFO)
 #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
 #define TAP_FEATURES (NETIF_F_GSO | NETIF_F_SG)
 
@@ -570,11 +570,14 @@ static int macvtap_skb_from_vnet_hdr(struct sk_buff *skb,
 			gso_type = SKB_GSO_TCPV6;
 			break;
 		case VIRTIO_NET_HDR_GSO_UDP:
-			pr_warn_once("macvtap: %s: using disabled UFO feature; please fix this program\n",
-				     current->comm);
 			gso_type = SKB_GSO_UDP;
-			if (skb->protocol == htons(ETH_P_IPV6))
+			if (vlan_get_protocol(skb) == htons(ETH_P_IPV6)) {
+				/* This is to support legacy appliacations.
+				 * Do not change the gso_type as legacy apps
+				 * may not know about the new type.
+				 */
 				ipv6_proxy_select_ident(skb);
+			}
 			break;
 		default:
 			return -EINVAL;
@@ -619,6 +622,8 @@ static void macvtap_skb_to_vnet_hdr(const struct sk_buff *skb,
 			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
 		else if (sinfo->gso_type & SKB_GSO_TCPV6)
 			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
+		else if (sinfo->gso_type & SKB_GSO_UDP)
+			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP;
 		else
 			BUG();
 		if (sinfo->gso_type & SKB_GSO_TCP_ECN)
@@ -955,6 +960,9 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
 			if (arg & TUN_F_TSO6)
 				feature_mask |= NETIF_F_TSO6;
 		}
+
+		if (arg & TUN_F_UFO)
+			feature_mask |= NETIF_F_UFO;
 	}
 
 	/* tun/tap driver inverts the usage for TSO offloads, where
@@ -965,7 +973,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
 	 * When user space turns off TSO, we turn off GSO/LRO so that
 	 * user-space will not receive TSO frames.
 	 */
-	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6))
+	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO))
 		features |= RX_OFFLOADS;
 	else
 		features &= ~RX_OFFLOADS;
@@ -1066,7 +1074,7 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
 	case TUNSETOFFLOAD:
 		/* let the user check for future flags */
 		if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
-			    TUN_F_TSO_ECN))
+			    TUN_F_TSO_ECN | TUN_F_UFO))
 			return -EINVAL;
 
 		rtnl_lock();
-- 
1.9.3

[PATCH 09/10] macvtap: Re-enable UFO support

[Vladislav Yasevich]

Now that UFO is split into v4 and v6 parts, we can bring
back v4 support.  Continue to handle legacy applications
by selecting the ipv6 fagment id but do not change the
gso type.  This allows 2 legacy VMs to continue to communicate.

Based on original work from Ben Hutchings.

Fixes: 88e0e0e5aa7a ("drivers/net: Disable UFO through virtio")
CC: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 drivers/net/macvtap.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 880cc09..75febd4 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -66,7 +66,7 @@ static struct cdev macvtap_cdev;
 static const struct proto_ops macvtap_socket_ops;
 
 #define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
-		      NETIF_F_TSO6)
+		      NETIF_F_TSO6 | NETIF_F_UFO)
 #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
 #define TAP_FEATURES (NETIF_F_GSO | NETIF_F_SG)
 
@@ -570,11 +570,14 @@ static int macvtap_skb_from_vnet_hdr(struct sk_buff *skb,
 			gso_type = SKB_GSO_TCPV6;
 			break;
 		case VIRTIO_NET_HDR_GSO_UDP:
-			pr_warn_once("macvtap: %s: using disabled UFO feature; please fix this program\n",
-				     current->comm);
 			gso_type = SKB_GSO_UDP;
-			if (skb->protocol == htons(ETH_P_IPV6))
+			if (vlan_get_protocol(skb) == htons(ETH_P_IPV6)) {
+				/* This is to support legacy appliacations.
+				 * Do not change the gso_type as legacy apps
+				 * may not know about the new type.
+				 */
 				ipv6_proxy_select_ident(skb);
+			}
 			break;
 		default:
 			return -EINVAL;
@@ -619,6 +622,8 @@ static void macvtap_skb_to_vnet_hdr(const struct sk_buff *skb,
 			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
 		else if (sinfo->gso_type & SKB_GSO_TCPV6)
 			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
+		else if (sinfo->gso_type & SKB_GSO_UDP)
+			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP;
 		else
 			BUG();
 		if (sinfo->gso_type & SKB_GSO_TCP_ECN)
@@ -955,6 +960,9 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
 			if (arg & TUN_F_TSO6)
 				feature_mask |= NETIF_F_TSO6;
 		}
+
+		if (arg & TUN_F_UFO)
+			feature_mask |= NETIF_F_UFO;
 	}
 
 	/* tun/tap driver inverts the usage for TSO offloads, where
@@ -965,7 +973,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
 	 * When user space turns off TSO, we turn off GSO/LRO so that
 	 * user-space will not receive TSO frames.
 	 */
-	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6))
+	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO))
 		features |= RX_OFFLOADS;
 	else
 		features &= ~RX_OFFLOADS;
@@ -1066,7 +1074,7 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
 	case TUNSETOFFLOAD:
 		/* let the user check for future flags */
 		if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
-			    TUN_F_TSO_ECN))
+			    TUN_F_TSO_ECN | TUN_F_UFO))
 			return -EINVAL;
 
 		rtnl_lock();
-- 
1.9.3

[PATCH 10/10] Revert "drivers/net: Disable UFO through virtio"

[Vladislav Yasevich]

This reverts commit 3d0ad09412ffe00c9afa201d01effdb6023d09b4.
Now that we've split UFO into v4 and v6 version, we can turn
back UFO support for ipv4.  Full IPv6 support will come later as
it requires extending vnet header structure.

Any older VM that assumes IPv6 support is included in UFO
will continue to use UFO and the host will generate fragment
ids for it, thus preserving connectivity.

Fixes: 88e0e0e5aa7a ("drivers/net: Disable UFO through virtio")
CC: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 drivers/net/virtio_net.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index b0bc8ea..534b633 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -491,17 +491,8 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
 			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
 			break;
 		case VIRTIO_NET_HDR_GSO_UDP:
-		{
-			static bool warned;
-
-			if (!warned) {
-				warned = true;
-				netdev_warn(dev,
-					    "host using disabled UFO feature; please fix it\n");
-			}
 			skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
 			break;
-		}
 		case VIRTIO_NET_HDR_GSO_TCPV6:
 			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
 			break;
@@ -890,6 +881,8 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
 			hdr->hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
 		else if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6)
 			hdr->hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
+		else if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
+			hdr->hdr.gso_type = VIRTIO_NET_HDR_GSO_UDP;
 		else
 			BUG();
 		if (skb_shinfo(skb)->gso_type & SKB_GSO_TCP_ECN)
@@ -1749,7 +1742,7 @@ static int virtnet_probe(struct virtio_device *vdev)
 			dev->features |= NETIF_F_HW_CSUM|NETIF_F_SG|NETIF_F_FRAGLIST;
 
 		if (virtio_has_feature(vdev, VIRTIO_NET_F_GSO)) {
-			dev->hw_features |= NETIF_F_TSO
+			dev->hw_features |= NETIF_F_TSO | NETIF_F_UFO
 				| NETIF_F_TSO_ECN | NETIF_F_TSO6;
 		}
 		/* Individual feature bits: what can host handle? */
@@ -1759,9 +1752,11 @@ static int virtnet_probe(struct virtio_device *vdev)
 			dev->hw_features |= NETIF_F_TSO6;
 		if (virtio_has_feature(vdev, VIRTIO_NET_F_HOST_ECN))
 			dev->hw_features |= NETIF_F_TSO_ECN;
+		if (virtio_has_feature(vdev, VIRTIO_NET_F_HOST_UFO))
+			dev->hw_features |= NETIF_F_UFO;
 
 		if (gso)
-			dev->features |= dev->hw_features & NETIF_F_ALL_TSO;
+			dev->features |= dev->hw_features & (NETIF_F_ALL_TSO|NETIF_F_UFO);
 		/* (!csum && gso) case will be fixed by register_netdev() */
 	}
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
@@ -1799,7 +1794,8 @@ static int virtnet_probe(struct virtio_device *vdev)
 	/* If we can receive ANY GSO packets, we must allocate large ones. */
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
 	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6) ||
-	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_ECN))
+	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_ECN) ||
+	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_UFO))
 		vi->big_packets = true;
 
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
@@ -1993,9 +1989,9 @@ static struct virtio_device_id id_table[] = {
 static unsigned int features[] = {
 	VIRTIO_NET_F_CSUM, VIRTIO_NET_F_GUEST_CSUM,
 	VIRTIO_NET_F_GSO, VIRTIO_NET_F_MAC,
-	VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_TSO6,
+	VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_UFO, VIRTIO_NET_F_HOST_TSO6,
 	VIRTIO_NET_F_HOST_ECN, VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
-	VIRTIO_NET_F_GUEST_ECN,
+	VIRTIO_NET_F_GUEST_ECN, VIRTIO_NET_F_GUEST_UFO,
 	VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ,
 	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN,
 	VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ,
-- 
1.9.3

[PATCH 10/10] Revert "drivers/net: Disable UFO through virtio"

[Vladislav Yasevich]

This reverts commit 3d0ad09412ffe00c9afa201d01effdb6023d09b4.
Now that we've split UFO into v4 and v6 version, we can turn
back UFO support for ipv4.  Full IPv6 support will come later as
it requires extending vnet header structure.

Any older VM that assumes IPv6 support is included in UFO
will continue to use UFO and the host will generate fragment
ids for it, thus preserving connectivity.

Fixes: 88e0e0e5aa7a ("drivers/net: Disable UFO through virtio")
CC: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 drivers/net/virtio_net.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index b0bc8ea..534b633 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -491,17 +491,8 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
 			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
 			break;
 		case VIRTIO_NET_HDR_GSO_UDP:
-		{
-			static bool warned;
-
-			if (!warned) {
-				warned = true;
-				netdev_warn(dev,
-					    "host using disabled UFO feature; please fix it\n");
-			}
 			skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
 			break;
-		}
 		case VIRTIO_NET_HDR_GSO_TCPV6:
 			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
 			break;
@@ -890,6 +881,8 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
 			hdr->hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
 		else if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6)
 			hdr->hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
+		else if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
+			hdr->hdr.gso_type = VIRTIO_NET_HDR_GSO_UDP;
 		else
 			BUG();
 		if (skb_shinfo(skb)->gso_type & SKB_GSO_TCP_ECN)
@@ -1749,7 +1742,7 @@ static int virtnet_probe(struct virtio_device *vdev)
 			dev->features |= NETIF_F_HW_CSUM|NETIF_F_SG|NETIF_F_FRAGLIST;
 
 		if (virtio_has_feature(vdev, VIRTIO_NET_F_GSO)) {
-			dev->hw_features |= NETIF_F_TSO
+			dev->hw_features |= NETIF_F_TSO | NETIF_F_UFO
 				| NETIF_F_TSO_ECN | NETIF_F_TSO6;
 		}
 		/* Individual feature bits: what can host handle? */
@@ -1759,9 +1752,11 @@ static int virtnet_probe(struct virtio_device *vdev)
 			dev->hw_features |= NETIF_F_TSO6;
 		if (virtio_has_feature(vdev, VIRTIO_NET_F_HOST_ECN))
 			dev->hw_features |= NETIF_F_TSO_ECN;
+		if (virtio_has_feature(vdev, VIRTIO_NET_F_HOST_UFO))
+			dev->hw_features |= NETIF_F_UFO;
 
 		if (gso)
-			dev->features |= dev->hw_features & NETIF_F_ALL_TSO;
+			dev->features |= dev->hw_features & (NETIF_F_ALL_TSO|NETIF_F_UFO);
 		/* (!csum && gso) case will be fixed by register_netdev() */
 	}
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
@@ -1799,7 +1794,8 @@ static int virtnet_probe(struct virtio_device *vdev)
 	/* If we can receive ANY GSO packets, we must allocate large ones. */
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
 	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6) ||
-	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_ECN))
+	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_ECN) ||
+	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_UFO))
 		vi->big_packets = true;
 
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
@@ -1993,9 +1989,9 @@ static struct virtio_device_id id_table[] = {
 static unsigned int features[] = {
 	VIRTIO_NET_F_CSUM, VIRTIO_NET_F_GUEST_CSUM,
 	VIRTIO_NET_F_GSO, VIRTIO_NET_F_MAC,
-	VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_TSO6,
+	VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_UFO, VIRTIO_NET_F_HOST_TSO6,
 	VIRTIO_NET_F_HOST_ECN, VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
-	VIRTIO_NET_F_GUEST_ECN,
+	VIRTIO_NET_F_GUEST_ECN, VIRTIO_NET_F_GUEST_UFO,
 	VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ,
 	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN,
 	VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ,
-- 
1.9.3

Re: [PATCH 01/10] core: Split out UFO6 support

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1588 bytes --]

On Wed, 2014-12-17 at 13:20 -0500, Vladislav Yasevich wrote:
> Split IPv6 support for UFO into its own feature similiar to TSO.
> This will later allow us to re-enable UFO support for virtio-net
> devices.
[...]
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 6c8b6f6..8538b67 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -372,6 +372,7 @@ enum {
>  
>  	SKB_GSO_MPLS = 1 << 12,
>  
> +	SKB_GSO_UDP6 = 1 << 13

It seems like it would be cleaner to use the names SKB_GSO_UDPV{4,6},
similarly to SKB_GSO_TCPV{4,6}.

>  };
>  
>  #if BITS_PER_LONG > 32
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 945bbd0..fa4d2ee 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
[...]
> @@ -5952,24 +5958,21 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
[...]
> +	/* UFO also needs checksumming */
> +	if ((features & NETIF_F_UFO) && !(features & NETIF_F_GEN_CSUM) &&
> +					!(features & NETIF_F_IP_CSUM)) {

You can use !(features & NETIF_F_V4_CSUM) instead of the last two terms.

> +		netdev_dbg(dev,
> +			   "Dropping NETIF_F_UFO since no checksum offload features.\n");
> +		features &= ~NETIF_F_UFO;
> +	}
> +	if ((features & NETIF_F_UFO6) && !(features & NETIF_F_GEN_CSUM) &&
> +					 !(features & NETIF_F_IPV6_CSUM)) {
[...]

Similarly you can use !(features & NETIF_F_V6_CSUM) instead of the last
two terms.

Aside from those minor points, this looks fine.

Ben.

-- 
Ben Hutchings
Absolutum obsoletum. (If it works, it's out of date.) - Stafford Beer

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 01/10] core: Split out UFO6 support

[Ben Hutchings]

[-- Attachment #1.1: Type: text/plain, Size: 1588 bytes --]

On Wed, 2014-12-17 at 13:20 -0500, Vladislav Yasevich wrote:
> Split IPv6 support for UFO into its own feature similiar to TSO.
> This will later allow us to re-enable UFO support for virtio-net
> devices.
[...]
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 6c8b6f6..8538b67 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -372,6 +372,7 @@ enum {
>  
>  	SKB_GSO_MPLS = 1 << 12,
>  
> +	SKB_GSO_UDP6 = 1 << 13

It seems like it would be cleaner to use the names SKB_GSO_UDPV{4,6},
similarly to SKB_GSO_TCPV{4,6}.

>  };
>  
>  #if BITS_PER_LONG > 32
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 945bbd0..fa4d2ee 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
[...]
> @@ -5952,24 +5958,21 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
[...]
> +	/* UFO also needs checksumming */
> +	if ((features & NETIF_F_UFO) && !(features & NETIF_F_GEN_CSUM) &&
> +					!(features & NETIF_F_IP_CSUM)) {

You can use !(features & NETIF_F_V4_CSUM) instead of the last two terms.

> +		netdev_dbg(dev,
> +			   "Dropping NETIF_F_UFO since no checksum offload features.\n");
> +		features &= ~NETIF_F_UFO;
> +	}
> +	if ((features & NETIF_F_UFO6) && !(features & NETIF_F_GEN_CSUM) &&
> +					 !(features & NETIF_F_IPV6_CSUM)) {
[...]

Similarly you can use !(features & NETIF_F_V6_CSUM) instead of the last
two terms.

Aside from those minor points, this looks fine.

Ben.

-- 
Ben Hutchings
Absolutum obsoletum. (If it works, it's out of date.) - Stafford Beer

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 03/10] ovs: Enable handling of UFO6 packets.

[Sergei Shtylyov]

Hello.

On 12/17/2014 09:20 PM, Vladislav Yasevich wrote:

> Since UFO6 packets can now be identified by SKB_GSO_UDP6, add proper checks
> to handel UFO6 flows.
> Legacy applications may still have UFO6 packets identified by SKB_GSO_UDP,
> so we need to continue to handle them correclty.

> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---
>   net/openvswitch/datapath.c | 3 ++-
>   net/openvswitch/flow.c     | 2 +-
>   2 files changed, 3 insertions(+), 2 deletions(-)

> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index f9e556b..b43fc60 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -334,7 +334,8 @@ static int queue_gso_packets(struct datapath *dp, struct sk_buff *skb,
>   		if (err)
>   			break;
>
> -		if (skb == segs && gso_type & SKB_GSO_UDP) {
> +		if (skb == segs &&
> +		    ((gso_type & SKB_GSO_UDP) || (gso_type & SKB_GSO_UDP6))) {

    'gso_type & (SKB_GSO_UDP | SKB_GSO_UDP6)' would be shorter...

>   			/* The initial flow key extracted by ovs_flow_extract()
>   			 * in this case is for a first fragment, so we need to
>   			 * properly mark later fragments.
> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> index 2b78789..d03adf4 100644
> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
> @@ -602,7 +602,7 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
>
>   		if (key->ip.frag == OVS_FRAG_TYPE_LATER)
>   			return 0;
> -		if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> +		if (skb_shinfo(skb)->gso_type & (SKB_GSO_UDP | SKB_GSO_UDP6))

    .... like here.

[...]

WBR, Sergei

Re: [PATCH 01/10] core: Split out UFO6 support

[Vlad Yasevich]

On 12/17/2014 03:10 PM, Ben Hutchings wrote:
> On Wed, 2014-12-17 at 13:20 -0500, Vladislav Yasevich wrote:
>> Split IPv6 support for UFO into its own feature similiar to TSO.
>> This will later allow us to re-enable UFO support for virtio-net
>> devices.
> [...]
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index 6c8b6f6..8538b67 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -372,6 +372,7 @@ enum {
>>  
>>  	SKB_GSO_MPLS = 1 << 12,
>>  
>> +	SKB_GSO_UDP6 = 1 << 13
> 
> It seems like it would be cleaner to use the names SKB_GSO_UDPV{4,6},
> similarly to SKB_GSO_TCPV{4,6}.

I wanted to try to avoid touched ipv4 paths if I could.  I could use
GSO_UDPV6 though.

> 
>>  };
>>  
>>  #if BITS_PER_LONG > 32
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 945bbd0..fa4d2ee 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
> [...]
>> @@ -5952,24 +5958,21 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
> [...]
>> +	/* UFO also needs checksumming */
>> +	if ((features & NETIF_F_UFO) && !(features & NETIF_F_GEN_CSUM) &&
>> +					!(features & NETIF_F_IP_CSUM)) {
> 
> You can use !(features & NETIF_F_V4_CSUM) instead of the last two terms.
> 
>> +		netdev_dbg(dev,
>> +			   "Dropping NETIF_F_UFO since no checksum offload features.\n");
>> +		features &= ~NETIF_F_UFO;
>> +	}
>> +	if ((features & NETIF_F_UFO6) && !(features & NETIF_F_GEN_CSUM) &&
>> +					 !(features & NETIF_F_IPV6_CSUM)) {
> [...]
> 
> Similarly you can use !(features & NETIF_F_V6_CSUM) instead of the last
> two terms.

I made those to look the same as the TSO checks for consistency, but I can change
these to be shorter like above.

-vlad

> 
> Aside from those minor points, this looks fine.
> 
> Ben.
> 

Re: [PATCH 03/10] ovs: Enable handling of UFO6 packets.

[Vlad Yasevich]

On 12/17/2014 03:17 PM, Sergei Shtylyov wrote:
> Hello.
> 
> On 12/17/2014 09:20 PM, Vladislav Yasevich wrote:
> 
>> Since UFO6 packets can now be identified by SKB_GSO_UDP6, add proper checks
>> to handel UFO6 flows.
>> Legacy applications may still have UFO6 packets identified by SKB_GSO_UDP,
>> so we need to continue to handle them correclty.
> 
>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>> ---
>>   net/openvswitch/datapath.c | 3 ++-
>>   net/openvswitch/flow.c     | 2 +-
>>   2 files changed, 3 insertions(+), 2 deletions(-)
> 
>> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
>> index f9e556b..b43fc60 100644
>> --- a/net/openvswitch/datapath.c
>> +++ b/net/openvswitch/datapath.c
>> @@ -334,7 +334,8 @@ static int queue_gso_packets(struct datapath *dp, struct sk_buff *skb,
>>           if (err)
>>               break;
>>
>> -        if (skb == segs && gso_type & SKB_GSO_UDP) {
>> +        if (skb == segs &&
>> +            ((gso_type & SKB_GSO_UDP) || (gso_type & SKB_GSO_UDP6))) {
> 
>    'gso_type & (SKB_GSO_UDP | SKB_GSO_UDP6)' would be shorter...

Thanks, will do.

-vlad

> 
>>               /* The initial flow key extracted by ovs_flow_extract()
>>                * in this case is for a first fragment, so we need to
>>                * properly mark later fragments.
>> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
>> index 2b78789..d03adf4 100644
>> --- a/net/openvswitch/flow.c
>> +++ b/net/openvswitch/flow.c
>> @@ -602,7 +602,7 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
>>
>>           if (key->ip.frag == OVS_FRAG_TYPE_LATER)
>>               return 0;
>> -        if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
>> +        if (skb_shinfo(skb)->gso_type & (SKB_GSO_UDP | SKB_GSO_UDP6))
> 
>    .... like here.
> 
> [...]
> 
> WBR, Sergei
> 

Re: [PATCH 03/10] ovs: Enable handling of UFO6 packets.

[Michael S. Tsirkin]

On Wed, Dec 17, 2014 at 01:20:48PM -0500, Vladislav Yasevich wrote:
> Since UFO6 packets can now be identified by SKB_GSO_UDP6, add proper checks
> to handel UFO6 flows.

s/handel/handle/

> Legacy applications may still have UFO6 packets identified by SKB_GSO_UDP,
> so we need to continue to handle them correclty.
> 
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---
>  net/openvswitch/datapath.c | 3 ++-
>  net/openvswitch/flow.c     | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index f9e556b..b43fc60 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -334,7 +334,8 @@ static int queue_gso_packets(struct datapath *dp, struct sk_buff *skb,
>  		if (err)
>  			break;
>  
> -		if (skb == segs && gso_type & SKB_GSO_UDP) {
> +		if (skb == segs &&
> +		    ((gso_type & SKB_GSO_UDP) || (gso_type & SKB_GSO_UDP6))) {
>  			/* The initial flow key extracted by ovs_flow_extract()
>  			 * in this case is for a first fragment, so we need to
>  			 * properly mark later fragments.
> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> index 2b78789..d03adf4 100644
> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
> @@ -602,7 +602,7 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
>  
>  		if (key->ip.frag == OVS_FRAG_TYPE_LATER)
>  			return 0;
> -		if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> +		if (skb_shinfo(skb)->gso_type & (SKB_GSO_UDP | SKB_GSO_UDP6))
>  			key->ip.frag = OVS_FRAG_TYPE_FIRST;
>  
>  		/* Transport layer. */
> -- 
> 1.9.3

Re: [PATCH 08/10] tun: Re-uanble UFO support.

[Michael S. Tsirkin]

subs: re-enable

On Wed, Dec 17, 2014 at 01:20:53PM -0500, Vladislav Yasevich wrote:
> Now that UFO is split into v4 and v6 parts, we can bring
> back v4 support without any trouble.
> 
> Continue to handle legacy applications by selecting the
> IPv6 fragment id but do not change the gso type.  Thist

s/Thist/this/

> makes sure that two legacy VMs may still communicate.

This means IPv6 skbs with UFO (not UFO6) flag set are present
in the stack.
If possible, I think it would be better to make GSO type correct: UFO6,
and then convert to UFO when copying to guest.

A similar approach should be possible  for OVS?


> Based on original work from Ben Hutchings.
> 
> Fixes: 88e0e0e5aa7a ("drivers/net: Disable UFO through virtio")
> CC: Ben Hutchings <ben@decadent.org.uk>
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---
>  drivers/net/tun.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 9dd3746..8c32fca 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -175,7 +175,7 @@ struct tun_struct {
>  	struct net_device	*dev;
>  	netdev_features_t	set_features;
>  #define TUN_USER_FEATURES (NETIF_F_HW_CSUM|NETIF_F_TSO_ECN|NETIF_F_TSO| \
> -			  NETIF_F_TSO6)
> +			  NETIF_F_TSO6|NETIF_F_UFO)
>  
>  	int			vnet_hdr_sz;
>  	int			sndbuf;
> @@ -1152,20 +1152,15 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
>  			break;
>  		case VIRTIO_NET_HDR_GSO_UDP:
> -		{
> -			static bool warned;
> -
> -			if (!warned) {
> -				warned = true;
> -				netdev_warn(tun->dev,
> -					    "%s: using disabled UFO feature; please fix this program\n",
> -					    current->comm);
> -			}
>  			skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> -			if (skb->protocol == htons(ETH_P_IPV6))
> +			if (vlan_get_protocol(skb) == htons(ETH_P_IPV6)) {
> +				/* This allows legacy application to work.
> +				 * Do not change the gso_type as it may
> +				 * not be upderstood by legacy applications.

Shouldn't we handle legacy applications when passing packets to
userspace?

> +				 */
>  				ipv6_proxy_select_ident(skb);
> +			}
>  			break;
> -		}
>  		default:
>  			tun->dev->stats.rx_frame_errors++;
>  			kfree_skb(skb);
> @@ -1273,6 +1268,8 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>  				gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
>  			else if (sinfo->gso_type & SKB_GSO_TCPV6)
>  				gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
> +			else if (sinfo->gso_type & SKB_GSO_UDP)
> +				gso.gso_type = VIRTIO_NET_HDR_GSO_UDP;
>  			else {
>  				pr_err("unexpected GSO type: "
>  				       "0x%x, gso_size %d, hdr_len %d\n",
> @@ -1780,6 +1777,11 @@ static int set_offload(struct tun_struct *tun, unsigned long arg)
>  				features |= NETIF_F_TSO6;
>  			arg &= ~(TUN_F_TSO4|TUN_F_TSO6);
>  		}
> +
> +		if (arg & TUN_F_UFO) {
> +			features |= NETIF_F_UFO;
> +			arg &= ~TUN_F_UFO;
> +		}
>  	}
>  
>  	/* This gives the user a way to test for new features in future by
> -- 
> 1.9.3

Re: [PATCH 09/10] macvtap: Re-enable UFO support

[Michael S. Tsirkin]

On Wed, Dec 17, 2014 at 01:20:54PM -0500, Vladislav Yasevich wrote:
> Now that UFO is split into v4 and v6 parts, we can bring
> back v4 support.  Continue to handle legacy applications
> by selecting the ipv6 fagment id but do not change the
> gso type.  This allows 2 legacy VMs to continue to communicate.
> 
> Based on original work from Ben Hutchings.
> 
> Fixes: 88e0e0e5aa7a ("drivers/net: Disable UFO through virtio")
> CC: Ben Hutchings <ben@decadent.org.uk>
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---
>  drivers/net/macvtap.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> index 880cc09..75febd4 100644
> --- a/drivers/net/macvtap.c
> +++ b/drivers/net/macvtap.c
> @@ -66,7 +66,7 @@ static struct cdev macvtap_cdev;
>  static const struct proto_ops macvtap_socket_ops;
>  
>  #define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> -		      NETIF_F_TSO6)
> +		      NETIF_F_TSO6 | NETIF_F_UFO)
>  #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
>  #define TAP_FEATURES (NETIF_F_GSO | NETIF_F_SG)
>  
> @@ -570,11 +570,14 @@ static int macvtap_skb_from_vnet_hdr(struct sk_buff *skb,
>  			gso_type = SKB_GSO_TCPV6;
>  			break;
>  		case VIRTIO_NET_HDR_GSO_UDP:
> -			pr_warn_once("macvtap: %s: using disabled UFO feature; please fix this program\n",
> -				     current->comm);
>  			gso_type = SKB_GSO_UDP;
> -			if (skb->protocol == htons(ETH_P_IPV6))
> +			if (vlan_get_protocol(skb) == htons(ETH_P_IPV6)) {
> +				/* This is to support legacy appliacations.
> +				 * Do not change the gso_type as legacy apps
> +				 * may not know about the new type.
> +				 */
>  				ipv6_proxy_select_ident(skb);
> +			}
>  			break;
>  		default:
>  			return -EINVAL;
> @@ -619,6 +622,8 @@ static void macvtap_skb_to_vnet_hdr(const struct sk_buff *skb,
>  			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
>  		else if (sinfo->gso_type & SKB_GSO_TCPV6)
>  			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
> +		else if (sinfo->gso_type & SKB_GSO_UDP)
> +			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP;
>  		else
>  			BUG();
>  		if (sinfo->gso_type & SKB_GSO_TCP_ECN)
> @@ -955,6 +960,9 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
>  			if (arg & TUN_F_TSO6)
>  				feature_mask |= NETIF_F_TSO6;
>  		}
> +
> +		if (arg & TUN_F_UFO)
> +			feature_mask |= NETIF_F_UFO;
>  	}
>  
>  	/* tun/tap driver inverts the usage for TSO offloads, where
> @@ -965,7 +973,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
>  	 * When user space turns off TSO, we turn off GSO/LRO so that
>  	 * user-space will not receive TSO frames.
>  	 */
> -	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6))
> +	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO))
>  		features |= RX_OFFLOADS;
>  	else
>  		features &= ~RX_OFFLOADS;

By the way this logic is completely broken, even without your patch,
isn't it?

It says: enable LRO+GRO if any of NETIF_F_TSO | NETIF_F_TSO6 |
NETIF_F_UFO set.

So what happens if I enable TSO only?
LRO gets enabled so I can still get TSO6 packets.


This really should be:

	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO) ==
			(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO)


fixing this probably should be a separate patch before your
series, and Cc stable.


> @@ -1066,7 +1074,7 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
>  	case TUNSETOFFLOAD:
>  		/* let the user check for future flags */
>  		if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
> -			    TUN_F_TSO_ECN))
> +			    TUN_F_TSO_ECN | TUN_F_UFO))
>  			return -EINVAL;
>  
>  		rtnl_lock();
> -- 
> 1.9.3

Re: [PATCH 10/10] Revert "drivers/net: Disable UFO through virtio"

[Michael S. Tsirkin]

On Wed, Dec 17, 2014 at 01:20:55PM -0500, Vladislav Yasevich wrote:
> This reverts commit 3d0ad09412ffe00c9afa201d01effdb6023d09b4.
> Now that we've split UFO into v4 and v6 version, we can turn
> back UFO support for ipv4.  Full IPv6 support will come later as
> it requires extending vnet header structure.
> 
> Any older VM that assumes IPv6 support is included in UFO
> will continue to use UFO and the host will generate fragment
> ids for it, thus preserving connectivity.
> 
> Fixes: 88e0e0e5aa7a ("drivers/net: Disable UFO through virtio")
> CC: Ben Hutchings <ben@decadent.org.uk>
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
>  drivers/net/virtio_net.c | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index b0bc8ea..534b633 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -491,17 +491,8 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
>  			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
>  			break;
>  		case VIRTIO_NET_HDR_GSO_UDP:
> -		{
> -			static bool warned;
> -
> -			if (!warned) {
> -				warned = true;
> -				netdev_warn(dev,
> -					    "host using disabled UFO feature; please fix it\n");
> -			}
>  			skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
>  			break;


This might not be true: could be a legacy host.
I think we need to check for IPv6 and set it
correctly to SKB_GSO_UDP/SKB_GSO_UDP6?


> -		}
>  		case VIRTIO_NET_HDR_GSO_TCPV6:
>  			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
>  			break;
> @@ -890,6 +881,8 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
>  			hdr->hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
>  		else if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6)
>  			hdr->hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
> +		else if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> +			hdr->hdr.gso_type = VIRTIO_NET_HDR_GSO_UDP;
>  		else
>  			BUG();
>  		if (skb_shinfo(skb)->gso_type & SKB_GSO_TCP_ECN)
> @@ -1749,7 +1742,7 @@ static int virtnet_probe(struct virtio_device *vdev)
>  			dev->features |= NETIF_F_HW_CSUM|NETIF_F_SG|NETIF_F_FRAGLIST;
>  
>  		if (virtio_has_feature(vdev, VIRTIO_NET_F_GSO)) {
> -			dev->hw_features |= NETIF_F_TSO
> +			dev->hw_features |= NETIF_F_TSO | NETIF_F_UFO
>  				| NETIF_F_TSO_ECN | NETIF_F_TSO6;
>  		}
>  		/* Individual feature bits: what can host handle? */
> @@ -1759,9 +1752,11 @@ static int virtnet_probe(struct virtio_device *vdev)
>  			dev->hw_features |= NETIF_F_TSO6;
>  		if (virtio_has_feature(vdev, VIRTIO_NET_F_HOST_ECN))
>  			dev->hw_features |= NETIF_F_TSO_ECN;
> +		if (virtio_has_feature(vdev, VIRTIO_NET_F_HOST_UFO))
> +			dev->hw_features |= NETIF_F_UFO;
>  
>  		if (gso)
> -			dev->features |= dev->hw_features & NETIF_F_ALL_TSO;
> +			dev->features |= dev->hw_features & (NETIF_F_ALL_TSO|NETIF_F_UFO);
>  		/* (!csum && gso) case will be fixed by register_netdev() */
>  	}
>  	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
> @@ -1799,7 +1794,8 @@ static int virtnet_probe(struct virtio_device *vdev)
>  	/* If we can receive ANY GSO packets, we must allocate large ones. */
>  	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
>  	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6) ||
> -	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_ECN))
> +	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_ECN) ||
> +	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_UFO))
>  		vi->big_packets = true;
>  
>  	if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
> @@ -1993,9 +1989,9 @@ static struct virtio_device_id id_table[] = {
>  static unsigned int features[] = {
>  	VIRTIO_NET_F_CSUM, VIRTIO_NET_F_GUEST_CSUM,
>  	VIRTIO_NET_F_GSO, VIRTIO_NET_F_MAC,
> -	VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_TSO6,
> +	VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_UFO, VIRTIO_NET_F_HOST_TSO6,
>  	VIRTIO_NET_F_HOST_ECN, VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
> -	VIRTIO_NET_F_GUEST_ECN,
> +	VIRTIO_NET_F_GUEST_ECN, VIRTIO_NET_F_GUEST_UFO,
>  	VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ,
>  	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN,
>  	VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ,
> -- 
> 1.9.3

Re: [PATCH 01/10] core: Split out UFO6 support

[Michael S. Tsirkin]

On Wed, Dec 17, 2014 at 01:20:46PM -0500, Vladislav Yasevich wrote:
> Split IPv6 support for UFO into its own feature similiar to TSO.
> This will later allow us to re-enable UFO support for virtio-net
> devices.
> 
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---
>  include/linux/netdev_features.h |  7 +++++--
>  include/linux/netdevice.h       |  1 +
>  include/linux/skbuff.h          |  1 +
>  net/core/dev.c                  | 35 +++++++++++++++++++----------------
>  net/core/ethtool.c              |  2 +-
>  5 files changed, 27 insertions(+), 19 deletions(-)
> 
> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> index dcfdecb..a078945 100644
> --- a/include/linux/netdev_features.h
> +++ b/include/linux/netdev_features.h
> @@ -48,8 +48,9 @@ enum {
>  	NETIF_F_GSO_UDP_TUNNEL_BIT,	/* ... UDP TUNNEL with TSO */
>  	NETIF_F_GSO_UDP_TUNNEL_CSUM_BIT,/* ... UDP TUNNEL with TSO & CSUM */
>  	NETIF_F_GSO_MPLS_BIT,		/* ... MPLS segmentation */
> +	NETIF_F_UFO6_BIT,		/* ... UDPv6 fragmentation */
>  	/**/NETIF_F_GSO_LAST =		/* last bit, see GSO_MASK */
> -		NETIF_F_GSO_MPLS_BIT,
> +		NETIF_F_UFO6_BIT,
>  
>  	NETIF_F_FCOE_CRC_BIT,		/* FCoE CRC32 */
>  	NETIF_F_SCTP_CSUM_BIT,		/* SCTP checksum offload */
> @@ -109,6 +110,7 @@ enum {
>  #define NETIF_F_TSO_ECN		__NETIF_F(TSO_ECN)
>  #define NETIF_F_TSO		__NETIF_F(TSO)
>  #define NETIF_F_UFO		__NETIF_F(UFO)
> +#define NETIF_F_UFO6		__NETIF_F(UFO6)
>  #define NETIF_F_VLAN_CHALLENGED	__NETIF_F(VLAN_CHALLENGED)
>  #define NETIF_F_RXFCS		__NETIF_F(RXFCS)
>  #define NETIF_F_RXALL		__NETIF_F(RXALL)
> @@ -141,7 +143,7 @@ enum {
>  
>  /* List of features with software fallbacks. */
>  #define NETIF_F_GSO_SOFTWARE	(NETIF_F_TSO | NETIF_F_TSO_ECN | \
> -				 NETIF_F_TSO6 | NETIF_F_UFO)
> +				 NETIF_F_TSO6 | NETIF_F_UFO | NETIF_F_UFO6)
>  
>  #define NETIF_F_GEN_CSUM	NETIF_F_HW_CSUM
>  #define NETIF_F_V4_CSUM		(NETIF_F_GEN_CSUM | NETIF_F_IP_CSUM)
> @@ -149,6 +151,7 @@ enum {
>  #define NETIF_F_ALL_CSUM	(NETIF_F_V4_CSUM | NETIF_F_V6_CSUM)
>  
>  #define NETIF_F_ALL_TSO 	(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_TSO_ECN)
> +#define NETIF_F_ALL_UFO		(NETIF_F_UFO | NETIF_F_UFO6)
>  
>  #define NETIF_F_ALL_FCOE	(NETIF_F_FCOE_CRC | NETIF_F_FCOE_MTU | \
>  				 NETIF_F_FSO)
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 74fd5d3..86af10a 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -3559,6 +3559,7 @@ static inline bool net_gso_ok(netdev_features_t features, int gso_type)
>  	/* check flags correspondence */
>  	BUILD_BUG_ON(SKB_GSO_TCPV4   != (NETIF_F_TSO >> NETIF_F_GSO_SHIFT));
>  	BUILD_BUG_ON(SKB_GSO_UDP     != (NETIF_F_UFO >> NETIF_F_GSO_SHIFT));
> +	BUILD_BUG_ON(SKB_GSO_UDP6    != (NETIF_F_UFO6 >> NETIF_F_GSO_SHIFT));
>  	BUILD_BUG_ON(SKB_GSO_DODGY   != (NETIF_F_GSO_ROBUST >> NETIF_F_GSO_SHIFT));
>  	BUILD_BUG_ON(SKB_GSO_TCP_ECN != (NETIF_F_TSO_ECN >> NETIF_F_GSO_SHIFT));
>  	BUILD_BUG_ON(SKB_GSO_TCPV6   != (NETIF_F_TSO6 >> NETIF_F_GSO_SHIFT));
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 6c8b6f6..8538b67 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -372,6 +372,7 @@ enum {
>  
>  	SKB_GSO_MPLS = 1 << 12,
>  
> +	SKB_GSO_UDP6 = 1 << 13
>  };
>  
>  #if BITS_PER_LONG > 32

So this implies anything getting GSO packets e.g.
from userspace now needs to check IP version to
set GSO type correctly.

I think you missed some places that do this, e.g. af_packet
sockets.


> diff --git a/net/core/dev.c b/net/core/dev.c
> index 945bbd0..fa4d2ee 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5929,6 +5929,12 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
>  		features &= ~NETIF_F_ALL_TSO;
>  	}
>  
> +	/* UFO requires that SG is present as well */
> +	if ((features & NETIF_F_ALL_UFO) && !(features & NETIF_F_SG)) {
> +		netdev_dbg(dev, "Dropping UFO features since no SG feature.\n");
> +		features &= ~NETIF_F_ALL_UFO;
> +	}
> +
>  	if ((features & NETIF_F_TSO) && !(features & NETIF_F_HW_CSUM) &&
>  					!(features & NETIF_F_IP_CSUM)) {
>  		netdev_dbg(dev, "Dropping TSO features since no CSUM feature.\n");
> @@ -5952,24 +5958,21 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
>  		features &= ~NETIF_F_GSO;
>  	}
>  
> -	/* UFO needs SG and checksumming */
> -	if (features & NETIF_F_UFO) {
> -		/* maybe split UFO into V4 and V6? */
> -		if (!((features & NETIF_F_GEN_CSUM) ||
> -		    (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
> -			    == (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
> -			netdev_dbg(dev,
> -				"Dropping NETIF_F_UFO since no checksum offload features.\n");
> -			features &= ~NETIF_F_UFO;
> -		}
> -
> -		if (!(features & NETIF_F_SG)) {
> -			netdev_dbg(dev,
> -				"Dropping NETIF_F_UFO since no NETIF_F_SG feature.\n");
> -			features &= ~NETIF_F_UFO;
> -		}
> +	/* UFO also needs checksumming */
> +	if ((features & NETIF_F_UFO) && !(features & NETIF_F_GEN_CSUM) &&
> +					!(features & NETIF_F_IP_CSUM)) {
> +		netdev_dbg(dev,
> +			   "Dropping NETIF_F_UFO since no checksum offload features.\n");
> +		features &= ~NETIF_F_UFO;
> +	}
> +	if ((features & NETIF_F_UFO6) && !(features & NETIF_F_GEN_CSUM) &&
> +					 !(features & NETIF_F_IPV6_CSUM)) {
> +		netdev_dbg(dev,
> +			   "Dropping NETIF_F_UFO6 since no checksum offload features.\n");
> +		features &= ~NETIF_F_UFO6;
>  	}
>  
> +
>  #ifdef CONFIG_NET_RX_BUSY_POLL
>  	if (dev->netdev_ops->ndo_busy_poll)
>  		features |= NETIF_F_BUSY_POLL;
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 06dfb29..93eff41 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -223,7 +223,7 @@ static netdev_features_t ethtool_get_feature_mask(u32 eth_cmd)
>  		return NETIF_F_ALL_TSO;
>  	case ETHTOOL_GUFO:
>  	case ETHTOOL_SUFO:
> -		return NETIF_F_UFO;
> +		return NETIF_F_ALL_UFO;
>  	case ETHTOOL_GGSO:
>  	case ETHTOOL_SGSO:
>  		return NETIF_F_GSO;
> -- 
> 1.9.3

Re: [PATCH 01/10] core: Split out UFO6 support

[Vlad Yasevich]

On 12/17/2014 05:45 PM, Michael S. Tsirkin wrote:
> On Wed, Dec 17, 2014 at 01:20:46PM -0500, Vladislav Yasevich wrote:
>> Split IPv6 support for UFO into its own feature similiar to TSO.
>> This will later allow us to re-enable UFO support for virtio-net
>> devices.
>>
>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>> ---
>>  include/linux/netdev_features.h |  7 +++++--
>>  include/linux/netdevice.h       |  1 +
>>  include/linux/skbuff.h          |  1 +
>>  net/core/dev.c                  | 35 +++++++++++++++++++----------------
>>  net/core/ethtool.c              |  2 +-
>>  5 files changed, 27 insertions(+), 19 deletions(-)
>>
>> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
>> index dcfdecb..a078945 100644
>> --- a/include/linux/netdev_features.h
>> +++ b/include/linux/netdev_features.h
>> @@ -48,8 +48,9 @@ enum {
>>  	NETIF_F_GSO_UDP_TUNNEL_BIT,	/* ... UDP TUNNEL with TSO */
>>  	NETIF_F_GSO_UDP_TUNNEL_CSUM_BIT,/* ... UDP TUNNEL with TSO & CSUM */
>>  	NETIF_F_GSO_MPLS_BIT,		/* ... MPLS segmentation */
>> +	NETIF_F_UFO6_BIT,		/* ... UDPv6 fragmentation */
>>  	/**/NETIF_F_GSO_LAST =		/* last bit, see GSO_MASK */
>> -		NETIF_F_GSO_MPLS_BIT,
>> +		NETIF_F_UFO6_BIT,
>>  
>>  	NETIF_F_FCOE_CRC_BIT,		/* FCoE CRC32 */
>>  	NETIF_F_SCTP_CSUM_BIT,		/* SCTP checksum offload */
>> @@ -109,6 +110,7 @@ enum {
>>  #define NETIF_F_TSO_ECN		__NETIF_F(TSO_ECN)
>>  #define NETIF_F_TSO		__NETIF_F(TSO)
>>  #define NETIF_F_UFO		__NETIF_F(UFO)
>> +#define NETIF_F_UFO6		__NETIF_F(UFO6)
>>  #define NETIF_F_VLAN_CHALLENGED	__NETIF_F(VLAN_CHALLENGED)
>>  #define NETIF_F_RXFCS		__NETIF_F(RXFCS)
>>  #define NETIF_F_RXALL		__NETIF_F(RXALL)
>> @@ -141,7 +143,7 @@ enum {
>>  
>>  /* List of features with software fallbacks. */
>>  #define NETIF_F_GSO_SOFTWARE	(NETIF_F_TSO | NETIF_F_TSO_ECN | \
>> -				 NETIF_F_TSO6 | NETIF_F_UFO)
>> +				 NETIF_F_TSO6 | NETIF_F_UFO | NETIF_F_UFO6)
>>  
>>  #define NETIF_F_GEN_CSUM	NETIF_F_HW_CSUM
>>  #define NETIF_F_V4_CSUM		(NETIF_F_GEN_CSUM | NETIF_F_IP_CSUM)
>> @@ -149,6 +151,7 @@ enum {
>>  #define NETIF_F_ALL_CSUM	(NETIF_F_V4_CSUM | NETIF_F_V6_CSUM)
>>  
>>  #define NETIF_F_ALL_TSO 	(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_TSO_ECN)
>> +#define NETIF_F_ALL_UFO		(NETIF_F_UFO | NETIF_F_UFO6)
>>  
>>  #define NETIF_F_ALL_FCOE	(NETIF_F_FCOE_CRC | NETIF_F_FCOE_MTU | \
>>  				 NETIF_F_FSO)
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 74fd5d3..86af10a 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -3559,6 +3559,7 @@ static inline bool net_gso_ok(netdev_features_t features, int gso_type)
>>  	/* check flags correspondence */
>>  	BUILD_BUG_ON(SKB_GSO_TCPV4   != (NETIF_F_TSO >> NETIF_F_GSO_SHIFT));
>>  	BUILD_BUG_ON(SKB_GSO_UDP     != (NETIF_F_UFO >> NETIF_F_GSO_SHIFT));
>> +	BUILD_BUG_ON(SKB_GSO_UDP6    != (NETIF_F_UFO6 >> NETIF_F_GSO_SHIFT));
>>  	BUILD_BUG_ON(SKB_GSO_DODGY   != (NETIF_F_GSO_ROBUST >> NETIF_F_GSO_SHIFT));
>>  	BUILD_BUG_ON(SKB_GSO_TCP_ECN != (NETIF_F_TSO_ECN >> NETIF_F_GSO_SHIFT));
>>  	BUILD_BUG_ON(SKB_GSO_TCPV6   != (NETIF_F_TSO6 >> NETIF_F_GSO_SHIFT));
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index 6c8b6f6..8538b67 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -372,6 +372,7 @@ enum {
>>  
>>  	SKB_GSO_MPLS = 1 << 12,
>>  
>> +	SKB_GSO_UDP6 = 1 << 13
>>  };
>>  
>>  #if BITS_PER_LONG > 32
> 
> So this implies anything getting GSO packets e.g.
> from userspace now needs to check IP version to
> set GSO type correctly.
> 
> I think you missed some places that do this, e.g. af_packet
> sockets.
> 

I looked at af_packet sockets and they set this only in the event
vnet header has been used with a GSO type.  In this case, the user
already knows the the type.

It is true that with this series af_packets now can't do IPv6 UFO
since there is no VIRTIO_NET_HDR_GSO_UDPV6 yet.

I suppose we could do something similar there as we do in tun code/macvtap code.
If that's the case, it currently broken as well.

-vlad

> 
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 945bbd0..fa4d2ee 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -5929,6 +5929,12 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
>>  		features &= ~NETIF_F_ALL_TSO;
>>  	}
>>  
>> +	/* UFO requires that SG is present as well */
>> +	if ((features & NETIF_F_ALL_UFO) && !(features & NETIF_F_SG)) {
>> +		netdev_dbg(dev, "Dropping UFO features since no SG feature.\n");
>> +		features &= ~NETIF_F_ALL_UFO;
>> +	}
>> +
>>  	if ((features & NETIF_F_TSO) && !(features & NETIF_F_HW_CSUM) &&
>>  					!(features & NETIF_F_IP_CSUM)) {
>>  		netdev_dbg(dev, "Dropping TSO features since no CSUM feature.\n");
>> @@ -5952,24 +5958,21 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
>>  		features &= ~NETIF_F_GSO;
>>  	}
>>  
>> -	/* UFO needs SG and checksumming */
>> -	if (features & NETIF_F_UFO) {
>> -		/* maybe split UFO into V4 and V6? */
>> -		if (!((features & NETIF_F_GEN_CSUM) ||
>> -		    (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
>> -			    == (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
>> -			netdev_dbg(dev,
>> -				"Dropping NETIF_F_UFO since no checksum offload features.\n");
>> -			features &= ~NETIF_F_UFO;
>> -		}
>> -
>> -		if (!(features & NETIF_F_SG)) {
>> -			netdev_dbg(dev,
>> -				"Dropping NETIF_F_UFO since no NETIF_F_SG feature.\n");
>> -			features &= ~NETIF_F_UFO;
>> -		}
>> +	/* UFO also needs checksumming */
>> +	if ((features & NETIF_F_UFO) && !(features & NETIF_F_GEN_CSUM) &&
>> +					!(features & NETIF_F_IP_CSUM)) {
>> +		netdev_dbg(dev,
>> +			   "Dropping NETIF_F_UFO since no checksum offload features.\n");
>> +		features &= ~NETIF_F_UFO;
>> +	}
>> +	if ((features & NETIF_F_UFO6) && !(features & NETIF_F_GEN_CSUM) &&
>> +					 !(features & NETIF_F_IPV6_CSUM)) {
>> +		netdev_dbg(dev,
>> +			   "Dropping NETIF_F_UFO6 since no checksum offload features.\n");
>> +		features &= ~NETIF_F_UFO6;
>>  	}
>>  
>> +
>>  #ifdef CONFIG_NET_RX_BUSY_POLL
>>  	if (dev->netdev_ops->ndo_busy_poll)
>>  		features |= NETIF_F_BUSY_POLL;
>> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
>> index 06dfb29..93eff41 100644
>> --- a/net/core/ethtool.c
>> +++ b/net/core/ethtool.c
>> @@ -223,7 +223,7 @@ static netdev_features_t ethtool_get_feature_mask(u32 eth_cmd)
>>  		return NETIF_F_ALL_TSO;
>>  	case ETHTOOL_GUFO:
>>  	case ETHTOOL_SUFO:
>> -		return NETIF_F_UFO;
>> +		return NETIF_F_ALL_UFO;
>>  	case ETHTOOL_GGSO:
>>  	case ETHTOOL_SGSO:
>>  		return NETIF_F_GSO;
>> -- 
>> 1.9.3

Re: [PATCH 09/10] macvtap: Re-enable UFO support

[Vlad Yasevich]

On 12/17/2014 05:41 PM, Michael S. Tsirkin wrote:
> On Wed, Dec 17, 2014 at 01:20:54PM -0500, Vladislav Yasevich wrote:
>> Now that UFO is split into v4 and v6 parts, we can bring
>> back v4 support.  Continue to handle legacy applications
>> by selecting the ipv6 fagment id but do not change the
>> gso type.  This allows 2 legacy VMs to continue to communicate.
>>
>> Based on original work from Ben Hutchings.
>>
>> Fixes: 88e0e0e5aa7a ("drivers/net: Disable UFO through virtio")
>> CC: Ben Hutchings <ben@decadent.org.uk>
>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>> ---
>>  drivers/net/macvtap.c | 20 ++++++++++++++------
>>  1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>> index 880cc09..75febd4 100644
>> --- a/drivers/net/macvtap.c
>> +++ b/drivers/net/macvtap.c
>> @@ -66,7 +66,7 @@ static struct cdev macvtap_cdev;
>>  static const struct proto_ops macvtap_socket_ops;
>>  
>>  #define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
>> -		      NETIF_F_TSO6)
>> +		      NETIF_F_TSO6 | NETIF_F_UFO)
>>  #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
>>  #define TAP_FEATURES (NETIF_F_GSO | NETIF_F_SG)
>>  
>> @@ -570,11 +570,14 @@ static int macvtap_skb_from_vnet_hdr(struct sk_buff *skb,
>>  			gso_type = SKB_GSO_TCPV6;
>>  			break;
>>  		case VIRTIO_NET_HDR_GSO_UDP:
>> -			pr_warn_once("macvtap: %s: using disabled UFO feature; please fix this program\n",
>> -				     current->comm);
>>  			gso_type = SKB_GSO_UDP;
>> -			if (skb->protocol == htons(ETH_P_IPV6))
>> +			if (vlan_get_protocol(skb) == htons(ETH_P_IPV6)) {
>> +				/* This is to support legacy appliacations.
>> +				 * Do not change the gso_type as legacy apps
>> +				 * may not know about the new type.
>> +				 */
>>  				ipv6_proxy_select_ident(skb);
>> +			}
>>  			break;
>>  		default:
>>  			return -EINVAL;
>> @@ -619,6 +622,8 @@ static void macvtap_skb_to_vnet_hdr(const struct sk_buff *skb,
>>  			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
>>  		else if (sinfo->gso_type & SKB_GSO_TCPV6)
>>  			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
>> +		else if (sinfo->gso_type & SKB_GSO_UDP)
>> +			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP;
>>  		else
>>  			BUG();
>>  		if (sinfo->gso_type & SKB_GSO_TCP_ECN)
>> @@ -955,6 +960,9 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
>>  			if (arg & TUN_F_TSO6)
>>  				feature_mask |= NETIF_F_TSO6;
>>  		}
>> +
>> +		if (arg & TUN_F_UFO)
>> +			feature_mask |= NETIF_F_UFO;
>>  	}
>>  
>>  	/* tun/tap driver inverts the usage for TSO offloads, where
>> @@ -965,7 +973,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
>>  	 * When user space turns off TSO, we turn off GSO/LRO so that
>>  	 * user-space will not receive TSO frames.
>>  	 */
>> -	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6))
>> +	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO))
>>  		features |= RX_OFFLOADS;
>>  	else
>>  		features &= ~RX_OFFLOADS;
> 
> By the way this logic is completely broken, even without your patch,
> isn't it?
> 
> It says: enable LRO+GRO if any of NETIF_F_TSO | NETIF_F_TSO6 |
> NETIF_F_UFO set.
> 
> So what happens if I enable TSO only?
> LRO gets enabled so I can still get TSO6 packets.
> 
> 
> This really should be:
> 
> 	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO) ==
> 			(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO)
> 
> 
> fixing this probably should be a separate patch before your
> series, and Cc stable.

Actually, all this LRO/GRO feature manipulation on the macvtap device is
rather pointless as those features aren't really checked at any point
on the macvtap receive path.  GRO calls are done from napi context on
the lowest device, so by the time a packet hits macvtap, GRO/LRO has
already been done.

So it doesn't really matter that we disable them incorrectly.  I
can send a separate patch to clean this up.

> 
> 
>> @@ -1066,7 +1074,7 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
>>  	case TUNSETOFFLOAD:
>>  		/* let the user check for future flags */
>>  		if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
>> -			    TUN_F_TSO_ECN))
>> +			    TUN_F_TSO_ECN | TUN_F_UFO))
>>  			return -EINVAL;
>>  
>>  		rtnl_lock();
>> -- 
>> 1.9.3

Re: [PATCH 00/10] Split UFO into v4 and v6 versions.

[Jason Wang]

----- Original Message -----
> UFO support in the kernel applies to both IPv4 and IPv6 protocols
> with the same device feature.  However some devices may not be able
> to support one of the offloads.  For this we split the UFO offload
> feature into 2 pieces.  NETIF_F_UFO now controlls the IPv4 part and
> this series introduces NETIF_F_UFO6.
> 
> As a result of this work, we can now re-enable NETIF_F_UFO on
> virtio_net devices and restore UDP over IPv4 performance for guests.
> We also continue to support legacy guests that assume that UFO6
> support included into UFO(4).
> 
> Without this work, migrating a guest to a 3.18 kernel fails.
> 

This series eliminate the ambiguous NETIF_F_UFO.

But VIRTIO_NET_F_{HOST|GUEST}_UFO and VIRTIO_NET_F_HDR_GSO_UDP is still
ambigious. I know it was used to keep compatibility for legacy guest. But
what's the future plan? Differentiate UFOv4 and UFOv6 in virtio features and
gso type in vnet header looks sufficient?

With the series, we can't send UFOv6 packet but legacy guest can. How about
fix the UFOv6 also in this series?

Thanks
> Vladislav Yasevich (10):
>   core: Split out UFO6 support
>   net:  Correctly mark IPv6 UFO offload type.
>   ovs: Enable handling of UFO6 packets.
>   loopback: Turn on UFO6 support.
>   veth: Enable UFO6 support.
>   macvlan: Enable UFO6 support.
>   s2io: Enable UFO6 support.
>   tun: Re-uanble UFO support.
>   macvtap: Re-enable UFO support
>   Revert "drivers/net: Disable UFO through virtio"
> 
>  drivers/net/ethernet/neterion/s2io.c |  6 +++---
>  drivers/net/loopback.c               |  4 ++--
>  drivers/net/macvlan.c                |  2 +-
>  drivers/net/macvtap.c                | 20 ++++++++++++++------
>  drivers/net/tun.c                    | 26 ++++++++++++++------------
>  drivers/net/veth.c                   |  2 +-
>  drivers/net/virtio_net.c             | 24 ++++++++++--------------
>  include/linux/netdev_features.h      |  7 +++++--
>  include/linux/netdevice.h            |  1 +
>  include/linux/skbuff.h               |  1 +
>  net/core/dev.c                       | 35
>  +++++++++++++++++++----------------
>  net/core/ethtool.c                   |  2 +-
>  net/ipv6/ip6_offload.c               |  1 +
>  net/ipv6/ip6_output.c                |  4 ++--
>  net/ipv6/udp_offload.c               |  3 ++-
>  net/mpls/mpls_gso.c                  |  1 +
>  net/openvswitch/datapath.c           |  3 ++-
>  net/openvswitch/flow.c               |  2 +-
>  18 files changed, 81 insertions(+), 63 deletions(-)
> 
> --
> 1.9.3
> 
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> 

Re: [PATCH 00/10] Split UFO into v4 and v6 versions.

[Jason Wang]

----- Original Message -----
> UFO support in the kernel applies to both IPv4 and IPv6 protocols
> with the same device feature.  However some devices may not be able
> to support one of the offloads.  For this we split the UFO offload
> feature into 2 pieces.  NETIF_F_UFO now controlls the IPv4 part and
> this series introduces NETIF_F_UFO6.
> 
> As a result of this work, we can now re-enable NETIF_F_UFO on
> virtio_net devices and restore UDP over IPv4 performance for guests.
> We also continue to support legacy guests that assume that UFO6
> support included into UFO(4).
> 
> Without this work, migrating a guest to a 3.18 kernel fails.
> 

This series eliminate the ambiguous NETIF_F_UFO.

But VIRTIO_NET_F_{HOST|GUEST}_UFO and VIRTIO_NET_F_HDR_GSO_UDP is still
ambigious. I know it was used to keep compatibility for legacy guest. But
what's the future plan? Differentiate UFOv4 and UFOv6 in virtio features and
gso type in vnet header looks sufficient?

With the series, we can't send UFOv6 packet but legacy guest can. How about
fix the UFOv6 also in this series?

Thanks
> Vladislav Yasevich (10):
>   core: Split out UFO6 support
>   net:  Correctly mark IPv6 UFO offload type.
>   ovs: Enable handling of UFO6 packets.
>   loopback: Turn on UFO6 support.
>   veth: Enable UFO6 support.
>   macvlan: Enable UFO6 support.
>   s2io: Enable UFO6 support.
>   tun: Re-uanble UFO support.
>   macvtap: Re-enable UFO support
>   Revert "drivers/net: Disable UFO through virtio"
> 
>  drivers/net/ethernet/neterion/s2io.c |  6 +++---
>  drivers/net/loopback.c               |  4 ++--
>  drivers/net/macvlan.c                |  2 +-
>  drivers/net/macvtap.c                | 20 ++++++++++++++------
>  drivers/net/tun.c                    | 26 ++++++++++++++------------
>  drivers/net/veth.c                   |  2 +-
>  drivers/net/virtio_net.c             | 24 ++++++++++--------------
>  include/linux/netdev_features.h      |  7 +++++--
>  include/linux/netdevice.h            |  1 +
>  include/linux/skbuff.h               |  1 +
>  net/core/dev.c                       | 35
>  +++++++++++++++++++----------------
>  net/core/ethtool.c                   |  2 +-
>  net/ipv6/ip6_offload.c               |  1 +
>  net/ipv6/ip6_output.c                |  4 ++--
>  net/ipv6/udp_offload.c               |  3 ++-
>  net/mpls/mpls_gso.c                  |  1 +
>  net/openvswitch/datapath.c           |  3 ++-
>  net/openvswitch/flow.c               |  2 +-
>  18 files changed, 81 insertions(+), 63 deletions(-)
> 
> --
> 1.9.3
> 
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> 

Re: [PATCH 08/10] tun: Re-uanble UFO support.

[Jason Wang]

----- Original Message -----
> Now that UFO is split into v4 and v6 parts, we can bring
> back v4 support without any trouble.
> 
> Continue to handle legacy applications by selecting the
> IPv6 fragment id but do not change the gso type.  Thist
> makes sure that two legacy VMs may still communicate.
> 
> Based on original work from Ben Hutchings.
> 
> Fixes: 88e0e0e5aa7a ("drivers/net: Disable UFO through virtio")
> CC: Ben Hutchings <ben@decadent.org.uk>
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---
>  drivers/net/tun.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 9dd3746..8c32fca 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -175,7 +175,7 @@ struct tun_struct {
>  	struct net_device	*dev;
>  	netdev_features_t	set_features;
>  #define TUN_USER_FEATURES (NETIF_F_HW_CSUM|NETIF_F_TSO_ECN|NETIF_F_TSO| \
> -			  NETIF_F_TSO6)
> +			  NETIF_F_TSO6|NETIF_F_UFO)
>  
>  	int			vnet_hdr_sz;
>  	int			sndbuf;
> @@ -1152,20 +1152,15 @@ static ssize_t tun_get_user(struct tun_struct *tun,
> struct tun_file *tfile,
>  			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
>  			break;
>  		case VIRTIO_NET_HDR_GSO_UDP:
> -		{
> -			static bool warned;
> -
> -			if (!warned) {
> -				warned = true;
> -				netdev_warn(tun->dev,
> -					    "%s: using disabled UFO feature; please fix this program\n",
> -					    current->comm);
> -			}
>  			skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> -			if (skb->protocol == htons(ETH_P_IPV6))
> +			if (vlan_get_protocol(skb) == htons(ETH_P_IPV6)) {

This probably means UDPv6 with vlan does not work well, looks like another
independent fixe and also for stable.
> +				/* This allows legacy application to work.
> +				 * Do not change the gso_type as it may
> +				 * not be upderstood by legacy applications.
> +				 */

Isn't this an issue that we use SKB_GSO_UDP for a UDPv6 packet? Especially
consider we want to fix UFOv6 in the future? We probably can use
SKB_GSO_UDP6 here and try to fix it in tun_put_user() if a legacy userspace
is detected.
>  				ipv6_proxy_select_ident(skb);

Question still for vlan, is network header correctly set here? Looks like
ipv6_proxy_select_ident() depends on correct network header to work.
> +			}
>  			break;
> -		}
>  		default:
>  			tun->dev->stats.rx_frame_errors++;
>  			kfree_skb(skb);
> @@ -1273,6 +1268,8 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>  				gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
>  			else if (sinfo->gso_type & SKB_GSO_TCPV6)
>  				gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
> +			else if (sinfo->gso_type & SKB_GSO_UDP)
> +				gso.gso_type = VIRTIO_NET_HDR_GSO_UDP;
>  			else {
>  				pr_err("unexpected GSO type: "
>  				       "0x%x, gso_size %d, hdr_len %d\n",
> @@ -1780,6 +1777,11 @@ static int set_offload(struct tun_struct *tun,
> unsigned long arg)
>  				features |= NETIF_F_TSO6;
>  			arg &= ~(TUN_F_TSO4|TUN_F_TSO6);
>  		}
> +
> +		if (arg & TUN_F_UFO) {
> +			features |= NETIF_F_UFO;
> +			arg &= ~TUN_F_UFO;
> +		}
>  	}
>  
>  	/* This gives the user a way to test for new features in future by
> --
> 1.9.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 01/10] core: Split out UFO6 support

[Michael S. Tsirkin]

Cc Dave, pls remember to do this next time otherwise
your patches won't get merged :)

On Wed, Dec 17, 2014 at 06:31:50PM -0500, Vlad Yasevich wrote:
> On 12/17/2014 05:45 PM, Michael S. Tsirkin wrote:
> > On Wed, Dec 17, 2014 at 01:20:46PM -0500, Vladislav Yasevich wrote:
> >> Split IPv6 support for UFO into its own feature similiar to TSO.
> >> This will later allow us to re-enable UFO support for virtio-net
> >> devices.
> >>
> >> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> >> ---
> >>  include/linux/netdev_features.h |  7 +++++--
> >>  include/linux/netdevice.h       |  1 +
> >>  include/linux/skbuff.h          |  1 +
> >>  net/core/dev.c                  | 35 +++++++++++++++++++----------------
> >>  net/core/ethtool.c              |  2 +-
> >>  5 files changed, 27 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> >> index dcfdecb..a078945 100644
> >> --- a/include/linux/netdev_features.h
> >> +++ b/include/linux/netdev_features.h
> >> @@ -48,8 +48,9 @@ enum {
> >>  	NETIF_F_GSO_UDP_TUNNEL_BIT,	/* ... UDP TUNNEL with TSO */
> >>  	NETIF_F_GSO_UDP_TUNNEL_CSUM_BIT,/* ... UDP TUNNEL with TSO & CSUM */
> >>  	NETIF_F_GSO_MPLS_BIT,		/* ... MPLS segmentation */
> >> +	NETIF_F_UFO6_BIT,		/* ... UDPv6 fragmentation */
> >>  	/**/NETIF_F_GSO_LAST =		/* last bit, see GSO_MASK */
> >> -		NETIF_F_GSO_MPLS_BIT,
> >> +		NETIF_F_UFO6_BIT,
> >>  
> >>  	NETIF_F_FCOE_CRC_BIT,		/* FCoE CRC32 */
> >>  	NETIF_F_SCTP_CSUM_BIT,		/* SCTP checksum offload */
> >> @@ -109,6 +110,7 @@ enum {
> >>  #define NETIF_F_TSO_ECN		__NETIF_F(TSO_ECN)
> >>  #define NETIF_F_TSO		__NETIF_F(TSO)
> >>  #define NETIF_F_UFO		__NETIF_F(UFO)
> >> +#define NETIF_F_UFO6		__NETIF_F(UFO6)
> >>  #define NETIF_F_VLAN_CHALLENGED	__NETIF_F(VLAN_CHALLENGED)
> >>  #define NETIF_F_RXFCS		__NETIF_F(RXFCS)
> >>  #define NETIF_F_RXALL		__NETIF_F(RXALL)
> >> @@ -141,7 +143,7 @@ enum {
> >>  
> >>  /* List of features with software fallbacks. */
> >>  #define NETIF_F_GSO_SOFTWARE	(NETIF_F_TSO | NETIF_F_TSO_ECN | \
> >> -				 NETIF_F_TSO6 | NETIF_F_UFO)
> >> +				 NETIF_F_TSO6 | NETIF_F_UFO | NETIF_F_UFO6)
> >>  
> >>  #define NETIF_F_GEN_CSUM	NETIF_F_HW_CSUM
> >>  #define NETIF_F_V4_CSUM		(NETIF_F_GEN_CSUM | NETIF_F_IP_CSUM)
> >> @@ -149,6 +151,7 @@ enum {
> >>  #define NETIF_F_ALL_CSUM	(NETIF_F_V4_CSUM | NETIF_F_V6_CSUM)
> >>  
> >>  #define NETIF_F_ALL_TSO 	(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_TSO_ECN)
> >> +#define NETIF_F_ALL_UFO		(NETIF_F_UFO | NETIF_F_UFO6)
> >>  
> >>  #define NETIF_F_ALL_FCOE	(NETIF_F_FCOE_CRC | NETIF_F_FCOE_MTU | \
> >>  				 NETIF_F_FSO)
> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >> index 74fd5d3..86af10a 100644
> >> --- a/include/linux/netdevice.h
> >> +++ b/include/linux/netdevice.h
> >> @@ -3559,6 +3559,7 @@ static inline bool net_gso_ok(netdev_features_t features, int gso_type)
> >>  	/* check flags correspondence */
> >>  	BUILD_BUG_ON(SKB_GSO_TCPV4   != (NETIF_F_TSO >> NETIF_F_GSO_SHIFT));
> >>  	BUILD_BUG_ON(SKB_GSO_UDP     != (NETIF_F_UFO >> NETIF_F_GSO_SHIFT));
> >> +	BUILD_BUG_ON(SKB_GSO_UDP6    != (NETIF_F_UFO6 >> NETIF_F_GSO_SHIFT));
> >>  	BUILD_BUG_ON(SKB_GSO_DODGY   != (NETIF_F_GSO_ROBUST >> NETIF_F_GSO_SHIFT));
> >>  	BUILD_BUG_ON(SKB_GSO_TCP_ECN != (NETIF_F_TSO_ECN >> NETIF_F_GSO_SHIFT));
> >>  	BUILD_BUG_ON(SKB_GSO_TCPV6   != (NETIF_F_TSO6 >> NETIF_F_GSO_SHIFT));
> >> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> >> index 6c8b6f6..8538b67 100644
> >> --- a/include/linux/skbuff.h
> >> +++ b/include/linux/skbuff.h
> >> @@ -372,6 +372,7 @@ enum {
> >>  
> >>  	SKB_GSO_MPLS = 1 << 12,
> >>  
> >> +	SKB_GSO_UDP6 = 1 << 13
> >>  };
> >>  
> >>  #if BITS_PER_LONG > 32
> > 
> > So this implies anything getting GSO packets e.g.
> > from userspace now needs to check IP version to
> > set GSO type correctly.
> > 
> > I think you missed some places that do this, e.g. af_packet
> > sockets.
> > 
> 
> I looked at af_packet sockets and they set this only in the event
> vnet header has been used with a GSO type.  In this case, the user
> already knows the the type.

Imagine you are receiving a packet:

                if (vnet_hdr.gso_type != VIRTIO_NET_HDR_GSO_NONE) {
                        switch (vnet_hdr.gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
                        case VIRTIO_NET_HDR_GSO_TCPV4:
                                gso_type = SKB_GSO_TCPV4;
                                break;
                        case VIRTIO_NET_HDR_GSO_TCPV6:
                                gso_type = SKB_GSO_TCPV6;
                                break;
                        case VIRTIO_NET_HDR_GSO_UDP:
                                gso_type = SKB_GSO_UDP;
                                break;
                        default:
                                goto out_unlock;
                        }

                        if (vnet_hdr.gso_type & VIRTIO_NET_HDR_GSO_ECN)
                                gso_type |= SKB_GSO_TCP_ECN;

                        if (vnet_hdr.gso_size == 0)
                                goto out_unlock;

                }

we used to report UFO6 as SKB_GSO_UDP, we probably
should keep doing this, with your patch we select the
goto out_unlock path.



> It is true that with this series af_packets now can't do IPv6 UFO
> since there is no VIRTIO_NET_HDR_GSO_UDPV6 yet.

What do you mean by "do".
Are we talking about sending or receiving packets?
You seem to conflate the two.

We always discarded ID on RX.

For tun, this is xmit, so just by saying "this device can
not do UFO" you will never get short packets.


> 
> I suppose we could do something similar there as we do in tun code/macvtap code.
> If that's the case, it currently broken as well.
> 
> -vlad


Broken is a big word.

Let's stop conflating two directions.

Here's the way I look at it:

1. Userspace doesn't have a way to get fragment IDs
from tun/macvtap/packet sockets.
Presumably, not all users need these IDs.
E.g. old guests clearly don't.

We should either give them packets stripping the ID,
like we always did, or make sure they never get these packets.
Second option seems achievable for tun if we
never tell linux it can do UFO, but I don't see
how to do it for macvtap and packet socket.


2. Userspace doesn't have a way to set fragment IDs
for tun/macvtap/packet sockets.
Presumably, not all users have these IDs.  E.g. old
guests clearly don't.

We should either generate our own ID,
like we always did, or make sure we don't accept
these packets.
Second option is almost sure to break userspace,
so it seems we must do the first one.


3.
Exactly the same two things if you replace userspace
with hypervisor and tun/macvtap/packet socket with
virtio device.


4. 
As a next step, we should add a way for userspace
to tell us that it has ids and can handle them.





> > 
> >> diff --git a/net/core/dev.c b/net/core/dev.c
> >> index 945bbd0..fa4d2ee 100644
> >> --- a/net/core/dev.c
> >> +++ b/net/core/dev.c
> >> @@ -5929,6 +5929,12 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
> >>  		features &= ~NETIF_F_ALL_TSO;
> >>  	}
> >>  
> >> +	/* UFO requires that SG is present as well */
> >> +	if ((features & NETIF_F_ALL_UFO) && !(features & NETIF_F_SG)) {
> >> +		netdev_dbg(dev, "Dropping UFO features since no SG feature.\n");
> >> +		features &= ~NETIF_F_ALL_UFO;
> >> +	}
> >> +
> >>  	if ((features & NETIF_F_TSO) && !(features & NETIF_F_HW_CSUM) &&
> >>  					!(features & NETIF_F_IP_CSUM)) {
> >>  		netdev_dbg(dev, "Dropping TSO features since no CSUM feature.\n");
> >> @@ -5952,24 +5958,21 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
> >>  		features &= ~NETIF_F_GSO;
> >>  	}
> >>  
> >> -	/* UFO needs SG and checksumming */
> >> -	if (features & NETIF_F_UFO) {
> >> -		/* maybe split UFO into V4 and V6? */
> >> -		if (!((features & NETIF_F_GEN_CSUM) ||
> >> -		    (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
> >> -			    == (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
> >> -			netdev_dbg(dev,
> >> -				"Dropping NETIF_F_UFO since no checksum offload features.\n");
> >> -			features &= ~NETIF_F_UFO;
> >> -		}
> >> -
> >> -		if (!(features & NETIF_F_SG)) {
> >> -			netdev_dbg(dev,
> >> -				"Dropping NETIF_F_UFO since no NETIF_F_SG feature.\n");
> >> -			features &= ~NETIF_F_UFO;
> >> -		}
> >> +	/* UFO also needs checksumming */
> >> +	if ((features & NETIF_F_UFO) && !(features & NETIF_F_GEN_CSUM) &&
> >> +					!(features & NETIF_F_IP_CSUM)) {
> >> +		netdev_dbg(dev,
> >> +			   "Dropping NETIF_F_UFO since no checksum offload features.\n");
> >> +		features &= ~NETIF_F_UFO;
> >> +	}
> >> +	if ((features & NETIF_F_UFO6) && !(features & NETIF_F_GEN_CSUM) &&
> >> +					 !(features & NETIF_F_IPV6_CSUM)) {
> >> +		netdev_dbg(dev,
> >> +			   "Dropping NETIF_F_UFO6 since no checksum offload features.\n");
> >> +		features &= ~NETIF_F_UFO6;
> >>  	}
> >>  
> >> +
> >>  #ifdef CONFIG_NET_RX_BUSY_POLL
> >>  	if (dev->netdev_ops->ndo_busy_poll)
> >>  		features |= NETIF_F_BUSY_POLL;
> >> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> >> index 06dfb29..93eff41 100644
> >> --- a/net/core/ethtool.c
> >> +++ b/net/core/ethtool.c
> >> @@ -223,7 +223,7 @@ static netdev_features_t ethtool_get_feature_mask(u32 eth_cmd)
> >>  		return NETIF_F_ALL_TSO;
> >>  	case ETHTOOL_GUFO:
> >>  	case ETHTOOL_SUFO:
> >> -		return NETIF_F_UFO;
> >> +		return NETIF_F_ALL_UFO;
> >>  	case ETHTOOL_GGSO:
> >>  	case ETHTOOL_SGSO:
> >>  		return NETIF_F_GSO;
> >> -- 
> >> 1.9.3

Re: [PATCH 09/10] macvtap: Re-enable UFO support

[Michael S. Tsirkin]

On Wed, Dec 17, 2014 at 09:43:51PM -0500, Vlad Yasevich wrote:
> On 12/17/2014 05:41 PM, Michael S. Tsirkin wrote:
> > On Wed, Dec 17, 2014 at 01:20:54PM -0500, Vladislav Yasevich wrote:
> >> Now that UFO is split into v4 and v6 parts, we can bring
> >> back v4 support.  Continue to handle legacy applications
> >> by selecting the ipv6 fagment id but do not change the
> >> gso type.  This allows 2 legacy VMs to continue to communicate.
> >>
> >> Based on original work from Ben Hutchings.
> >>
> >> Fixes: 88e0e0e5aa7a ("drivers/net: Disable UFO through virtio")
> >> CC: Ben Hutchings <ben@decadent.org.uk>
> >> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> >> ---
> >>  drivers/net/macvtap.c | 20 ++++++++++++++------
> >>  1 file changed, 14 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> >> index 880cc09..75febd4 100644
> >> --- a/drivers/net/macvtap.c
> >> +++ b/drivers/net/macvtap.c
> >> @@ -66,7 +66,7 @@ static struct cdev macvtap_cdev;
> >>  static const struct proto_ops macvtap_socket_ops;
> >>  
> >>  #define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> >> -		      NETIF_F_TSO6)
> >> +		      NETIF_F_TSO6 | NETIF_F_UFO)
> >>  #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
> >>  #define TAP_FEATURES (NETIF_F_GSO | NETIF_F_SG)
> >>  
> >> @@ -570,11 +570,14 @@ static int macvtap_skb_from_vnet_hdr(struct sk_buff *skb,
> >>  			gso_type = SKB_GSO_TCPV6;
> >>  			break;
> >>  		case VIRTIO_NET_HDR_GSO_UDP:
> >> -			pr_warn_once("macvtap: %s: using disabled UFO feature; please fix this program\n",
> >> -				     current->comm);
> >>  			gso_type = SKB_GSO_UDP;
> >> -			if (skb->protocol == htons(ETH_P_IPV6))
> >> +			if (vlan_get_protocol(skb) == htons(ETH_P_IPV6)) {
> >> +				/* This is to support legacy appliacations.
> >> +				 * Do not change the gso_type as legacy apps
> >> +				 * may not know about the new type.
> >> +				 */
> >>  				ipv6_proxy_select_ident(skb);
> >> +			}
> >>  			break;
> >>  		default:
> >>  			return -EINVAL;
> >> @@ -619,6 +622,8 @@ static void macvtap_skb_to_vnet_hdr(const struct sk_buff *skb,
> >>  			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
> >>  		else if (sinfo->gso_type & SKB_GSO_TCPV6)
> >>  			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
> >> +		else if (sinfo->gso_type & SKB_GSO_UDP)
> >> +			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP;
> >>  		else
> >>  			BUG();
> >>  		if (sinfo->gso_type & SKB_GSO_TCP_ECN)
> >> @@ -955,6 +960,9 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
> >>  			if (arg & TUN_F_TSO6)
> >>  				feature_mask |= NETIF_F_TSO6;
> >>  		}
> >> +
> >> +		if (arg & TUN_F_UFO)
> >> +			feature_mask |= NETIF_F_UFO;
> >>  	}
> >>  
> >>  	/* tun/tap driver inverts the usage for TSO offloads, where
> >> @@ -965,7 +973,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
> >>  	 * When user space turns off TSO, we turn off GSO/LRO so that
> >>  	 * user-space will not receive TSO frames.
> >>  	 */
> >> -	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6))
> >> +	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO))
> >>  		features |= RX_OFFLOADS;
> >>  	else
> >>  		features &= ~RX_OFFLOADS;
> > 
> > By the way this logic is completely broken, even without your patch,
> > isn't it?
> > 
> > It says: enable LRO+GRO if any of NETIF_F_TSO | NETIF_F_TSO6 |
> > NETIF_F_UFO set.
> > 
> > So what happens if I enable TSO only?
> > LRO gets enabled so I can still get TSO6 packets.
> > 
> > 
> > This really should be:
> > 
> > 	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO) ==
> > 			(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO)
> > 
> > 
> > fixing this probably should be a separate patch before your
> > series, and Cc stable.
> 
> Actually, all this LRO/GRO feature manipulation on the macvtap device is
> rather pointless as those features aren't really checked at any point
> on the macvtap receive path.  GRO calls are done from napi context on
> the lowest device, so by the time a packet hits macvtap, GRO/LRO has
> already been done.
> 
> So it doesn't really matter that we disable them incorrectly.  I
> can send a separate patch to clean this up.

Hmm so if userspace says it can't handle TSO packets,
it might get them anyway?


> > 
> > 
> >> @@ -1066,7 +1074,7 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
> >>  	case TUNSETOFFLOAD:
> >>  		/* let the user check for future flags */
> >>  		if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
> >> -			    TUN_F_TSO_ECN))
> >> +			    TUN_F_TSO_ECN | TUN_F_UFO))
> >>  			return -EINVAL;
> >>  
> >>  		rtnl_lock();
> >> -- 
> >> 1.9.3

Re: [PATCH 01/10] core: Split out UFO6 support

[Vlad Yasevich]

On 12/18/2014 02:54 AM, Michael S. Tsirkin wrote:
> Cc Dave, pls remember to do this next time otherwise
> your patches won't get merged :)
> 
> On Wed, Dec 17, 2014 at 06:31:50PM -0500, Vlad Yasevich wrote:
>> On 12/17/2014 05:45 PM, Michael S. Tsirkin wrote:
>>> On Wed, Dec 17, 2014 at 01:20:46PM -0500, Vladislav Yasevich wrote:
>>>> Split IPv6 support for UFO into its own feature similiar to TSO.
>>>> This will later allow us to re-enable UFO support for virtio-net
>>>> devices.
>>>>
>>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>>>> ---
>>>>  include/linux/netdev_features.h |  7 +++++--
>>>>  include/linux/netdevice.h       |  1 +
>>>>  include/linux/skbuff.h          |  1 +
>>>>  net/core/dev.c                  | 35 +++++++++++++++++++----------------
>>>>  net/core/ethtool.c              |  2 +-
>>>>  5 files changed, 27 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
>>>> index dcfdecb..a078945 100644
>>>> --- a/include/linux/netdev_features.h
>>>> +++ b/include/linux/netdev_features.h
>>>> @@ -48,8 +48,9 @@ enum {
>>>>  	NETIF_F_GSO_UDP_TUNNEL_BIT,	/* ... UDP TUNNEL with TSO */
>>>>  	NETIF_F_GSO_UDP_TUNNEL_CSUM_BIT,/* ... UDP TUNNEL with TSO & CSUM */
>>>>  	NETIF_F_GSO_MPLS_BIT,		/* ... MPLS segmentation */
>>>> +	NETIF_F_UFO6_BIT,		/* ... UDPv6 fragmentation */
>>>>  	/**/NETIF_F_GSO_LAST =		/* last bit, see GSO_MASK */
>>>> -		NETIF_F_GSO_MPLS_BIT,
>>>> +		NETIF_F_UFO6_BIT,
>>>>  
>>>>  	NETIF_F_FCOE_CRC_BIT,		/* FCoE CRC32 */
>>>>  	NETIF_F_SCTP_CSUM_BIT,		/* SCTP checksum offload */
>>>> @@ -109,6 +110,7 @@ enum {
>>>>  #define NETIF_F_TSO_ECN		__NETIF_F(TSO_ECN)
>>>>  #define NETIF_F_TSO		__NETIF_F(TSO)
>>>>  #define NETIF_F_UFO		__NETIF_F(UFO)
>>>> +#define NETIF_F_UFO6		__NETIF_F(UFO6)
>>>>  #define NETIF_F_VLAN_CHALLENGED	__NETIF_F(VLAN_CHALLENGED)
>>>>  #define NETIF_F_RXFCS		__NETIF_F(RXFCS)
>>>>  #define NETIF_F_RXALL		__NETIF_F(RXALL)
>>>> @@ -141,7 +143,7 @@ enum {
>>>>  
>>>>  /* List of features with software fallbacks. */
>>>>  #define NETIF_F_GSO_SOFTWARE	(NETIF_F_TSO | NETIF_F_TSO_ECN | \
>>>> -				 NETIF_F_TSO6 | NETIF_F_UFO)
>>>> +				 NETIF_F_TSO6 | NETIF_F_UFO | NETIF_F_UFO6)
>>>>  
>>>>  #define NETIF_F_GEN_CSUM	NETIF_F_HW_CSUM
>>>>  #define NETIF_F_V4_CSUM		(NETIF_F_GEN_CSUM | NETIF_F_IP_CSUM)
>>>> @@ -149,6 +151,7 @@ enum {
>>>>  #define NETIF_F_ALL_CSUM	(NETIF_F_V4_CSUM | NETIF_F_V6_CSUM)
>>>>  
>>>>  #define NETIF_F_ALL_TSO 	(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_TSO_ECN)
>>>> +#define NETIF_F_ALL_UFO		(NETIF_F_UFO | NETIF_F_UFO6)
>>>>  
>>>>  #define NETIF_F_ALL_FCOE	(NETIF_F_FCOE_CRC | NETIF_F_FCOE_MTU | \
>>>>  				 NETIF_F_FSO)
>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>>> index 74fd5d3..86af10a 100644
>>>> --- a/include/linux/netdevice.h
>>>> +++ b/include/linux/netdevice.h
>>>> @@ -3559,6 +3559,7 @@ static inline bool net_gso_ok(netdev_features_t features, int gso_type)
>>>>  	/* check flags correspondence */
>>>>  	BUILD_BUG_ON(SKB_GSO_TCPV4   != (NETIF_F_TSO >> NETIF_F_GSO_SHIFT));
>>>>  	BUILD_BUG_ON(SKB_GSO_UDP     != (NETIF_F_UFO >> NETIF_F_GSO_SHIFT));
>>>> +	BUILD_BUG_ON(SKB_GSO_UDP6    != (NETIF_F_UFO6 >> NETIF_F_GSO_SHIFT));
>>>>  	BUILD_BUG_ON(SKB_GSO_DODGY   != (NETIF_F_GSO_ROBUST >> NETIF_F_GSO_SHIFT));
>>>>  	BUILD_BUG_ON(SKB_GSO_TCP_ECN != (NETIF_F_TSO_ECN >> NETIF_F_GSO_SHIFT));
>>>>  	BUILD_BUG_ON(SKB_GSO_TCPV6   != (NETIF_F_TSO6 >> NETIF_F_GSO_SHIFT));
>>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>>> index 6c8b6f6..8538b67 100644
>>>> --- a/include/linux/skbuff.h
>>>> +++ b/include/linux/skbuff.h
>>>> @@ -372,6 +372,7 @@ enum {
>>>>  
>>>>  	SKB_GSO_MPLS = 1 << 12,
>>>>  
>>>> +	SKB_GSO_UDP6 = 1 << 13
>>>>  };
>>>>  
>>>>  #if BITS_PER_LONG > 32
>>>
>>> So this implies anything getting GSO packets e.g.
>>> from userspace now needs to check IP version to
>>> set GSO type correctly.
>>>
>>> I think you missed some places that do this, e.g. af_packet
>>> sockets.
>>>
>>
>> I looked at af_packet sockets and they set this only in the event
>> vnet header has been used with a GSO type.  In this case, the user
>> already knows the the type.
> 
> Imagine you are receiving a packet:
> 
>                 if (vnet_hdr.gso_type != VIRTIO_NET_HDR_GSO_NONE) {
>                         switch (vnet_hdr.gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
>                         case VIRTIO_NET_HDR_GSO_TCPV4:
>                                 gso_type = SKB_GSO_TCPV4;
>                                 break;
>                         case VIRTIO_NET_HDR_GSO_TCPV6:
>                                 gso_type = SKB_GSO_TCPV6;
>                                 break;
>                         case VIRTIO_NET_HDR_GSO_UDP:
>                                 gso_type = SKB_GSO_UDP;
>                                 break;
>                         default:
>                                 goto out_unlock;
>                         }
> 
>                         if (vnet_hdr.gso_type & VIRTIO_NET_HDR_GSO_ECN)
>                                 gso_type |= SKB_GSO_TCP_ECN;
> 
>                         if (vnet_hdr.gso_size == 0)
>                                 goto out_unlock;
> 
>                 }
> 
> we used to report UFO6 as SKB_GSO_UDP, we probably
> should keep doing this, with your patch we select the
> goto out_unlock path.
> 
> 

No.  The vnet_hdr.gso_type is still going to be VIRTIO_NET_HDR_GSO_UDP
since the UDP6 version isn't defined yet.  So, it will be marked as
GSO_UDP.

This code most likely needs the same workaround as exists in tap and macvtap,
i.e select the proxy fragment id and decide what to do with gso_type.

> 
>> It is true that with this series af_packets now can't do IPv6 UFO
>> since there is no VIRTIO_NET_HDR_GSO_UDPV6 yet.
> 
> What do you mean by "do".

What I mean is that AF_PACKET sockets currently do not do IPv6 UFO
correctly, even after Ben's fixes to tap/macvtap.  There is no
proxy fragment id selection in af_packet case and we have the
same problem Ben was trying address for tap/macvtap.

> Are we talking about sending or receiving packets?

I am talking about sending, see above.

> You seem to conflate the two.
> 
> We always discarded ID on RX.
> 
> For tun, this is xmit, so just by saying "this device can
> not do UFO" you will never get short packets.
> 

You must mean long packets.  This is actually an issue I've been thinking
about. With with your suggestion of switching the GSO type for legacy
applications we end up with fragments for IPv6 traffic.   As a result,
legacy VMs will see a regression for large IPv6 datagrams.

> 
>>
>> I suppose we could do something similar there as we do in tun code/macvtap code.
>> If that's the case, it currently broken as well.
>>
>> -vlad
> 
> 
> Broken is a big word.
> 
> Let's stop conflating two directions.

I am not and was talking only about af_packet as that is what you asked about.
There is no tun/macvtap in play here.  They are handled separately in their
respective drivers.

> 
> Here's the way I look at it:
> 
> 1. Userspace doesn't have a way to get fragment IDs
> from tun/macvtap/packet sockets.
> Presumably, not all users need these IDs.
> E.g. old guests clearly don't.
> 
> We should either give them packets stripping the ID,
> like we always did, or make sure they never get these packets.
> Second option seems achievable for tun if we
> never tell linux it can do UFO, but I don't see
> how to do it for macvtap and packet socket.
> 

macvtap is slightly problematic, but doable with some tricks.
packet socket is an interesting problem.  The only way
an af_packet socket can receive an skb marked SKB_GSO_UDPV6
is if someone else on the host sent it (like another guest).
Since there is are no feature or vnet header length negotiations,
it is impossible to tell if an application using this af_packet
socket is capable of processing VIRTIO_NET_HDR_GSO_UDPV6
type (yet to be defined).

So, we can either use existing VIRTIO_NET_HDR_GSO_UDP on receive
path, add some kind of negotiation logic to packet socket (like
storing the application expected vnet header size), or perform
IPv6 fragmentation somehow.

Options 1 or 2 are doable.

> 
> 2. Userspace doesn't have a way to set fragment IDs
> for tun/macvtap/packet sockets.
> Presumably, not all users have these IDs.  E.g. old
> guests clearly don't.
> 
> We should either generate our own ID,
> like we always did, or make sure we don't accept
> these packets.
> Second option is almost sure to break userspace,
> so it seems we must do the first one.
> 

Right.  This was missing from packet sockets.  I can fix it.

-vlad
> 
> 3.
> Exactly the same two things if you replace userspace
> with hypervisor and tun/macvtap/packet socket with
> virtio device.
> 
> 
> 4. 
> As a next step, we should add a way for userspace
> to tell us that it has ids and can handle them.
> 
> 
> 
> 
> 
>>>
>>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>>> index 945bbd0..fa4d2ee 100644
>>>> --- a/net/core/dev.c
>>>> +++ b/net/core/dev.c
>>>> @@ -5929,6 +5929,12 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
>>>>  		features &= ~NETIF_F_ALL_TSO;
>>>>  	}
>>>>  
>>>> +	/* UFO requires that SG is present as well */
>>>> +	if ((features & NETIF_F_ALL_UFO) && !(features & NETIF_F_SG)) {
>>>> +		netdev_dbg(dev, "Dropping UFO features since no SG feature.\n");
>>>> +		features &= ~NETIF_F_ALL_UFO;
>>>> +	}
>>>> +
>>>>  	if ((features & NETIF_F_TSO) && !(features & NETIF_F_HW_CSUM) &&
>>>>  					!(features & NETIF_F_IP_CSUM)) {
>>>>  		netdev_dbg(dev, "Dropping TSO features since no CSUM feature.\n");
>>>> @@ -5952,24 +5958,21 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
>>>>  		features &= ~NETIF_F_GSO;
>>>>  	}
>>>>  
>>>> -	/* UFO needs SG and checksumming */
>>>> -	if (features & NETIF_F_UFO) {
>>>> -		/* maybe split UFO into V4 and V6? */
>>>> -		if (!((features & NETIF_F_GEN_CSUM) ||
>>>> -		    (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
>>>> -			    == (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
>>>> -			netdev_dbg(dev,
>>>> -				"Dropping NETIF_F_UFO since no checksum offload features.\n");
>>>> -			features &= ~NETIF_F_UFO;
>>>> -		}
>>>> -
>>>> -		if (!(features & NETIF_F_SG)) {
>>>> -			netdev_dbg(dev,
>>>> -				"Dropping NETIF_F_UFO since no NETIF_F_SG feature.\n");
>>>> -			features &= ~NETIF_F_UFO;
>>>> -		}
>>>> +	/* UFO also needs checksumming */
>>>> +	if ((features & NETIF_F_UFO) && !(features & NETIF_F_GEN_CSUM) &&
>>>> +					!(features & NETIF_F_IP_CSUM)) {
>>>> +		netdev_dbg(dev,
>>>> +			   "Dropping NETIF_F_UFO since no checksum offload features.\n");
>>>> +		features &= ~NETIF_F_UFO;
>>>> +	}
>>>> +	if ((features & NETIF_F_UFO6) && !(features & NETIF_F_GEN_CSUM) &&
>>>> +					 !(features & NETIF_F_IPV6_CSUM)) {
>>>> +		netdev_dbg(dev,
>>>> +			   "Dropping NETIF_F_UFO6 since no checksum offload features.\n");
>>>> +		features &= ~NETIF_F_UFO6;
>>>>  	}
>>>>  
>>>> +
>>>>  #ifdef CONFIG_NET_RX_BUSY_POLL
>>>>  	if (dev->netdev_ops->ndo_busy_poll)
>>>>  		features |= NETIF_F_BUSY_POLL;
>>>> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
>>>> index 06dfb29..93eff41 100644
>>>> --- a/net/core/ethtool.c
>>>> +++ b/net/core/ethtool.c
>>>> @@ -223,7 +223,7 @@ static netdev_features_t ethtool_get_feature_mask(u32 eth_cmd)
>>>>  		return NETIF_F_ALL_TSO;
>>>>  	case ETHTOOL_GUFO:
>>>>  	case ETHTOOL_SUFO:
>>>> -		return NETIF_F_UFO;
>>>> +		return NETIF_F_ALL_UFO;
>>>>  	case ETHTOOL_GGSO:
>>>>  	case ETHTOOL_SGSO:
>>>>  		return NETIF_F_GSO;
>>>> -- 
>>>> 1.9.3

Re: [PATCH 08/10] tun: Re-uanble UFO support.

[Vlad Yasevich]

On 12/18/2014 12:51 AM, Jason Wang wrote:
> 
> 
> ----- Original Message -----
>> Now that UFO is split into v4 and v6 parts, we can bring
>> back v4 support without any trouble.
>>
>> Continue to handle legacy applications by selecting the
>> IPv6 fragment id but do not change the gso type.  Thist
>> makes sure that two legacy VMs may still communicate.
>>
>> Based on original work from Ben Hutchings.
>>
>> Fixes: 88e0e0e5aa7a ("drivers/net: Disable UFO through virtio")
>> CC: Ben Hutchings <ben@decadent.org.uk>
>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>> ---
>>  drivers/net/tun.c | 26 ++++++++++++++------------
>>  1 file changed, 14 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index 9dd3746..8c32fca 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
>> @@ -175,7 +175,7 @@ struct tun_struct {
>>  	struct net_device	*dev;
>>  	netdev_features_t	set_features;
>>  #define TUN_USER_FEATURES (NETIF_F_HW_CSUM|NETIF_F_TSO_ECN|NETIF_F_TSO| \
>> -			  NETIF_F_TSO6)
>> +			  NETIF_F_TSO6|NETIF_F_UFO)
>>  
>>  	int			vnet_hdr_sz;
>>  	int			sndbuf;
>> @@ -1152,20 +1152,15 @@ static ssize_t tun_get_user(struct tun_struct *tun,
>> struct tun_file *tfile,
>>  			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
>>  			break;
>>  		case VIRTIO_NET_HDR_GSO_UDP:
>> -		{
>> -			static bool warned;
>> -
>> -			if (!warned) {
>> -				warned = true;
>> -				netdev_warn(tun->dev,
>> -					    "%s: using disabled UFO feature; please fix this program\n",
>> -					    current->comm);
>> -			}
>>  			skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
>> -			if (skb->protocol == htons(ETH_P_IPV6))
>> +			if (vlan_get_protocol(skb) == htons(ETH_P_IPV6)) {
> 
> This probably means UDPv6 with vlan does not work well, looks like another
> independent fixe and also for stable.

Ok.  I can split this out.

>> +				/* This allows legacy application to work.
>> +				 * Do not change the gso_type as it may
>> +				 * not be upderstood by legacy applications.
>> +				 */
> 
> Isn't this an issue that we use SKB_GSO_UDP for a UDPv6 packet? Especially
> consider we want to fix UFOv6 in the future? We probably can use
> SKB_GSO_UDP6 here and try to fix it in tun_put_user() if a legacy userspace
> is detected.

There is a slight problem with this.  This will force fragmentation of IPv6
traffic between VMs since UFO6 wouldn't be enabled on a destination device.
So, suddenly older VMs will see a performance regression for large UDPv6 traffic.

With this code, a legacy VM will continue to receive large UDPv6 traffic.
New VMs will have an updated virtio-net which will reset the gso type on input.

>>  				ipv6_proxy_select_ident(skb);
> 
> Question still for vlan, is network header correctly set here? Looks like
> ipv6_proxy_select_ident() depends on correct network header to work.
>> +	

Yes, you are right...  I wonder how that worked.  I'll re-test.

Thanks
-vlad
		}
>>  			break;
>> -		}
>>  		default:
>>  			tun->dev->stats.rx_frame_errors++;
>>  			kfree_skb(skb);
>> @@ -1273,6 +1268,8 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>>  				gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
>>  			else if (sinfo->gso_type & SKB_GSO_TCPV6)
>>  				gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
>> +			else if (sinfo->gso_type & SKB_GSO_UDP)
>> +				gso.gso_type = VIRTIO_NET_HDR_GSO_UDP;
>>  			else {
>>  				pr_err("unexpected GSO type: "
>>  				       "0x%x, gso_size %d, hdr_len %d\n",
>> @@ -1780,6 +1777,11 @@ static int set_offload(struct tun_struct *tun,
>> unsigned long arg)
>>  				features |= NETIF_F_TSO6;
>>  			arg &= ~(TUN_F_TSO4|TUN_F_TSO6);
>>  		}
>> +
>> +		if (arg & TUN_F_UFO) {
>> +			features |= NETIF_F_UFO;
>> +			arg &= ~TUN_F_UFO;
>> +		}
>>  	}
>>  
>>  	/* This gives the user a way to test for new features in future by
>> --
>> 1.9.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

Re: [PATCH 09/10] macvtap: Re-enable UFO support

[Vlad Yasevich]

On 12/18/2014 02:55 AM, Michael S. Tsirkin wrote:
> On Wed, Dec 17, 2014 at 09:43:51PM -0500, Vlad Yasevich wrote:
>> On 12/17/2014 05:41 PM, Michael S. Tsirkin wrote:
>>> On Wed, Dec 17, 2014 at 01:20:54PM -0500, Vladislav Yasevich wrote:
>>>> Now that UFO is split into v4 and v6 parts, we can bring
>>>> back v4 support.  Continue to handle legacy applications
>>>> by selecting the ipv6 fagment id but do not change the
>>>> gso type.  This allows 2 legacy VMs to continue to communicate.
>>>>
>>>> Based on original work from Ben Hutchings.
>>>>
>>>> Fixes: 88e0e0e5aa7a ("drivers/net: Disable UFO through virtio")
>>>> CC: Ben Hutchings <ben@decadent.org.uk>
>>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>>>> ---
>>>>  drivers/net/macvtap.c | 20 ++++++++++++++------
>>>>  1 file changed, 14 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>>>> index 880cc09..75febd4 100644
>>>> --- a/drivers/net/macvtap.c
>>>> +++ b/drivers/net/macvtap.c
>>>> @@ -66,7 +66,7 @@ static struct cdev macvtap_cdev;
>>>>  static const struct proto_ops macvtap_socket_ops;
>>>>  
>>>>  #define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
>>>> -		      NETIF_F_TSO6)
>>>> +		      NETIF_F_TSO6 | NETIF_F_UFO)
>>>>  #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
>>>>  #define TAP_FEATURES (NETIF_F_GSO | NETIF_F_SG)
>>>>  
>>>> @@ -570,11 +570,14 @@ static int macvtap_skb_from_vnet_hdr(struct sk_buff *skb,
>>>>  			gso_type = SKB_GSO_TCPV6;
>>>>  			break;
>>>>  		case VIRTIO_NET_HDR_GSO_UDP:
>>>> -			pr_warn_once("macvtap: %s: using disabled UFO feature; please fix this program\n",
>>>> -				     current->comm);
>>>>  			gso_type = SKB_GSO_UDP;
>>>> -			if (skb->protocol == htons(ETH_P_IPV6))
>>>> +			if (vlan_get_protocol(skb) == htons(ETH_P_IPV6)) {
>>>> +				/* This is to support legacy appliacations.
>>>> +				 * Do not change the gso_type as legacy apps
>>>> +				 * may not know about the new type.
>>>> +				 */
>>>>  				ipv6_proxy_select_ident(skb);
>>>> +			}
>>>>  			break;
>>>>  		default:
>>>>  			return -EINVAL;
>>>> @@ -619,6 +622,8 @@ static void macvtap_skb_to_vnet_hdr(const struct sk_buff *skb,
>>>>  			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
>>>>  		else if (sinfo->gso_type & SKB_GSO_TCPV6)
>>>>  			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
>>>> +		else if (sinfo->gso_type & SKB_GSO_UDP)
>>>> +			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP;
>>>>  		else
>>>>  			BUG();
>>>>  		if (sinfo->gso_type & SKB_GSO_TCP_ECN)
>>>> @@ -955,6 +960,9 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
>>>>  			if (arg & TUN_F_TSO6)
>>>>  				feature_mask |= NETIF_F_TSO6;
>>>>  		}
>>>> +
>>>> +		if (arg & TUN_F_UFO)
>>>> +			feature_mask |= NETIF_F_UFO;
>>>>  	}
>>>>  
>>>>  	/* tun/tap driver inverts the usage for TSO offloads, where
>>>> @@ -965,7 +973,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
>>>>  	 * When user space turns off TSO, we turn off GSO/LRO so that
>>>>  	 * user-space will not receive TSO frames.
>>>>  	 */
>>>> -	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6))
>>>> +	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO))
>>>>  		features |= RX_OFFLOADS;
>>>>  	else
>>>>  		features &= ~RX_OFFLOADS;
>>>
>>> By the way this logic is completely broken, even without your patch,
>>> isn't it?
>>>
>>> It says: enable LRO+GRO if any of NETIF_F_TSO | NETIF_F_TSO6 |
>>> NETIF_F_UFO set.
>>>
>>> So what happens if I enable TSO only?
>>> LRO gets enabled so I can still get TSO6 packets.
>>>
>>>
>>> This really should be:
>>>
>>> 	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO) ==
>>> 			(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO)
>>>
>>>
>>> fixing this probably should be a separate patch before your
>>> series, and Cc stable.
>>
>> Actually, all this LRO/GRO feature manipulation on the macvtap device is
>> rather pointless as those features aren't really checked at any point
>> on the macvtap receive path.  GRO calls are done from napi context on
>> the lowest device, so by the time a packet hits macvtap, GRO/LRO has
>> already been done.
>>
>> So it doesn't really matter that we disable them incorrectly.  I
>> can send a separate patch to clean this up.
> 
> Hmm so if userspace says it can't handle TSO packets,
> it might get them anyway?

No.  It will get individual segments because in macvtap_handle_frame
we use user specified features to decide if segmentation needs
to be performed or not.

-vlad

> 
> 
>>>
>>>
>>>> @@ -1066,7 +1074,7 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
>>>>  	case TUNSETOFFLOAD:
>>>>  		/* let the user check for future flags */
>>>>  		if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
>>>> -			    TUN_F_TSO_ECN))
>>>> +			    TUN_F_TSO_ECN | TUN_F_UFO))
>>>>  			return -EINVAL;
>>>>  
>>>>  		rtnl_lock();
>>>> -- 
>>>> 1.9.3

Re: [PATCH 09/10] macvtap: Re-enable UFO support

[Vlad Yasevich]

On 12/18/2014 02:55 AM, Michael S. Tsirkin wrote:
> On Wed, Dec 17, 2014 at 09:43:51PM -0500, Vlad Yasevich wrote:
>> On 12/17/2014 05:41 PM, Michael S. Tsirkin wrote:
>>> On Wed, Dec 17, 2014 at 01:20:54PM -0500, Vladislav Yasevich wrote:
>>>> Now that UFO is split into v4 and v6 parts, we can bring
>>>> back v4 support.  Continue to handle legacy applications
>>>> by selecting the ipv6 fagment id but do not change the
>>>> gso type.  This allows 2 legacy VMs to continue to communicate.
>>>>
>>>> Based on original work from Ben Hutchings.
>>>>
>>>> Fixes: 88e0e0e5aa7a ("drivers/net: Disable UFO through virtio")
>>>> CC: Ben Hutchings <ben@decadent.org.uk>
>>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>>>> ---
>>>>  drivers/net/macvtap.c | 20 ++++++++++++++------
>>>>  1 file changed, 14 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>>>> index 880cc09..75febd4 100644
>>>> --- a/drivers/net/macvtap.c
>>>> +++ b/drivers/net/macvtap.c
>>>> @@ -66,7 +66,7 @@ static struct cdev macvtap_cdev;
>>>>  static const struct proto_ops macvtap_socket_ops;
>>>>  
>>>>  #define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
>>>> -		      NETIF_F_TSO6)
>>>> +		      NETIF_F_TSO6 | NETIF_F_UFO)
>>>>  #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
>>>>  #define TAP_FEATURES (NETIF_F_GSO | NETIF_F_SG)
>>>>  
>>>> @@ -570,11 +570,14 @@ static int macvtap_skb_from_vnet_hdr(struct sk_buff *skb,
>>>>  			gso_type = SKB_GSO_TCPV6;
>>>>  			break;
>>>>  		case VIRTIO_NET_HDR_GSO_UDP:
>>>> -			pr_warn_once("macvtap: %s: using disabled UFO feature; please fix this program\n",
>>>> -				     current->comm);
>>>>  			gso_type = SKB_GSO_UDP;
>>>> -			if (skb->protocol == htons(ETH_P_IPV6))
>>>> +			if (vlan_get_protocol(skb) == htons(ETH_P_IPV6)) {
>>>> +				/* This is to support legacy appliacations.
>>>> +				 * Do not change the gso_type as legacy apps
>>>> +				 * may not know about the new type.
>>>> +				 */
>>>>  				ipv6_proxy_select_ident(skb);
>>>> +			}
>>>>  			break;
>>>>  		default:
>>>>  			return -EINVAL;
>>>> @@ -619,6 +622,8 @@ static void macvtap_skb_to_vnet_hdr(const struct sk_buff *skb,
>>>>  			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
>>>>  		else if (sinfo->gso_type & SKB_GSO_TCPV6)
>>>>  			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
>>>> +		else if (sinfo->gso_type & SKB_GSO_UDP)
>>>> +			vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP;
>>>>  		else
>>>>  			BUG();
>>>>  		if (sinfo->gso_type & SKB_GSO_TCP_ECN)
>>>> @@ -955,6 +960,9 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
>>>>  			if (arg & TUN_F_TSO6)
>>>>  				feature_mask |= NETIF_F_TSO6;
>>>>  		}
>>>> +
>>>> +		if (arg & TUN_F_UFO)
>>>> +			feature_mask |= NETIF_F_UFO;
>>>>  	}
>>>>  
>>>>  	/* tun/tap driver inverts the usage for TSO offloads, where
>>>> @@ -965,7 +973,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
>>>>  	 * When user space turns off TSO, we turn off GSO/LRO so that
>>>>  	 * user-space will not receive TSO frames.
>>>>  	 */
>>>> -	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6))
>>>> +	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO))
>>>>  		features |= RX_OFFLOADS;
>>>>  	else
>>>>  		features &= ~RX_OFFLOADS;
>>>
>>> By the way this logic is completely broken, even without your patch,
>>> isn't it?
>>>
>>> It says: enable LRO+GRO if any of NETIF_F_TSO | NETIF_F_TSO6 |
>>> NETIF_F_UFO set.
>>>
>>> So what happens if I enable TSO only?
>>> LRO gets enabled so I can still get TSO6 packets.
>>>
>>>
>>> This really should be:
>>>
>>> 	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO) ==
>>> 			(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO)
>>>
>>>
>>> fixing this probably should be a separate patch before your
>>> series, and Cc stable.
>>
>> Actually, all this LRO/GRO feature manipulation on the macvtap device is
>> rather pointless as those features aren't really checked at any point
>> on the macvtap receive path.  GRO calls are done from napi context on
>> the lowest device, so by the time a packet hits macvtap, GRO/LRO has
>> already been done.
>>
>> So it doesn't really matter that we disable them incorrectly.  I
>> can send a separate patch to clean this up.
> 
> Hmm so if userspace says it can't handle TSO packets,
> it might get them anyway?

No.  It will get individual segments because in macvtap_handle_frame
we use user specified features to decide if segmentation needs
to be performed or not.

-vlad

> 
> 
>>>
>>>
>>>> @@ -1066,7 +1074,7 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
>>>>  	case TUNSETOFFLOAD:
>>>>  		/* let the user check for future flags */
>>>>  		if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
>>>> -			    TUN_F_TSO_ECN))
>>>> +			    TUN_F_TSO_ECN | TUN_F_UFO))
>>>>  			return -EINVAL;
>>>>  
>>>>  		rtnl_lock();
>>>> -- 
>>>> 1.9.3

Re: [PATCH 01/10] core: Split out UFO6 support

[Michael S. Tsirkin]

On Thu, Dec 18, 2014 at 10:01:35AM -0500, Vlad Yasevich wrote:
> On 12/18/2014 02:54 AM, Michael S. Tsirkin wrote:
> > Cc Dave, pls remember to do this next time otherwise
> > your patches won't get merged :)
> > 
> > On Wed, Dec 17, 2014 at 06:31:50PM -0500, Vlad Yasevich wrote:
> >> On 12/17/2014 05:45 PM, Michael S. Tsirkin wrote:
> >>> On Wed, Dec 17, 2014 at 01:20:46PM -0500, Vladislav Yasevich wrote:
> >>>> Split IPv6 support for UFO into its own feature similiar to TSO.
> >>>> This will later allow us to re-enable UFO support for virtio-net
> >>>> devices.
> >>>>
> >>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> >>>> ---
> >>>>  include/linux/netdev_features.h |  7 +++++--
> >>>>  include/linux/netdevice.h       |  1 +
> >>>>  include/linux/skbuff.h          |  1 +
> >>>>  net/core/dev.c                  | 35 +++++++++++++++++++----------------
> >>>>  net/core/ethtool.c              |  2 +-
> >>>>  5 files changed, 27 insertions(+), 19 deletions(-)
> >>>>
> >>>> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> >>>> index dcfdecb..a078945 100644
> >>>> --- a/include/linux/netdev_features.h
> >>>> +++ b/include/linux/netdev_features.h
> >>>> @@ -48,8 +48,9 @@ enum {
> >>>>  	NETIF_F_GSO_UDP_TUNNEL_BIT,	/* ... UDP TUNNEL with TSO */
> >>>>  	NETIF_F_GSO_UDP_TUNNEL_CSUM_BIT,/* ... UDP TUNNEL with TSO & CSUM */
> >>>>  	NETIF_F_GSO_MPLS_BIT,		/* ... MPLS segmentation */
> >>>> +	NETIF_F_UFO6_BIT,		/* ... UDPv6 fragmentation */
> >>>>  	/**/NETIF_F_GSO_LAST =		/* last bit, see GSO_MASK */
> >>>> -		NETIF_F_GSO_MPLS_BIT,
> >>>> +		NETIF_F_UFO6_BIT,
> >>>>  
> >>>>  	NETIF_F_FCOE_CRC_BIT,		/* FCoE CRC32 */
> >>>>  	NETIF_F_SCTP_CSUM_BIT,		/* SCTP checksum offload */
> >>>> @@ -109,6 +110,7 @@ enum {
> >>>>  #define NETIF_F_TSO_ECN		__NETIF_F(TSO_ECN)
> >>>>  #define NETIF_F_TSO		__NETIF_F(TSO)
> >>>>  #define NETIF_F_UFO		__NETIF_F(UFO)
> >>>> +#define NETIF_F_UFO6		__NETIF_F(UFO6)
> >>>>  #define NETIF_F_VLAN_CHALLENGED	__NETIF_F(VLAN_CHALLENGED)
> >>>>  #define NETIF_F_RXFCS		__NETIF_F(RXFCS)
> >>>>  #define NETIF_F_RXALL		__NETIF_F(RXALL)
> >>>> @@ -141,7 +143,7 @@ enum {
> >>>>  
> >>>>  /* List of features with software fallbacks. */
> >>>>  #define NETIF_F_GSO_SOFTWARE	(NETIF_F_TSO | NETIF_F_TSO_ECN | \
> >>>> -				 NETIF_F_TSO6 | NETIF_F_UFO)
> >>>> +				 NETIF_F_TSO6 | NETIF_F_UFO | NETIF_F_UFO6)
> >>>>  
> >>>>  #define NETIF_F_GEN_CSUM	NETIF_F_HW_CSUM
> >>>>  #define NETIF_F_V4_CSUM		(NETIF_F_GEN_CSUM | NETIF_F_IP_CSUM)
> >>>> @@ -149,6 +151,7 @@ enum {
> >>>>  #define NETIF_F_ALL_CSUM	(NETIF_F_V4_CSUM | NETIF_F_V6_CSUM)
> >>>>  
> >>>>  #define NETIF_F_ALL_TSO 	(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_TSO_ECN)
> >>>> +#define NETIF_F_ALL_UFO		(NETIF_F_UFO | NETIF_F_UFO6)
> >>>>  
> >>>>  #define NETIF_F_ALL_FCOE	(NETIF_F_FCOE_CRC | NETIF_F_FCOE_MTU | \
> >>>>  				 NETIF_F_FSO)
> >>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >>>> index 74fd5d3..86af10a 100644
> >>>> --- a/include/linux/netdevice.h
> >>>> +++ b/include/linux/netdevice.h
> >>>> @@ -3559,6 +3559,7 @@ static inline bool net_gso_ok(netdev_features_t features, int gso_type)
> >>>>  	/* check flags correspondence */
> >>>>  	BUILD_BUG_ON(SKB_GSO_TCPV4   != (NETIF_F_TSO >> NETIF_F_GSO_SHIFT));
> >>>>  	BUILD_BUG_ON(SKB_GSO_UDP     != (NETIF_F_UFO >> NETIF_F_GSO_SHIFT));
> >>>> +	BUILD_BUG_ON(SKB_GSO_UDP6    != (NETIF_F_UFO6 >> NETIF_F_GSO_SHIFT));
> >>>>  	BUILD_BUG_ON(SKB_GSO_DODGY   != (NETIF_F_GSO_ROBUST >> NETIF_F_GSO_SHIFT));
> >>>>  	BUILD_BUG_ON(SKB_GSO_TCP_ECN != (NETIF_F_TSO_ECN >> NETIF_F_GSO_SHIFT));
> >>>>  	BUILD_BUG_ON(SKB_GSO_TCPV6   != (NETIF_F_TSO6 >> NETIF_F_GSO_SHIFT));
> >>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> >>>> index 6c8b6f6..8538b67 100644
> >>>> --- a/include/linux/skbuff.h
> >>>> +++ b/include/linux/skbuff.h
> >>>> @@ -372,6 +372,7 @@ enum {
> >>>>  
> >>>>  	SKB_GSO_MPLS = 1 << 12,
> >>>>  
> >>>> +	SKB_GSO_UDP6 = 1 << 13
> >>>>  };
> >>>>  
> >>>>  #if BITS_PER_LONG > 32
> >>>
> >>> So this implies anything getting GSO packets e.g.
> >>> from userspace now needs to check IP version to
> >>> set GSO type correctly.
> >>>
> >>> I think you missed some places that do this, e.g. af_packet
> >>> sockets.
> >>>
> >>
> >> I looked at af_packet sockets and they set this only in the event
> >> vnet header has been used with a GSO type.  In this case, the user
> >> already knows the the type.
> > 
> > Imagine you are receiving a packet:
> > 
> >                 if (vnet_hdr.gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> >                         switch (vnet_hdr.gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> >                         case VIRTIO_NET_HDR_GSO_TCPV4:
> >                                 gso_type = SKB_GSO_TCPV4;
> >                                 break;
> >                         case VIRTIO_NET_HDR_GSO_TCPV6:
> >                                 gso_type = SKB_GSO_TCPV6;
> >                                 break;
> >                         case VIRTIO_NET_HDR_GSO_UDP:
> >                                 gso_type = SKB_GSO_UDP;
> >                                 break;
> >                         default:
> >                                 goto out_unlock;
> >                         }
> > 
> >                         if (vnet_hdr.gso_type & VIRTIO_NET_HDR_GSO_ECN)
> >                                 gso_type |= SKB_GSO_TCP_ECN;
> > 
> >                         if (vnet_hdr.gso_size == 0)
> >                                 goto out_unlock;
> > 
> >                 }
> > 
> > we used to report UFO6 as SKB_GSO_UDP, we probably
> > should keep doing this, with your patch we select the
> > goto out_unlock path.
> > 
> > 
> 
> No.  The vnet_hdr.gso_type is still going to be VIRTIO_NET_HDR_GSO_UDP
> since the UDP6 version isn't defined yet.  So, it will be marked as
> GSO_UDP.

I pasted the wrong snippet.
I meant this:

                        /* This is a hint as to how much should be * linear. */
                        vnet_hdr.hdr_len = __cpu_to_virtio16(false, skb_headlen(skb));
                        vnet_hdr.gso_size = __cpu_to_virtio16(false, sinfo->gso_size);
                        if (sinfo->gso_type & SKB_GSO_TCPV4)
                                vnet_hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
                        else if (sinfo->gso_type & SKB_GSO_TCPV6)
                                vnet_hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
                        else if (sinfo->gso_type & SKB_GSO_UDP)
                                vnet_hdr.gso_type = VIRTIO_NET_HDR_GSO_UDP;
                        else if (sinfo->gso_type & SKB_GSO_FCOE)
                                goto out_free;
                        else
                                BUG();

so if we get SKB_GSO_UDP we'll get BUG().



> This code most likely needs the same workaround as exists in tap and macvtap,
> i.e select the proxy fragment id and decide what to do with gso_type.

And fixup type to GSO_UDP6 while we are at it.

> > 
> >> It is true that with this series af_packets now can't do IPv6 UFO
> >> since there is no VIRTIO_NET_HDR_GSO_UDPV6 yet.
> > 
> > What do you mean by "do".
> 
> What I mean is that AF_PACKET sockets currently do not do IPv6 UFO
> correctly, even after Ben's fixes to tap/macvtap.  There is no
> proxy fragment id selection in af_packet case and we have the
> same problem Ben was trying address for tap/macvtap.
> > Are we talking about sending or receiving packets?
> 
> I am talking about sending, see above.
> 
> > You seem to conflate the two.
> > 
> > We always discarded ID on RX.
> > 
> > For tun, this is xmit, so just by saying "this device can
> > not do UFO" you will never get short packets.
> > 
> 
> You must mean long packets.

Yes.

> This is actually an issue I've been thinking
> about. With with your suggestion of switching the GSO type for legacy
> applications we end up with fragments for IPv6 traffic.   As a result,
> legacy VMs will see a regression for large IPv6 datagrams.

I'm not sure what's meant by my suggestion here :)
It seems clear that legacy applications don't want to get IPv6
fragment IDs in virtio header. Either we pass them plain ethernet
packets or assume they are ok with discarding the IDs even
if we set GSO_UDP.

> > 
> >>
> >> I suppose we could do something similar there as we do in tun code/macvtap code.
> >> If that's the case, it currently broken as well.
> >>
> >> -vlad
> > 
> > 
> > Broken is a big word.
> > 
> > Let's stop conflating two directions.
> 
> I am not and was talking only about af_packet as that is what you asked about.
> There is no tun/macvtap in play here.  They are handled separately in their
> respective drivers.
> 
> > 
> > Here's the way I look at it:
> > 
> > 1. Userspace doesn't have a way to get fragment IDs
> > from tun/macvtap/packet sockets.
> > Presumably, not all users need these IDs.
> > E.g. old guests clearly don't.
> > 
> > We should either give them packets stripping the ID,
> > like we always did, or make sure they never get these packets.
> > Second option seems achievable for tun if we
> > never tell linux it can do UFO, but I don't see
> > how to do it for macvtap and packet socket.
> > 
> 
> macvtap is slightly problematic, but doable with some tricks.
> packet socket is an interesting problem.  The only way
> an af_packet socket can receive an skb marked SKB_GSO_UDPV6
> is if someone else on the host sent it (like another guest).

Or if a NIC set it.

> Since there is are no feature or vnet header length negotiations,
> it is impossible to tell if an application using this af_packet
> socket is capable of processing VIRTIO_NET_HDR_GSO_UDPV6
> type (yet to be defined).
> 
> So, we can either use existing VIRTIO_NET_HDR_GSO_UDP on receive
> path, add some kind of negotiation logic to packet socket (like
> storing the application expected vnet header size), or perform
> IPv6 fragmentation somehow.
> 
> Options 1 or 2 are doable.

1 is using VIRTIO_NET_HDR_GSO_UDP and discarding ID,
2 is "some kind of negotiation logic"?
2 can't be enough, you will need 1 as well.

So let's start with 1 as a first step.




> > 
> > 2. Userspace doesn't have a way to set fragment IDs
> > for tun/macvtap/packet sockets.
> > Presumably, not all users have these IDs.  E.g. old
> > guests clearly don't.
> > 
> > We should either generate our own ID,
> > like we always did, or make sure we don't accept
> > these packets.
> > Second option is almost sure to break userspace,
> > so it seems we must do the first one.
> > 
> 
> Right.  This was missing from packet sockets.  I can fix it.
> 
> -vlad

Also, this can't be a patch on top, since we don't
want bisect to give us configurations which
can BUG().


> > 
> > 3.
> > Exactly the same two things if you replace userspace
> > with hypervisor and tun/macvtap/packet socket with
> > virtio device.
> > 
> > 
> > 4. 
> > As a next step, we should add a way for userspace
> > to tell us that it has ids and can handle them.
> > 
> > 
> > 
> > 
> > 
> >>>
> >>>> diff --git a/net/core/dev.c b/net/core/dev.c
> >>>> index 945bbd0..fa4d2ee 100644
> >>>> --- a/net/core/dev.c
> >>>> +++ b/net/core/dev.c
> >>>> @@ -5929,6 +5929,12 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
> >>>>  		features &= ~NETIF_F_ALL_TSO;
> >>>>  	}
> >>>>  
> >>>> +	/* UFO requires that SG is present as well */
> >>>> +	if ((features & NETIF_F_ALL_UFO) && !(features & NETIF_F_SG)) {
> >>>> +		netdev_dbg(dev, "Dropping UFO features since no SG feature.\n");
> >>>> +		features &= ~NETIF_F_ALL_UFO;
> >>>> +	}
> >>>> +
> >>>>  	if ((features & NETIF_F_TSO) && !(features & NETIF_F_HW_CSUM) &&
> >>>>  					!(features & NETIF_F_IP_CSUM)) {
> >>>>  		netdev_dbg(dev, "Dropping TSO features since no CSUM feature.\n");
> >>>> @@ -5952,24 +5958,21 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
> >>>>  		features &= ~NETIF_F_GSO;
> >>>>  	}
> >>>>  
> >>>> -	/* UFO needs SG and checksumming */
> >>>> -	if (features & NETIF_F_UFO) {
> >>>> -		/* maybe split UFO into V4 and V6? */
> >>>> -		if (!((features & NETIF_F_GEN_CSUM) ||
> >>>> -		    (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
> >>>> -			    == (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
> >>>> -			netdev_dbg(dev,
> >>>> -				"Dropping NETIF_F_UFO since no checksum offload features.\n");
> >>>> -			features &= ~NETIF_F_UFO;
> >>>> -		}
> >>>> -
> >>>> -		if (!(features & NETIF_F_SG)) {
> >>>> -			netdev_dbg(dev,
> >>>> -				"Dropping NETIF_F_UFO since no NETIF_F_SG feature.\n");
> >>>> -			features &= ~NETIF_F_UFO;
> >>>> -		}
> >>>> +	/* UFO also needs checksumming */
> >>>> +	if ((features & NETIF_F_UFO) && !(features & NETIF_F_GEN_CSUM) &&
> >>>> +					!(features & NETIF_F_IP_CSUM)) {
> >>>> +		netdev_dbg(dev,
> >>>> +			   "Dropping NETIF_F_UFO since no checksum offload features.\n");
> >>>> +		features &= ~NETIF_F_UFO;
> >>>> +	}
> >>>> +	if ((features & NETIF_F_UFO6) && !(features & NETIF_F_GEN_CSUM) &&
> >>>> +					 !(features & NETIF_F_IPV6_CSUM)) {
> >>>> +		netdev_dbg(dev,
> >>>> +			   "Dropping NETIF_F_UFO6 since no checksum offload features.\n");
> >>>> +		features &= ~NETIF_F_UFO6;
> >>>>  	}
> >>>>  
> >>>> +
> >>>>  #ifdef CONFIG_NET_RX_BUSY_POLL
> >>>>  	if (dev->netdev_ops->ndo_busy_poll)
> >>>>  		features |= NETIF_F_BUSY_POLL;
> >>>> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> >>>> index 06dfb29..93eff41 100644
> >>>> --- a/net/core/ethtool.c
> >>>> +++ b/net/core/ethtool.c
> >>>> @@ -223,7 +223,7 @@ static netdev_features_t ethtool_get_feature_mask(u32 eth_cmd)
> >>>>  		return NETIF_F_ALL_TSO;
> >>>>  	case ETHTOOL_GUFO:
> >>>>  	case ETHTOOL_SUFO:
> >>>> -		return NETIF_F_UFO;
> >>>> +		return NETIF_F_ALL_UFO;
> >>>>  	case ETHTOOL_GGSO:
> >>>>  	case ETHTOOL_SGSO:
> >>>>  		return NETIF_F_GSO;
> >>>> -- 
> >>>> 1.9.3

Re: [PATCH 01/10] core: Split out UFO6 support

[Michael S. Tsirkin]

On Thu, Dec 18, 2014 at 07:35:46PM +0200, Michael S. Tsirkin wrote:
> > > We should either generate our own ID,
> > > like we always did, or make sure we don't accept
> > > these packets.
> > > Second option is almost sure to break userspace,
> > > so it seems we must do the first one.
> > > 
> > 
> > Right.  This was missing from packet sockets.  I can fix it.
> > 
> > -vlad
> 
> Also, this can't be a patch on top, since we don't
> want bisect to give us configurations which
> can BUG().

So how doing this in stages:

1. add helper that checks skb GSO type.
If it is SKB_GSO_UDP, check for IPv6, and
generate the fragment ID.

Call this helper in
	- virtio net rx path
	- tun rx path (get user)
	- macvtap tx patch (get user)
	- packet socket tx patch (get user)

2. Put back UFO flag everywhere: virtio,tun,macvtap,packet,
since we can handle IPv6 now even if it's suboptimal.

Above two seem like smalling patches, appropriate for stable.

Next, work on a new feature to pass
fragment ID in virtio header:

3. split UFO/UFO6 as you did here, but add code
in above 4 places to convert UDP to UDP6,
additionally, add code in
	- virtio net tx path
	- tun tx path (get user)
	- macvtap rx patch (put user)
	- packet socket rx patch (put user)
to convert UDP6 to UDP.

	step 3 needs to be bisect-clean, somehow.

4. add new field in header, new feature bit for virtio net to gate it,
new ioctls to tun,macvtap,packet socket.

These two are more like optimizations, so not stable material.


-- 
MST

Re: [PATCH 08/10] tun: Re-uanble UFO support.

[Jason Wang]

On Thu, Dec 18, 2014 at 11:12 PM, Vlad Yasevich <vyasevic@redhat.com> 
wrote:
> On 12/18/2014 12:51 AM, Jason Wang wrote:
>>  
>>  
>>  ----- Original Message -----
>>>  Now that UFO is split into v4 and v6 parts, we can bring
>>>  back v4 support without any trouble.
>>> 
>>>  Continue to handle legacy applications by selecting the
>>>  IPv6 fragment id but do not change the gso type.  Thist
>>>  makes sure that two legacy VMs may still communicate.
>>> 
>>>  Based on original work from Ben Hutchings.
>>> 
>>>  Fixes: 88e0e0e5aa7a ("drivers/net: Disable UFO through virtio")
>>>  CC: Ben Hutchings <ben@decadent.org.uk>
>>>  Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>>>  ---
>>>   drivers/net/tun.c | 26 ++++++++++++++------------
>>>   1 file changed, 14 insertions(+), 12 deletions(-)
>>> 
>>>  diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>>>  index 9dd3746..8c32fca 100644
>>>  --- a/drivers/net/tun.c
>>>  +++ b/drivers/net/tun.c
>>>  @@ -175,7 +175,7 @@ struct tun_struct {
>>>   	struct net_device	*dev;
>>>   	netdev_features_t	set_features;
>>>   #define TUN_USER_FEATURES 
>>> (NETIF_F_HW_CSUM|NETIF_F_TSO_ECN|NETIF_F_TSO| \
>>>  -			  NETIF_F_TSO6)
>>>  +			  NETIF_F_TSO6|NETIF_F_UFO)
>>>   
>>>   	int			vnet_hdr_sz;
>>>   	int			sndbuf;
>>>  @@ -1152,20 +1152,15 @@ static ssize_t tun_get_user(struct 
>>> tun_struct *tun,
>>>  struct tun_file *tfile,
>>>   			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
>>>   			break;
>>>   		case VIRTIO_NET_HDR_GSO_UDP:
>>>  -		{
>>>  -			static bool warned;
>>>  -
>>>  -			if (!warned) {
>>>  -				warned = true;
>>>  -				netdev_warn(tun->dev,
>>>  -					    "%s: using disabled UFO feature; please fix this 
>>> program\n",
>>>  -					    current->comm);
>>>  -			}
>>>   			skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
>>>  -			if (skb->protocol == htons(ETH_P_IPV6))
>>>  +			if (vlan_get_protocol(skb) == htons(ETH_P_IPV6)) {
>>  
>>  This probably means UDPv6 with vlan does not work well, looks like 
>> another
>>  independent fixe and also for stable.
> 
> Ok.  I can split this out.
> 
>>>  +				/* This allows legacy application to work.
>>>  +				 * Do not change the gso_type as it may
>>>  +				 * not be upderstood by legacy applications.
>>>  +				 */
>>  
>>  Isn't this an issue that we use SKB_GSO_UDP for a UDPv6 packet? 
>> Especially
>>  consider we want to fix UFOv6 in the future? We probably can use
>>  SKB_GSO_UDP6 here and try to fix it in tun_put_user() if a legacy 
>> userspace
>>  is detected.
> 
> There is a slight problem with this.  This will force fragmentation 
> of IPv6
> traffic between VMs since UFO6 wouldn't be enabled on a destination 
> device.
> So, suddenly older VMs will see a performance regression for large 
> UDPv6 traffic.
> 
> 
> With this code, a legacy VM will continue to receive large UDPv6 
> traffic.
> New VMs will have an updated virtio-net which will reset the gso type 
> on input.

I get the point and that's why you still want ipv6 offload helpers to
accept SKB_GSO_UDP. So SKB_GSO_UDP was still ambigious since it can be a
UDP6 packet in fact.

I'm not sure, but probably we could just leave the regression of legacy
guest as is and fix current kernel. The sooner we eliminate the 
ambigious
the better for future development.
> 
> 
>>>   				ipv6_proxy_select_ident(skb);
>>  
>>  Question still for vlan, is network header correctly set here? 
>> Looks like
>>  ipv6_proxy_select_ident() depends on correct network header to work.
>>>  +	
> 
> Yes, you are right...  I wonder how that worked.  I'll re-test.
> 
> Thanks
> -vlad
> 		}
>>>   			break;
>>>  -		}
>>>   		default:
>>>   			tun->dev->stats.rx_frame_errors++;
>>>   			kfree_skb(skb);
>>>  @@ -1273,6 +1268,8 @@ static ssize_t tun_put_user(struct 
>>> tun_struct *tun,
>>>   				gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
>>>   			else if (sinfo->gso_type & SKB_GSO_TCPV6)
>>>   				gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
>>>  +			else if (sinfo->gso_type & SKB_GSO_UDP)
>>>  +				gso.gso_type = VIRTIO_NET_HDR_GSO_UDP;
>>>   			else {
>>>   				pr_err("unexpected GSO type: "
>>>   				       "0x%x, gso_size %d, hdr_len %d\n",
>>>  @@ -1780,6 +1777,11 @@ static int set_offload(struct tun_struct 
>>> *tun,
>>>  unsigned long arg)
>>>   				features |= NETIF_F_TSO6;
>>>   			arg &= ~(TUN_F_TSO4|TUN_F_TSO6);
>>>   		}
>>>  +
>>>  +		if (arg & TUN_F_UFO) {
>>>  +			features |= NETIF_F_UFO;
>>>  +			arg &= ~TUN_F_UFO;
>>>  +		}
>>>   	}
>>>   
>>>   	/* This gives the user a way to test for new features in future 
>>> by
>>>  --
>>>  1.9.3
>>> 
>>>  --
>>>  To unsubscribe from this list: send the line "unsubscribe netdev" 
>>> in
>>>  the body of a message to majordomo@vger.kernel.org
>>>  More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> 
> 

Re: [PATCH 01/10] core: Split out UFO6 support

[Vlad Yasevich]

On 12/18/2014 12:35 PM, Michael S. Tsirkin wrote:
> On Thu, Dec 18, 2014 at 10:01:35AM -0500, Vlad Yasevich wrote:
>> On 12/18/2014 02:54 AM, Michael S. Tsirkin wrote:
>>> Cc Dave, pls remember to do this next time otherwise
>>> your patches won't get merged :)
>>>
>>> On Wed, Dec 17, 2014 at 06:31:50PM -0500, Vlad Yasevich wrote:
>>>> On 12/17/2014 05:45 PM, Michael S. Tsirkin wrote:
>>>>> On Wed, Dec 17, 2014 at 01:20:46PM -0500, Vladislav Yasevich wrote:
>>>>>> Split IPv6 support for UFO into its own feature similiar to TSO.
>>>>>> This will later allow us to re-enable UFO support for virtio-net
>>>>>> devices.
>>>>>>
>>>>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>>>>>> ---
>>>>>>  include/linux/netdev_features.h |  7 +++++--
>>>>>>  include/linux/netdevice.h       |  1 +
>>>>>>  include/linux/skbuff.h          |  1 +
>>>>>>  net/core/dev.c                  | 35 +++++++++++++++++++----------------
>>>>>>  net/core/ethtool.c              |  2 +-
>>>>>>  5 files changed, 27 insertions(+), 19 deletions(-)
>>>>>>
>>>>>> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
>>>>>> index dcfdecb..a078945 100644
>>>>>> --- a/include/linux/netdev_features.h
>>>>>> +++ b/include/linux/netdev_features.h
>>>>>> @@ -48,8 +48,9 @@ enum {
>>>>>>  	NETIF_F_GSO_UDP_TUNNEL_BIT,	/* ... UDP TUNNEL with TSO */
>>>>>>  	NETIF_F_GSO_UDP_TUNNEL_CSUM_BIT,/* ... UDP TUNNEL with TSO & CSUM */
>>>>>>  	NETIF_F_GSO_MPLS_BIT,		/* ... MPLS segmentation */
>>>>>> +	NETIF_F_UFO6_BIT,		/* ... UDPv6 fragmentation */
>>>>>>  	/**/NETIF_F_GSO_LAST =		/* last bit, see GSO_MASK */
>>>>>> -		NETIF_F_GSO_MPLS_BIT,
>>>>>> +		NETIF_F_UFO6_BIT,
>>>>>>  
>>>>>>  	NETIF_F_FCOE_CRC_BIT,		/* FCoE CRC32 */
>>>>>>  	NETIF_F_SCTP_CSUM_BIT,		/* SCTP checksum offload */
>>>>>> @@ -109,6 +110,7 @@ enum {
>>>>>>  #define NETIF_F_TSO_ECN		__NETIF_F(TSO_ECN)
>>>>>>  #define NETIF_F_TSO		__NETIF_F(TSO)
>>>>>>  #define NETIF_F_UFO		__NETIF_F(UFO)
>>>>>> +#define NETIF_F_UFO6		__NETIF_F(UFO6)
>>>>>>  #define NETIF_F_VLAN_CHALLENGED	__NETIF_F(VLAN_CHALLENGED)
>>>>>>  #define NETIF_F_RXFCS		__NETIF_F(RXFCS)
>>>>>>  #define NETIF_F_RXALL		__NETIF_F(RXALL)
>>>>>> @@ -141,7 +143,7 @@ enum {
>>>>>>  
>>>>>>  /* List of features with software fallbacks. */
>>>>>>  #define NETIF_F_GSO_SOFTWARE	(NETIF_F_TSO | NETIF_F_TSO_ECN | \
>>>>>> -				 NETIF_F_TSO6 | NETIF_F_UFO)
>>>>>> +				 NETIF_F_TSO6 | NETIF_F_UFO | NETIF_F_UFO6)
>>>>>>  
>>>>>>  #define NETIF_F_GEN_CSUM	NETIF_F_HW_CSUM
>>>>>>  #define NETIF_F_V4_CSUM		(NETIF_F_GEN_CSUM | NETIF_F_IP_CSUM)
>>>>>> @@ -149,6 +151,7 @@ enum {
>>>>>>  #define NETIF_F_ALL_CSUM	(NETIF_F_V4_CSUM | NETIF_F_V6_CSUM)
>>>>>>  
>>>>>>  #define NETIF_F_ALL_TSO 	(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_TSO_ECN)
>>>>>> +#define NETIF_F_ALL_UFO		(NETIF_F_UFO | NETIF_F_UFO6)
>>>>>>  
>>>>>>  #define NETIF_F_ALL_FCOE	(NETIF_F_FCOE_CRC | NETIF_F_FCOE_MTU | \
>>>>>>  				 NETIF_F_FSO)
>>>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>>>>> index 74fd5d3..86af10a 100644
>>>>>> --- a/include/linux/netdevice.h
>>>>>> +++ b/include/linux/netdevice.h
>>>>>> @@ -3559,6 +3559,7 @@ static inline bool net_gso_ok(netdev_features_t features, int gso_type)
>>>>>>  	/* check flags correspondence */
>>>>>>  	BUILD_BUG_ON(SKB_GSO_TCPV4   != (NETIF_F_TSO >> NETIF_F_GSO_SHIFT));
>>>>>>  	BUILD_BUG_ON(SKB_GSO_UDP     != (NETIF_F_UFO >> NETIF_F_GSO_SHIFT));
>>>>>> +	BUILD_BUG_ON(SKB_GSO_UDP6    != (NETIF_F_UFO6 >> NETIF_F_GSO_SHIFT));
>>>>>>  	BUILD_BUG_ON(SKB_GSO_DODGY   != (NETIF_F_GSO_ROBUST >> NETIF_F_GSO_SHIFT));
>>>>>>  	BUILD_BUG_ON(SKB_GSO_TCP_ECN != (NETIF_F_TSO_ECN >> NETIF_F_GSO_SHIFT));
>>>>>>  	BUILD_BUG_ON(SKB_GSO_TCPV6   != (NETIF_F_TSO6 >> NETIF_F_GSO_SHIFT));
>>>>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>>>>> index 6c8b6f6..8538b67 100644
>>>>>> --- a/include/linux/skbuff.h
>>>>>> +++ b/include/linux/skbuff.h
>>>>>> @@ -372,6 +372,7 @@ enum {
>>>>>>  
>>>>>>  	SKB_GSO_MPLS = 1 << 12,
>>>>>>  
>>>>>> +	SKB_GSO_UDP6 = 1 << 13
>>>>>>  };
>>>>>>  
>>>>>>  #if BITS_PER_LONG > 32
>>>>>
>>>>> So this implies anything getting GSO packets e.g.
>>>>> from userspace now needs to check IP version to
>>>>> set GSO type correctly.
>>>>>
>>>>> I think you missed some places that do this, e.g. af_packet
>>>>> sockets.
>>>>>
>>>>
>>>> I looked at af_packet sockets and they set this only in the event
>>>> vnet header has been used with a GSO type.  In this case, the user
>>>> already knows the the type.
>>>
>>> Imagine you are receiving a packet:
>>>
>>>                 if (vnet_hdr.gso_type != VIRTIO_NET_HDR_GSO_NONE) {
>>>                         switch (vnet_hdr.gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
>>>                         case VIRTIO_NET_HDR_GSO_TCPV4:
>>>                                 gso_type = SKB_GSO_TCPV4;
>>>                                 break;
>>>                         case VIRTIO_NET_HDR_GSO_TCPV6:
>>>                                 gso_type = SKB_GSO_TCPV6;
>>>                                 break;
>>>                         case VIRTIO_NET_HDR_GSO_UDP:
>>>                                 gso_type = SKB_GSO_UDP;
>>>                                 break;
>>>                         default:
>>>                                 goto out_unlock;
>>>                         }
>>>
>>>                         if (vnet_hdr.gso_type & VIRTIO_NET_HDR_GSO_ECN)
>>>                                 gso_type |= SKB_GSO_TCP_ECN;
>>>
>>>                         if (vnet_hdr.gso_size == 0)
>>>                                 goto out_unlock;
>>>
>>>                 }
>>>
>>> we used to report UFO6 as SKB_GSO_UDP, we probably
>>> should keep doing this, with your patch we select the
>>> goto out_unlock path.
>>>
>>>
>>
>> No.  The vnet_hdr.gso_type is still going to be VIRTIO_NET_HDR_GSO_UDP
>> since the UDP6 version isn't defined yet.  So, it will be marked as
>> GSO_UDP.
> 
> I pasted the wrong snippet.
> I meant this:
> 
>                         /* This is a hint as to how much should be * linear. */
>                         vnet_hdr.hdr_len = __cpu_to_virtio16(false, skb_headlen(skb));
>                         vnet_hdr.gso_size = __cpu_to_virtio16(false, sinfo->gso_size);
>                         if (sinfo->gso_type & SKB_GSO_TCPV4)
>                                 vnet_hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
>                         else if (sinfo->gso_type & SKB_GSO_TCPV6)
>                                 vnet_hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
>                         else if (sinfo->gso_type & SKB_GSO_UDP)
>                                 vnet_hdr.gso_type = VIRTIO_NET_HDR_GSO_UDP;
>                         else if (sinfo->gso_type & SKB_GSO_FCOE)
>                                 goto out_free;
>                         else
>                                 BUG();
> 
> so if we get SKB_GSO_UDP we'll get BUG().
> 
> 
> 
>> This code most likely needs the same workaround as exists in tap and macvtap,
>> i.e select the proxy fragment id and decide what to do with gso_type.
> 
> And fixup type to GSO_UDP6 while we are at it.
> 
>>>
>>>> It is true that with this series af_packets now can't do IPv6 UFO
>>>> since there is no VIRTIO_NET_HDR_GSO_UDPV6 yet.
>>>
>>> What do you mean by "do".
>>
>> What I mean is that AF_PACKET sockets currently do not do IPv6 UFO
>> correctly, even after Ben's fixes to tap/macvtap.  There is no
>> proxy fragment id selection in af_packet case and we have the
>> same problem Ben was trying address for tap/macvtap.
>>> Are we talking about sending or receiving packets?
>>
>> I am talking about sending, see above.
>>
>>> You seem to conflate the two.
>>>
>>> We always discarded ID on RX.
>>>
>>> For tun, this is xmit, so just by saying "this device can
>>> not do UFO" you will never get short packets.
>>>
>>
>> You must mean long packets.
> 
> Yes.
> 
>> This is actually an issue I've been thinking
>> about. With with your suggestion of switching the GSO type for legacy
>> applications we end up with fragments for IPv6 traffic.   As a result,
>> legacy VMs will see a regression for large IPv6 datagrams.
> 
> I'm not sure what's meant by my suggestion here :)

What I am referring to here is to change the gso_type to UDPV6 in tap/macvtap
when passing packet to the host.

> It seems clear that legacy applications don't want to get IPv6
> fragment IDs in virtio header. Either we pass them plain ethernet
> packets or assume they are ok with discarding the IDs even
> if we set GSO_UDP.

So, I am not try to pass fragment IDs yet.  I am trying to make sure that
older gueest that assume that UFO == UFO4 + UFO6 continue to work and do not
regress.  At the same time, I want to enable UFO(4) for new guests.

If we set gso_type to SKB_GSO_UDPV6 before passing the data to the forwarding
path of the host, then this will cause IPv6 fragmentation.  If we leave gso_type
as SKB_GSO_UDP, then older VMs will continue to communicate using large UDP
data packets.

Later when VMs support UFO6, when UFO6 is enabled, the fragment id can be communicated.
But that's later.

> 
>>>
>>>>
>>>> I suppose we could do something similar there as we do in tun code/macvtap code.
>>>> If that's the case, it currently broken as well.
>>>>
>>>> -vlad
>>>
>>>
>>> Broken is a big word.
>>>
>>> Let's stop conflating two directions.
>>
>> I am not and was talking only about af_packet as that is what you asked about.
>> There is no tun/macvtap in play here.  They are handled separately in their
>> respective drivers.
>>
>>>
>>> Here's the way I look at it:
>>>
>>> 1. Userspace doesn't have a way to get fragment IDs
>>> from tun/macvtap/packet sockets.
>>> Presumably, not all users need these IDs.
>>> E.g. old guests clearly don't.
>>>
>>> We should either give them packets stripping the ID,
>>> like we always did, or make sure they never get these packets.
>>> Second option seems achievable for tun if we
>>> never tell linux it can do UFO, but I don't see
>>> how to do it for macvtap and packet socket.
>>>
>>
>> macvtap is slightly problematic, but doable with some tricks.
>> packet socket is an interesting problem.  The only way
>> an af_packet socket can receive an skb marked SKB_GSO_UDPV6
>> is if someone else on the host sent it (like another guest).
> 
> Or if a NIC set it.

OK, virtio seems to be the only nic doing this... You are right, I
need to cover that scenario.

-vlad

> 
>> Since there is are no feature or vnet header length negotiations,
>> it is impossible to tell if an application using this af_packet
>> socket is capable of processing VIRTIO_NET_HDR_GSO_UDPV6
>> type (yet to be defined).
>>
>> So, we can either use existing VIRTIO_NET_HDR_GSO_UDP on receive
>> path, add some kind of negotiation logic to packet socket (like
>> storing the application expected vnet header size), or perform
>> IPv6 fragmentation somehow.
>>
>> Options 1 or 2 are doable.
> 
> 1 is using VIRTIO_NET_HDR_GSO_UDP and discarding ID,
> 2 is "some kind of negotiation logic"?
> 2 can't be enough, you will need 1 as well.
> 
> So let's start with 1 as a first step.
> 
> 
> 
> 
>>>
>>> 2. Userspace doesn't have a way to set fragment IDs
>>> for tun/macvtap/packet sockets.
>>> Presumably, not all users have these IDs.  E.g. old
>>> guests clearly don't.
>>>
>>> We should either generate our own ID,
>>> like we always did, or make sure we don't accept
>>> these packets.
>>> Second option is almost sure to break userspace,
>>> so it seems we must do the first one.
>>>
>>
>> Right.  This was missing from packet sockets.  I can fix it.
>>
>> -vlad
> 
> Also, this can't be a patch on top, since we don't
> want bisect to give us configurations which
> can BUG().
> 
> 
>>>
>>> 3.
>>> Exactly the same two things if you replace userspace
>>> with hypervisor and tun/macvtap/packet socket with
>>> virtio device.
>>>
>>>
>>> 4. 
>>> As a next step, we should add a way for userspace
>>> to tell us that it has ids and can handle them.
>>>
>>>
>>>
>>>
>>>
>>>>>
>>>>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>>>>> index 945bbd0..fa4d2ee 100644
>>>>>> --- a/net/core/dev.c
>>>>>> +++ b/net/core/dev.c
>>>>>> @@ -5929,6 +5929,12 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
>>>>>>  		features &= ~NETIF_F_ALL_TSO;
>>>>>>  	}
>>>>>>  
>>>>>> +	/* UFO requires that SG is present as well */
>>>>>> +	if ((features & NETIF_F_ALL_UFO) && !(features & NETIF_F_SG)) {
>>>>>> +		netdev_dbg(dev, "Dropping UFO features since no SG feature.\n");
>>>>>> +		features &= ~NETIF_F_ALL_UFO;
>>>>>> +	}
>>>>>> +
>>>>>>  	if ((features & NETIF_F_TSO) && !(features & NETIF_F_HW_CSUM) &&
>>>>>>  					!(features & NETIF_F_IP_CSUM)) {
>>>>>>  		netdev_dbg(dev, "Dropping TSO features since no CSUM feature.\n");
>>>>>> @@ -5952,24 +5958,21 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
>>>>>>  		features &= ~NETIF_F_GSO;
>>>>>>  	}
>>>>>>  
>>>>>> -	/* UFO needs SG and checksumming */
>>>>>> -	if (features & NETIF_F_UFO) {
>>>>>> -		/* maybe split UFO into V4 and V6? */
>>>>>> -		if (!((features & NETIF_F_GEN_CSUM) ||
>>>>>> -		    (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
>>>>>> -			    == (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
>>>>>> -			netdev_dbg(dev,
>>>>>> -				"Dropping NETIF_F_UFO since no checksum offload features.\n");
>>>>>> -			features &= ~NETIF_F_UFO;
>>>>>> -		}
>>>>>> -
>>>>>> -		if (!(features & NETIF_F_SG)) {
>>>>>> -			netdev_dbg(dev,
>>>>>> -				"Dropping NETIF_F_UFO since no NETIF_F_SG feature.\n");
>>>>>> -			features &= ~NETIF_F_UFO;
>>>>>> -		}
>>>>>> +	/* UFO also needs checksumming */
>>>>>> +	if ((features & NETIF_F_UFO) && !(features & NETIF_F_GEN_CSUM) &&
>>>>>> +					!(features & NETIF_F_IP_CSUM)) {
>>>>>> +		netdev_dbg(dev,
>>>>>> +			   "Dropping NETIF_F_UFO since no checksum offload features.\n");
>>>>>> +		features &= ~NETIF_F_UFO;
>>>>>> +	}
>>>>>> +	if ((features & NETIF_F_UFO6) && !(features & NETIF_F_GEN_CSUM) &&
>>>>>> +					 !(features & NETIF_F_IPV6_CSUM)) {
>>>>>> +		netdev_dbg(dev,
>>>>>> +			   "Dropping NETIF_F_UFO6 since no checksum offload features.\n");
>>>>>> +		features &= ~NETIF_F_UFO6;
>>>>>>  	}
>>>>>>  
>>>>>> +
>>>>>>  #ifdef CONFIG_NET_RX_BUSY_POLL
>>>>>>  	if (dev->netdev_ops->ndo_busy_poll)
>>>>>>  		features |= NETIF_F_BUSY_POLL;
>>>>>> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
>>>>>> index 06dfb29..93eff41 100644
>>>>>> --- a/net/core/ethtool.c
>>>>>> +++ b/net/core/ethtool.c
>>>>>> @@ -223,7 +223,7 @@ static netdev_features_t ethtool_get_feature_mask(u32 eth_cmd)
>>>>>>  		return NETIF_F_ALL_TSO;
>>>>>>  	case ETHTOOL_GUFO:
>>>>>>  	case ETHTOOL_SUFO:
>>>>>> -		return NETIF_F_UFO;
>>>>>> +		return NETIF_F_ALL_UFO;
>>>>>>  	case ETHTOOL_GGSO:
>>>>>>  	case ETHTOOL_SGSO:
>>>>>>  		return NETIF_F_GSO;
>>>>>> -- 
>>>>>> 1.9.3

Re: [PATCH 01/10] core: Split out UFO6 support

[Vlad Yasevich]

On 12/18/2014 12:50 PM, Michael S. Tsirkin wrote:
> On Thu, Dec 18, 2014 at 07:35:46PM +0200, Michael S. Tsirkin wrote:
>>>> We should either generate our own ID,
>>>> like we always did, or make sure we don't accept
>>>> these packets.
>>>> Second option is almost sure to break userspace,
>>>> so it seems we must do the first one.
>>>>
>>>
>>> Right.  This was missing from packet sockets.  I can fix it.
>>>
>>> -vlad
>>
>> Also, this can't be a patch on top, since we don't
>> want bisect to give us configurations which
>> can BUG().
> 
> So how doing this in stages:
> 
> 1. add helper that checks skb GSO type.
> If it is SKB_GSO_UDP, check for IPv6, and
> generate the fragment ID.
> 
> Call this helper in
> 	- virtio net rx path

Why do we need id on rx path?  Fragment ID should only be generated on tx.

> 	- tun rx path (get user)
> 	- macvtap tx patch (get user)
> 	- packet socket tx patch (get user)

The reset makes sense.
> 
> 2. Put back UFO flag everywhere: virtio,tun,macvtap,packet,
> since we can handle IPv6 now even if it's suboptimal.
> 
> Above two seem like smalling patches, appropriate for stable.

OK.

> 
> Next, work on a new feature to pass
> fragment ID in virtio header:
> 
> 3. split UFO/UFO6 as you did here, but add code
> in above 4 places to convert UDP to UDP6,

Doing so will adversely impact IPv6 UFO performance for legacy
guests.  I know how many times I've seen mail wrt patch xyz caused
%X  regression in my setup and how we've reverted or tried to fixed
things to solve this.  If we go with approach, the only "fix' would be
to upgrade the guest and that's not available to some users.

-vlad

> additionally, add code in
> 	- virtio net tx path
> 	- tun tx path (get user)
> 	- macvtap rx patch (put user)
> 	- packet socket rx patch (put user)
> to convert UDP6 to UDP.
>
> 	step 3 needs to be bisect-clean, somehow.
> 
> 4. add new field in header, new feature bit for virtio net to gate it,
> new ioctls to tun,macvtap,packet socket.
> 
> These two are more like optimizations, so not stable material.
> 
> 

Re: [PATCH 01/10] core: Split out UFO6 support

[Michael S. Tsirkin]

On Fri, Dec 19, 2014 at 03:13:20PM -0500, Vlad Yasevich wrote:
> On 12/18/2014 12:50 PM, Michael S. Tsirkin wrote:
> > On Thu, Dec 18, 2014 at 07:35:46PM +0200, Michael S. Tsirkin wrote:
> >>>> We should either generate our own ID,
> >>>> like we always did, or make sure we don't accept
> >>>> these packets.
> >>>> Second option is almost sure to break userspace,
> >>>> so it seems we must do the first one.
> >>>>
> >>>
> >>> Right.  This was missing from packet sockets.  I can fix it.
> >>>
> >>> -vlad
> >>
> >> Also, this can't be a patch on top, since we don't
> >> want bisect to give us configurations which
> >> can BUG().
> > 
> > So how doing this in stages:
> > 
> > 1. add helper that checks skb GSO type.
> > If it is SKB_GSO_UDP, check for IPv6, and
> > generate the fragment ID.
> > 
> > Call this helper in
> > 	- virtio net rx path
> 
> Why do we need id on rx path?  Fragment ID should only be generated on tx.

So that all GSO_UDP6 packets have fragment ID as appropriate.
It's similar to how we fill it on rx in tun, is it not?


> > 	- tun rx path (get user)
> > 	- macvtap tx patch (get user)
> > 	- packet socket tx patch (get user)
> 
> The reset makes sense.
> > 
> > 2. Put back UFO flag everywhere: virtio,tun,macvtap,packet,
> > since we can handle IPv6 now even if it's suboptimal.
> > 
> > Above two seem like smalling patches, appropriate for stable.
> 
> OK.
> 
> > 
> > Next, work on a new feature to pass
> > fragment ID in virtio header:
> > 
> > 3. split UFO/UFO6 as you did here, but add code
> > in above 4 places to convert UDP to UDP6,
> 
> Doing so will adversely impact IPv6 UFO performance for legacy
> guests.  I know how many times I've seen mail wrt patch xyz caused
> %X  regression in my setup and how we've reverted or tried to fixed
> things to solve this.  If we go with approach, the only "fix' would be
> to upgrade the guest and that's not available to some users.
> 
> -vlad

I think there's some misunderstanding here.

I merely mean that after split, host should always have
SKB_GSO_UDP6 set for IPv6.

To make sure legacy userspace/guests don't notice changes,
whenever we detect SKB_GSO_UDP6 we should set VIRTIO_NET_HDR_GSO_UDP,
and whenever we get VIRTIO_NET_HDR_GSO_UDP we should set either
SKB_GSO_UDP6 or SKB_GSO_UDP depending on IP type.

Given this clarification there's no reason to think
old guests will regress, correct?

> > additionally, add code in
> > 	- virtio net tx path
> > 	- tun tx path (get user)
> > 	- macvtap rx patch (put user)
> > 	- packet socket rx patch (put user)
> > to convert UDP6 to UDP.
> >
> > 	step 3 needs to be bisect-clean, somehow.
> > 
> > 4. add new field in header, new feature bit for virtio net to gate it,
> > new ioctls to tun,macvtap,packet socket.
> > 
> > These two are more like optimizations, so not stable material.
> > 
> > 

Re: [PATCH 01/10] core: Split out UFO6 support

[Vlad Yasevich]

On 12/20/2014 04:03 PM, Michael S. Tsirkin wrote:
> On Fri, Dec 19, 2014 at 03:13:20PM -0500, Vlad Yasevich wrote:
>> On 12/18/2014 12:50 PM, Michael S. Tsirkin wrote:
>>> On Thu, Dec 18, 2014 at 07:35:46PM +0200, Michael S. Tsirkin wrote:
>>>>>> We should either generate our own ID,
>>>>>> like we always did, or make sure we don't accept
>>>>>> these packets.
>>>>>> Second option is almost sure to break userspace,
>>>>>> so it seems we must do the first one.
>>>>>>
>>>>>
>>>>> Right.  This was missing from packet sockets.  I can fix it.
>>>>>
>>>>> -vlad
>>>>
>>>> Also, this can't be a patch on top, since we don't
>>>> want bisect to give us configurations which
>>>> can BUG().
>>>
>>> So how doing this in stages:
>>>
>>> 1. add helper that checks skb GSO type.
>>> If it is SKB_GSO_UDP, check for IPv6, and
>>> generate the fragment ID.
>>>
>>> Call this helper in
>>> 	- virtio net rx path
>>
>> Why do we need id on rx path?  Fragment ID should only be generated on tx.
> 
> So that all GSO_UDP6 packets have fragment ID as appropriate.
> It's similar to how we fill it on rx in tun, is it not?

Saying "rx in tun" always hurts my head.  We fill it in in tun_get_user()
which then passed the packet to the kernel for forwarding.  The is later
used in the forwarding process.

I've been thinking about putting this fragment generation into the GSO
path so there is only 1 spot that ever needs to this.   This way it
would only be done if the fragment id is actually needed.  For most
guest-to-guest comms, the id isn't needed.

> 
> 
>>> 	- tun rx path (get user)
>>> 	- macvtap tx patch (get user)
>>> 	- packet socket tx patch (get user)
>>
>> The reset makes sense.
>>>
>>> 2. Put back UFO flag everywhere: virtio,tun,macvtap,packet,
>>> since we can handle IPv6 now even if it's suboptimal.
>>>
>>> Above two seem like smalling patches, appropriate for stable.
>>
>> OK.
>>
>>>
>>> Next, work on a new feature to pass
>>> fragment ID in virtio header:
>>>
>>> 3. split UFO/UFO6 as you did here, but add code
>>> in above 4 places to convert UDP to UDP6,
>>
>> Doing so will adversely impact IPv6 UFO performance for legacy
>> guests.  I know how many times I've seen mail wrt patch xyz caused
>> %X  regression in my setup and how we've reverted or tried to fixed
>> things to solve this.  If we go with approach, the only "fix' would be
>> to upgrade the guest and that's not available to some users.
>>
>> -vlad
> 
> I think there's some misunderstanding here.
> 
> I merely mean that after split, host should always have
> SKB_GSO_UDP6 set for IPv6.
> 
> To make sure legacy userspace/guests don't notice changes,
> whenever we detect SKB_GSO_UDP6 we should set VIRTIO_NET_HDR_GSO_UDP,
> and whenever we get VIRTIO_NET_HDR_GSO_UDP we should set either
> SKB_GSO_UDP6 or SKB_GSO_UDP depending on IP type.

This is the part that introduced the regression.  By setting the gso_type
to UDP6, we trigger skb_gso_segment() to actually perform IPv6 fragmentation.

I've seen this when passing UDP traffic from 2 fedora19 systems over the
kernel that does the above.

-vlad

> 
> Given this clarification there's no reason to think
> old guests will regress, correct?
> 
>>> additionally, add code in
>>> 	- virtio net tx path
>>> 	- tun tx path (get user)
>>> 	- macvtap rx patch (put user)
>>> 	- packet socket rx patch (put user)
>>> to convert UDP6 to UDP.
>>>
>>> 	step 3 needs to be bisect-clean, somehow.
>>>
>>> 4. add new field in header, new feature bit for virtio net to gate it,
>>> new ioctls to tun,macvtap,packet socket.
>>>
>>> These two are more like optimizations, so not stable material.
>>>
>>>

Re: [PATCH 00/10] Split UFO into v4 and v6 versions.

[Ben Hutchings]

[-- Attachment #1.1: Type: text/plain, Size: 1359 bytes --]

On Thu, 2014-12-18 at 00:28 -0500, Jason Wang wrote:
> 
> ----- Original Message -----
> > UFO support in the kernel applies to both IPv4 and IPv6 protocols
> > with the same device feature.  However some devices may not be able
> > to support one of the offloads.  For this we split the UFO offload
> > feature into 2 pieces.  NETIF_F_UFO now controlls the IPv4 part and
> > this series introduces NETIF_F_UFO6.
> > 
> > As a result of this work, we can now re-enable NETIF_F_UFO on
> > virtio_net devices and restore UDP over IPv4 performance for guests.
> > We also continue to support legacy guests that assume that UFO6
> > support included into UFO(4).
> > 
> > Without this work, migrating a guest to a 3.18 kernel fails.
> > 
> 
> This series eliminate the ambiguous NETIF_F_UFO.
> 
> But VIRTIO_NET_F_{HOST|GUEST}_UFO and VIRTIO_NET_F_HDR_GSO_UDP is still
> ambigious. I know it was used to keep compatibility for legacy guest. But
> what's the future plan? Differentiate UFOv4 and UFOv6 in virtio features and
> gso type in vnet header looks sufficient?
[...]

The IPv6 fragmentation ID needs to be added to the vnet header, to do
UFOv6 properly.  If it wasn't for that lack, we wouldn't have to split
the feature flag.

Ben.

-- 
Ben Hutchings
If more than one person is responsible for a bug, no one is at fault.

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 00/10] Split UFO into v4 and v6 versions.

[Michael S. Tsirkin]

On Wed, Dec 24, 2014 at 07:11:20PM +0100, Ben Hutchings wrote:
> On Thu, 2014-12-18 at 00:28 -0500, Jason Wang wrote:
> > 
> > ----- Original Message -----
> > > UFO support in the kernel applies to both IPv4 and IPv6 protocols
> > > with the same device feature.  However some devices may not be able
> > > to support one of the offloads.  For this we split the UFO offload
> > > feature into 2 pieces.  NETIF_F_UFO now controlls the IPv4 part and
> > > this series introduces NETIF_F_UFO6.
> > > 
> > > As a result of this work, we can now re-enable NETIF_F_UFO on
> > > virtio_net devices and restore UDP over IPv4 performance for guests.
> > > We also continue to support legacy guests that assume that UFO6
> > > support included into UFO(4).
> > > 
> > > Without this work, migrating a guest to a 3.18 kernel fails.
> > > 
> > 
> > This series eliminate the ambiguous NETIF_F_UFO.
> > 
> > But VIRTIO_NET_F_{HOST|GUEST}_UFO and VIRTIO_NET_F_HDR_GSO_UDP is still
> > ambigious. I know it was used to keep compatibility for legacy guest. But
> > what's the future plan? Differentiate UFOv4 and UFOv6 in virtio features and
> > gso type in vnet header looks sufficient?
> [...]
> 
> The IPv6 fragmentation ID needs to be added to the vnet header, to do
> UFOv6 properly.  If it wasn't for that lack, we wouldn't have to split
> the feature flag.
> 
> Ben.


Right.

I think a good plan is as follows:

1. add code generating IDs on rx path for virtio,
   and re-enable UFO (4+6).

2. Ben's patch doing similar things for tun/macvtap

3. similarly for packet sockets

above seem appropriate for stable

4. 4+6 split, to make it easier for drivers
   to identify when fragment ID is needed

5. extend vnet header, and add ways to enable it,
   when enabled, use that to pass
   fragment ID for tun/virtio/vhost/macvtap/packet socket

4 is what this patchset does.


> -- 
> Ben Hutchings
> If more than one person is responsible for a bug, no one is at fault.

Re: [PATCH 00/10] Split UFO into v4 and v6 versions.

[Jason Wang]

On Thu, Dec 25, 2014 at 2:59 AM, Michael S. Tsirkin <mst@redhat.com> 
wrote:
> On Wed, Dec 24, 2014 at 07:11:20PM +0100, Ben Hutchings wrote:
>>  On Thu, 2014-12-18 at 00:28 -0500, Jason Wang wrote:
>>  > 
>>  > ----- Original Message -----
>>  > > UFO support in the kernel applies to both IPv4 and IPv6 
>> protocols
>>  > > with the same device feature.  However some devices may not be 
>> able
>>  > > to support one of the offloads.  For this we split the UFO 
>> offload
>>  > > feature into 2 pieces.  NETIF_F_UFO now controlls the IPv4 part 
>> and
>>  > > this series introduces NETIF_F_UFO6.
>>  > > 
>>  > > As a result of this work, we can now re-enable NETIF_F_UFO on
>>  > > virtio_net devices and restore UDP over IPv4 performance for 
>> guests.
>>  > > We also continue to support legacy guests that assume that UFO6
>>  > > support included into UFO(4).
>>  > > 
>>  > > Without this work, migrating a guest to a 3.18 kernel fails.
>>  > > 
>>  > 
>>  > This series eliminate the ambiguous NETIF_F_UFO.
>>  > 
>>  > But VIRTIO_NET_F_{HOST|GUEST}_UFO and VIRTIO_NET_F_HDR_GSO_UDP is 
>> still
>>  > ambigious. I know it was used to keep compatibility for legacy 
>> guest. But
>>  > what's the future plan? Differentiate UFOv4 and UFOv6 in virtio 
>> features and
>>  > gso type in vnet header looks sufficient?
>>  [...]
>>  
>>  The IPv6 fragmentation ID needs to be added to the vnet header, to 
>> do
>>  UFOv6 properly.  If it wasn't for that lack, we wouldn't have to 
>> split
>>  the feature flag.
>>  
>>  Ben.
> 
> 
> Right.
> 
> I think a good plan is as follows:
> 
> 1. add code generating IDs on rx path for virtio,
>    and re-enable UFO (4+6).
> 
> 2. Ben's patch doing similar things for tun/macvtap

I wonder whether or not we can do this lazily.

E.g during UFO6 software segment for dodgy packets?

And this can eliminate the effort of duplicating it in all untrusted 
sources?
> 
> 3. similarly for packet sockets
> 
> 
> above seem appropriate for stable
> 
> 4. 4+6 split, to make it easier for drivers
>    to identify when fragment ID is needed
> 
> 5. extend vnet header, and add ways to enable it,
>    when enabled, use that to pass
>    fragment ID for tun/virtio/vhost/macvtap/packet socket
> 
> 4 is what this patchset does.
> 
> 
>>  -- 
>>  Ben Hutchings
>>  If more than one person is responsible for a bug, no one is at 
>> fault.
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/10] Split UFO into v4 and v6 versions.

[Michael S. Tsirkin]

On Thu, Dec 25, 2014 at 03:10:01AM +0008, Jason Wang wrote:
> 
> 
> On Thu, Dec 25, 2014 at 2:59 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >On Wed, Dec 24, 2014 at 07:11:20PM +0100, Ben Hutchings wrote:
> >> On Thu, 2014-12-18 at 00:28 -0500, Jason Wang wrote:
> >> >  > ----- Original Message -----
> >> > > UFO support in the kernel applies to both IPv4 and IPv6 protocols
> >> > > with the same device feature.  However some devices may not be able
> >> > > to support one of the offloads.  For this we split the UFO offload
> >> > > feature into 2 pieces.  NETIF_F_UFO now controlls the IPv4 part and
> >> > > this series introduces NETIF_F_UFO6.
> >> > >  > > As a result of this work, we can now re-enable NETIF_F_UFO on
> >> > > virtio_net devices and restore UDP over IPv4 performance for
> >>guests.
> >> > > We also continue to support legacy guests that assume that UFO6
> >> > > support included into UFO(4).
> >> > >  > > Without this work, migrating a guest to a 3.18 kernel fails.
> >> > >  >  > This series eliminate the ambiguous NETIF_F_UFO.
> >> >  > But VIRTIO_NET_F_{HOST|GUEST}_UFO and VIRTIO_NET_F_HDR_GSO_UDP is
> >>still
> >> > ambigious. I know it was used to keep compatibility for legacy guest.
> >>But
> >> > what's the future plan? Differentiate UFOv4 and UFOv6 in virtio
> >>features and
> >> > gso type in vnet header looks sufficient?
> >> [...]
> >>   The IPv6 fragmentation ID needs to be added to the vnet header, to do
> >> UFOv6 properly.  If it wasn't for that lack, we wouldn't have to split
> >> the feature flag.
> >> Ben.
> >
> >
> >Right.
> >
> >I think a good plan is as follows:
> >
> >1. add code generating IDs on rx path for virtio,
> >   and re-enable UFO (4+6).
> >
> >2. Ben's patch doing similar things for tun/macvtap
> 
> I wonder whether or not we can do this lazily.
> 
> E.g during UFO6 software segment for dodgy packets?
> 
> And this can eliminate the effort of duplicating it in all untrusted
> sources?

But then we'll need to then change it again in step 5,
probably by setting some flag?
Might as well do it directly.

> >
> >3. similarly for packet sockets
> >
> >
> >above seem appropriate for stable
> >
> >4. 4+6 split, to make it easier for drivers
> >   to identify when fragment ID is needed
> >
> >5. extend vnet header, and add ways to enable it,
> >   when enabled, use that to pass
> >   fragment ID for tun/virtio/vhost/macvtap/packet socket
> >
> >4 is what this patchset does.
> >
> >
> >> --  Ben Hutchings
> >> If more than one person is responsible for a bug, no one is at fault.
> >
> >
> >--
> >To unsubscribe from this list: send the line "unsubscribe netdev" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/10] Split UFO into v4 and v6 versions.

[Jason Wang]

On Thu, Dec 25, 2014 at 3:14 PM, Michael S. Tsirkin <mst@redhat.com> 
wrote:
> On Thu, Dec 25, 2014 at 03:10:01AM +0008, Jason Wang wrote:
>>  
>>  
>>  On Thu, Dec 25, 2014 at 2:59 AM, Michael S. Tsirkin 
>> <mst@redhat.com> wrote:
>>  >On Wed, Dec 24, 2014 at 07:11:20PM +0100, Ben Hutchings wrote:
>>  >> On Thu, 2014-12-18 at 00:28 -0500, Jason Wang wrote:
>>  >> >  > ----- Original Message -----
>>  >> > > UFO support in the kernel applies to both IPv4 and IPv6 
>> protocols
>>  >> > > with the same device feature.  However some devices may not 
>> be able
>>  >> > > to support one of the offloads.  For this we split the UFO 
>> offload
>>  >> > > feature into 2 pieces.  NETIF_F_UFO now controlls the IPv4 
>> part and
>>  >> > > this series introduces NETIF_F_UFO6.
>>  >> > >  > > As a result of this work, we can now re-enable 
>> NETIF_F_UFO on
>>  >> > > virtio_net devices and restore UDP over IPv4 performance for
>>  >>guests.
>>  >> > > We also continue to support legacy guests that assume that 
>> UFO6
>>  >> > > support included into UFO(4).
>>  >> > >  > > Without this work, migrating a guest to a 3.18 kernel 
>> fails.
>>  >> > >  >  > This series eliminate the ambiguous NETIF_F_UFO.
>>  >> >  > But VIRTIO_NET_F_{HOST|GUEST}_UFO and 
>> VIRTIO_NET_F_HDR_GSO_UDP is
>>  >>still
>>  >> > ambigious. I know it was used to keep compatibility for legacy 
>> guest.
>>  >>But
>>  >> > what's the future plan? Differentiate UFOv4 and UFOv6 in virtio
>>  >>features and
>>  >> > gso type in vnet header looks sufficient?
>>  >> [...]
>>  >>   The IPv6 fragmentation ID needs to be added to the vnet 
>> header, to do
>>  >> UFOv6 properly.  If it wasn't for that lack, we wouldn't have to 
>> split
>>  >> the feature flag.
>>  >> Ben.
>>  >
>>  >
>>  >Right.
>>  >
>>  >I think a good plan is as follows:
>>  >
>>  >1. add code generating IDs on rx path for virtio,
>>  >   and re-enable UFO (4+6).
>>  >
>>  >2. Ben's patch doing similar things for tun/macvtap
>>  
>>  I wonder whether or not we can do this lazily.
>>  
>>  E.g during UFO6 software segment for dodgy packets?
>>  
>>  And this can eliminate the effort of duplicating it in all untrusted
>>  sources?
> 
> But then we'll need to then change it again in step 5,
> probably by setting some flag?

The idea is if no segmentation (either software or hardware), no need 
for an id.
 
So looks like dodgy is sufficient since we are sure kernel should set 
id in all other cases. And we can add the id only when we see it was 
not set for the ufo packet(legacy guest). This seems not conflict with 
step 5.
> 
> Might as well do it directly.
> 
>>  >
>>  >3. similarly for packet sockets
>>  >
>>  >
>>  >above seem appropriate for stable
>>  >
>>  >4. 4+6 split, to make it easier for drivers
>>  >   to identify when fragment ID is needed
>>  >
>>  >5. extend vnet header, and add ways to enable it,
>>  >   when enabled, use that to pass
>>  >   fragment ID for tun/virtio/vhost/macvtap/packet socket
>>  >
>>  >4 is what this patchset does.
>>  >
>>  >
>>  >> --  Ben Hutchings
>>  >> If more than one person is responsible for a bug, no one is at 
>> fault.
>>  >
>>  >
>>  >--
>>  >To unsubscribe from this list: send the line "unsubscribe netdev" 
>> in
>>  >the body of a message to majordomo@vger.kernel.org
>>  >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html