[PATCH v3 net-next 0/8] Local Checksum Offload

[PATCH v3 net-next 0/8] Local Checksum Offload

[Edward Cree]

Tested with a VXLAN tunnel over a device that doesn't support inner checksum
 offload (so the checksum will have been done in sw by validate_xmit_skb()).

Changes from v2:
 * Added support for IPv4 GRE.
 * Split out 'always set up for checksum offload' into its own patch.
 * Removed csum_help from iptunnel_handle_offloads.
 * Rewrote LCO callers to only fold once.
 * Simplified nocheck handling.

Changes from v1:
 * Enabled support in more encapsulation protocols.
   I think it now covers everything except GRE.
 * Wrote up some documentation covering TX checksum offload, LCO and RCO.

Edward Cree (8):
  net: local checksum offload for encapsulation
  net: udp: always set up for CHECKSUM_PARTIAL offload
  net: enable LCO for udp_tunnel_handle_offloads() users
  net: vxlan: enable local checksum offload
  fou: enable LCO in FOU and GUE
  net: gre: Implement LCO for GRE over IPv4
  net: ip_tunnel: remove 'csum_help' argument to
    iptunnel_handle_offloads
  Documentation/networking: add checksum-offloads.txt to explain LCO

 Documentation/networking/00-INDEX              |   2 +
 Documentation/networking/checksum-offloads.txt | 119 +++++++++++++++++++++++++
 drivers/net/vxlan.c                            |  18 ++--
 include/linux/skbuff.h                         |  26 ++++++
 include/net/ip_tunnels.h                       |   3 +-
 include/net/udp_tunnel.h                       |   2 +-
 net/ipv4/fou.c                                 |  14 ++-
 net/ipv4/ip_gre.c                              |  20 ++++-
 net/ipv4/ip_tunnel_core.c                      |  15 +---
 net/ipv4/ipip.c                                |   2 +-
 net/ipv4/udp.c                                 |  28 ++----
 net/ipv6/ip6_checksum.c                        |  23 ++---
 net/ipv6/sit.c                                 |   4 +-
 net/netfilter/ipvs/ip_vs_xmit.c                |   6 +-
 14 files changed, 197 insertions(+), 85 deletions(-)
 create mode 100644 Documentation/networking/checksum-offloads.txt

[PATCH net-next 1/8] net: local checksum offload for encapsulation

[Edward Cree]

The arithmetic properties of the ones-complement checksum mean that a
 correctly checksummed inner packet, including its checksum, has a ones
 complement sum depending only on whatever value was used to initialise
 the checksum field before checksumming (in the case of TCP and UDP,
 this is the ones complement sum of the pseudo header, complemented).
Consequently, if we are going to offload the inner checksum with
 CHECKSUM_PARTIAL, we can compute the outer checksum based only on the
 packed data not covered by the inner checksum, and the initial value of
 the inner checksum field.

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 include/linux/skbuff.h  | 24 ++++++++++++++++++++++++
 net/ipv4/udp.c          | 20 ++++++++++----------
 net/ipv6/ip6_checksum.c | 14 +++++++-------
 3 files changed, 41 insertions(+), 17 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 6b6bd42..ee063ff 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3665,5 +3665,29 @@ static inline unsigned int skb_gso_network_seglen(const struct sk_buff *skb)
 	return hdr_len + skb_gso_transport_seglen(skb);
 }
 
+/* Local Checksum Offload.
+ * Compute outer checksum based on the assumption that the
+ * inner checksum will be offloaded later.
+ * Fill in outer checksum adjustment (e.g. with sum of outer
+ * pseudo-header) before calling.
+ * Also ensure that inner checksum is in linear data area.
+ */
+static inline __wsum lco_csum(struct sk_buff *skb)
+{
+	char *inner_csum_field;
+	__wsum csum;
+
+	/* Start with complement of inner checksum adjustment */
+	inner_csum_field = skb->data + skb_checksum_start_offset(skb) +
+				skb->csum_offset;
+	csum = ~csum_unfold(*(__force __sum16 *)inner_csum_field);
+	/* Add in checksum of our headers (incl. outer checksum
+	 * adjustment filled in by caller)
+	 */
+	csum = skb_checksum(skb, 0, skb_checksum_start_offset(skb), csum);
+	/* The result is the checksum from skb->data to end of packet */
+	return csum;
+}
+
 #endif	/* __KERNEL__ */
 #endif	/* _LINUX_SKBUFF_H */
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 8841e98..f9bd5db 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -767,16 +767,18 @@ void udp_set_csum(bool nocheck, struct sk_buff *skb,
 {
 	struct udphdr *uh = udp_hdr(skb);
 
-	if (nocheck)
+	if (nocheck) {
 		uh->check = 0;
-	else if (skb_is_gso(skb))
+	} else if (skb_is_gso(skb)) {
 		uh->check = ~udp_v4_check(len, saddr, daddr, 0);
-	else if (skb_dst(skb) && skb_dst(skb)->dev &&
-		 (skb_dst(skb)->dev->features &
-		  (NETIF_F_IP_CSUM | NETIF_F_HW_CSUM))) {
-
-		BUG_ON(skb->ip_summed == CHECKSUM_PARTIAL);
-
+	} else if (skb->ip_summed == CHECKSUM_PARTIAL) {
+		uh->check = 0;
+		uh->check = ~udp_v4_check(len, saddr, daddr, lco_csum(skb));
+		if (uh->check == 0)
+			uh->check = CSUM_MANGLED_0;
+	} else if (skb_dst(skb) && skb_dst(skb)->dev &&
+		   (skb_dst(skb)->dev->features &
+		    (NETIF_F_IP_CSUM | NETIF_F_HW_CSUM))) {
 		skb->ip_summed = CHECKSUM_PARTIAL;
 		skb->csum_start = skb_transport_header(skb) - skb->head;
 		skb->csum_offset = offsetof(struct udphdr, check);
@@ -784,8 +786,6 @@ void udp_set_csum(bool nocheck, struct sk_buff *skb,
 	} else {
 		__wsum csum;
 
-		BUG_ON(skb->ip_summed == CHECKSUM_PARTIAL);
-
 		uh->check = 0;
 		csum = skb_checksum(skb, 0, len, 0);
 		uh->check = udp_v4_check(len, saddr, daddr, csum);
diff --git a/net/ipv6/ip6_checksum.c b/net/ipv6/ip6_checksum.c
index 9a4d732..eba525d 100644
--- a/net/ipv6/ip6_checksum.c
+++ b/net/ipv6/ip6_checksum.c
@@ -98,11 +98,13 @@ void udp6_set_csum(bool nocheck, struct sk_buff *skb,
 		uh->check = 0;
 	else if (skb_is_gso(skb))
 		uh->check = ~udp_v6_check(len, saddr, daddr, 0);
-	else if (skb_dst(skb) && skb_dst(skb)->dev &&
-		 (skb_dst(skb)->dev->features & NETIF_F_IPV6_CSUM)) {
-
-		BUG_ON(skb->ip_summed == CHECKSUM_PARTIAL);
-
+	else if (skb->ip_summed == CHECKSUM_PARTIAL) {
+		uh->check = 0;
+		uh->check = ~udp_v6_check(len, saddr, daddr, csum_fold(lco_csum(skb));
+		if (uh->check == 0)
+			uh->check = CSUM_MANGLED_0;
+	} else if (skb_dst(skb) && skb_dst(skb)->dev &&
+		   (skb_dst(skb)->dev->features & NETIF_F_IPV6_CSUM) {
 		skb->ip_summed = CHECKSUM_PARTIAL;
 		skb->csum_start = skb_transport_header(skb) - skb->head;
 		skb->csum_offset = offsetof(struct udphdr, check);
@@ -110,8 +112,6 @@ void udp6_set_csum(bool nocheck, struct sk_buff *skb,
 	} else {
 		__wsum csum;
 
-		BUG_ON(skb->ip_summed == CHECKSUM_PARTIAL);
-
 		uh->check = 0;
 		csum = skb_checksum(skb, 0, len, 0);
 		uh->check = udp_v6_check(len, saddr, daddr, csum);

[PATCH net-next 2/8] net: udp: always set up for CHECKSUM_PARTIAL offload

[Edward Cree]

If the dst device doesn't support it, it'll get fixed up later anyway
 by validate_xmit_skb().  Also, this allows us to take advantage of LCO
 to avoid summing the payload multiple times.

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 net/ipv4/udp.c          | 14 +-------------
 net/ipv6/ip6_checksum.c | 13 +------------
 2 files changed, 2 insertions(+), 25 deletions(-)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index f9bd5db..916d6ea 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -776,23 +776,11 @@ void udp_set_csum(bool nocheck, struct sk_buff *skb,
 		uh->check = ~udp_v4_check(len, saddr, daddr, lco_csum(skb));
 		if (uh->check == 0)
 			uh->check = CSUM_MANGLED_0;
-	} else if (skb_dst(skb) && skb_dst(skb)->dev &&
-		   (skb_dst(skb)->dev->features &
-		    (NETIF_F_IP_CSUM | NETIF_F_HW_CSUM))) {
+	} else {
 		skb->ip_summed = CHECKSUM_PARTIAL;
 		skb->csum_start = skb_transport_header(skb) - skb->head;
 		skb->csum_offset = offsetof(struct udphdr, check);
 		uh->check = ~udp_v4_check(len, saddr, daddr, 0);
-	} else {
-		__wsum csum;
-
-		uh->check = 0;
-		csum = skb_checksum(skb, 0, len, 0);
-		uh->check = udp_v4_check(len, saddr, daddr, csum);
-		if (uh->check == 0)
-			uh->check = CSUM_MANGLED_0;
-
-		skb->ip_summed = CHECKSUM_UNNECESSARY;
 	}
 }
 EXPORT_SYMBOL(udp_set_csum);
diff --git a/net/ipv6/ip6_checksum.c b/net/ipv6/ip6_checksum.c
index eba525d..199c191 100644
--- a/net/ipv6/ip6_checksum.c
+++ b/net/ipv6/ip6_checksum.c
@@ -103,22 +103,11 @@ void udp6_set_csum(bool nocheck, struct sk_buff *skb,
 		uh->check = ~udp_v6_check(len, saddr, daddr, csum_fold(lco_csum(skb));
 		if (uh->check == 0)
 			uh->check = CSUM_MANGLED_0;
-	} else if (skb_dst(skb) && skb_dst(skb)->dev &&
-		   (skb_dst(skb)->dev->features & NETIF_F_IPV6_CSUM) {
+	} else {
 		skb->ip_summed = CHECKSUM_PARTIAL;
 		skb->csum_start = skb_transport_header(skb) - skb->head;
 		skb->csum_offset = offsetof(struct udphdr, check);
 		uh->check = ~udp_v6_check(len, saddr, daddr, 0);
-	} else {
-		__wsum csum;
-
-		uh->check = 0;
-		csum = skb_checksum(skb, 0, len, 0);
-		uh->check = udp_v6_check(len, saddr, daddr, csum);
-		if (uh->check == 0)
-			uh->check = CSUM_MANGLED_0;
-
-		skb->ip_summed = CHECKSUM_UNNECESSARY;
 	}
 }
 EXPORT_SYMBOL(udp6_set_csum);

[PATCH net-next 3/8] net: enable LCO for udp_tunnel_handle_offloads() users

[Edward Cree]

The only protocol affected at present is Geneve.

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 include/net/udp_tunnel.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h
index cb2f89f..210eb90 100644
--- a/include/net/udp_tunnel.h
+++ b/include/net/udp_tunnel.h
@@ -103,7 +103,8 @@ static inline struct sk_buff *udp_tunnel_handle_offloads(struct sk_buff *skb,
 {
 	int type = udp_csum ? SKB_GSO_UDP_TUNNEL_CSUM : SKB_GSO_UDP_TUNNEL;
 
-	return iptunnel_handle_offloads(skb, udp_csum, type);
+	/* As we're a UDP tunnel, we support LCO, so don't need csum_help */
+	return iptunnel_handle_offloads(skb, false, type);
 }
 
 static inline void udp_tunnel_gro_complete(struct sk_buff *skb, int nhoff)

[PATCH net-next 4/8] net: vxlan: enable local checksum offload

[Edward Cree]

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 drivers/net/vxlan.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 6369a57..d3509a1 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1708,10 +1708,8 @@ static int vxlan6_xmit_skb(struct dst_entry *dst, struct sock *sk,
 		if (csum_start <= VXLAN_MAX_REMCSUM_START &&
 		    !(csum_start & VXLAN_RCO_SHIFT_MASK) &&
 		    (skb->csum_offset == offsetof(struct udphdr, check) ||
-		     skb->csum_offset == offsetof(struct tcphdr, check))) {
-			udp_sum = false;
+		     skb->csum_offset == offsetof(struct tcphdr, check)))
 			type |= SKB_GSO_TUNNEL_REMCSUM;
-		}
 	}
 
 	skb_scrub_packet(skb, xnet);
@@ -1733,7 +1731,7 @@ static int vxlan6_xmit_skb(struct dst_entry *dst, struct sock *sk,
 		goto err;
 	}
 
-	skb = iptunnel_handle_offloads(skb, udp_sum, type);
+	skb = iptunnel_handle_offloads(skb, false, type);
 	if (IS_ERR(skb)) {
 		err = -EINVAL;
 		goto err;
@@ -1765,8 +1763,7 @@ static int vxlan6_xmit_skb(struct dst_entry *dst, struct sock *sk,
 	skb_set_inner_protocol(skb, htons(ETH_P_TEB));
 
 	udp_tunnel6_xmit_skb(dst, sk, skb, dev, saddr, daddr, prio,
-			     ttl, src_port, dst_port,
-			     !!(vxflags & VXLAN_F_UDP_ZERO_CSUM6_TX));
+			     ttl, src_port, dst_port, !udp_sum);
 	return 0;
 err:
 	dst_release(dst);
@@ -1793,10 +1790,8 @@ static int vxlan_xmit_skb(struct rtable *rt, struct sock *sk, struct sk_buff *sk
 		if (csum_start <= VXLAN_MAX_REMCSUM_START &&
 		    !(csum_start & VXLAN_RCO_SHIFT_MASK) &&
 		    (skb->csum_offset == offsetof(struct udphdr, check) ||
-		     skb->csum_offset == offsetof(struct tcphdr, check))) {
-			udp_sum = false;
+		     skb->csum_offset == offsetof(struct tcphdr, check)))
 			type |= SKB_GSO_TUNNEL_REMCSUM;
-		}
 	}
 
 	min_headroom = LL_RESERVED_SPACE(rt->dst.dev) + rt->dst.header_len
@@ -1814,7 +1809,7 @@ static int vxlan_xmit_skb(struct rtable *rt, struct sock *sk, struct sk_buff *sk
 	if (WARN_ON(!skb))
 		return -ENOMEM;
 
-	skb = iptunnel_handle_offloads(skb, udp_sum, type);
+	skb = iptunnel_handle_offloads(skb, false, type);
 	if (IS_ERR(skb))
 		return PTR_ERR(skb);
 
@@ -1844,8 +1839,7 @@ static int vxlan_xmit_skb(struct rtable *rt, struct sock *sk, struct sk_buff *sk
 	skb_set_inner_protocol(skb, htons(ETH_P_TEB));
 
 	return udp_tunnel_xmit_skb(rt, sk, skb, src, dst, tos,
-				   ttl, df, src_port, dst_port, xnet,
-				   !(vxflags & VXLAN_F_UDP_CSUM));
+				   ttl, df, src_port, dst_port, xnet, !udp_sum);
 }
 
 /* Bypass encapsulation if the destination is local */

[PATCH net-next 5/8] fou: enable LCO in FOU and GUE

[Edward Cree]

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 net/ipv4/fou.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/net/ipv4/fou.c b/net/ipv4/fou.c
index e0fcbbb..3d4c9a7 100644
--- a/net/ipv4/fou.c
+++ b/net/ipv4/fou.c
@@ -773,7 +773,6 @@ static void fou_build_udp(struct sk_buff *skb, struct ip_tunnel_encap *e,
 	uh->dest = e->dport;
 	uh->source = sport;
 	uh->len = htons(skb->len);
-	uh->check = 0;
 	udp_set_csum(!(e->flags & TUNNEL_ENCAP_FLAG_CSUM), skb,
 		     fl4->saddr, fl4->daddr, skb->len);
 
@@ -783,11 +782,11 @@ static void fou_build_udp(struct sk_buff *skb, struct ip_tunnel_encap *e,
 int fou_build_header(struct sk_buff *skb, struct ip_tunnel_encap *e,
 		     u8 *protocol, struct flowi4 *fl4)
 {
-	bool csum = !!(e->flags & TUNNEL_ENCAP_FLAG_CSUM);
-	int type = csum ? SKB_GSO_UDP_TUNNEL_CSUM : SKB_GSO_UDP_TUNNEL;
+	int type = e->flags & TUNNEL_ENCAP_FLAG_CSUM ? SKB_GSO_UDP_TUNNEL_CSUM :
+						       SKB_GSO_UDP_TUNNEL;
 	__be16 sport;
 
-	skb = iptunnel_handle_offloads(skb, csum, type);
+	skb = iptunnel_handle_offloads(skb, false, type);
 
 	if (IS_ERR(skb))
 		return PTR_ERR(skb);
@@ -803,8 +802,8 @@ EXPORT_SYMBOL(fou_build_header);
 int gue_build_header(struct sk_buff *skb, struct ip_tunnel_encap *e,
 		     u8 *protocol, struct flowi4 *fl4)
 {
-	bool csum = !!(e->flags & TUNNEL_ENCAP_FLAG_CSUM);
-	int type = csum ? SKB_GSO_UDP_TUNNEL_CSUM : SKB_GSO_UDP_TUNNEL;
+	int type = e->flags & TUNNEL_ENCAP_FLAG_CSUM ? SKB_GSO_UDP_TUNNEL_CSUM :
+						       SKB_GSO_UDP_TUNNEL;
 	struct guehdr *guehdr;
 	size_t hdrlen, optlen = 0;
 	__be16 sport;
@@ -813,7 +812,6 @@ int gue_build_header(struct sk_buff *skb, struct ip_tunnel_encap *e,
 
 	if ((e->flags & TUNNEL_ENCAP_FLAG_REMCSUM) &&
 	    skb->ip_summed == CHECKSUM_PARTIAL) {
-		csum = false;
 		optlen += GUE_PLEN_REMCSUM;
 		type |= SKB_GSO_TUNNEL_REMCSUM;
 		need_priv = true;
@@ -821,7 +819,7 @@ int gue_build_header(struct sk_buff *skb, struct ip_tunnel_encap *e,
 
 	optlen += need_priv ? GUE_LEN_PRIV : 0;
 
-	skb = iptunnel_handle_offloads(skb, csum, type);
+	skb = iptunnel_handle_offloads(skb, false, type);
 
 	if (IS_ERR(skb))
 		return PTR_ERR(skb);

[PATCH net-next 6/8] net: gre: Implement LCO for GRE over IPv4

[Edward Cree]

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 net/ipv4/ip_gre.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 04a48c0..8a589d3 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -440,6 +440,20 @@ drop:
 	return 0;
 }
 
+static __sum16 gre_checksum(struct sk_buff *skb)
+{
+	__sum16 csum;
+
+	if (skb->ip_summed == CHECKSUM_PARTIAL) {
+		csum = csum_fold(lco_csum(skb));
+		if (csum == 0)
+			csum = CSUM_MANGLED_0;
+		return csum;
+	} else {
+		return csum_fold(skb_checksum(skb, 0, skb->len, 0));
+	}
+}
+
 static void build_header(struct sk_buff *skb, int hdr_len, __be16 flags,
 			 __be16 proto, __be32 key, __be32 seq)
 {
@@ -467,8 +481,7 @@ static void build_header(struct sk_buff *skb, int hdr_len, __be16 flags,
 		    !(skb_shinfo(skb)->gso_type &
 		      (SKB_GSO_GRE | SKB_GSO_GRE_CSUM))) {
 			*ptr = 0;
-			*(__sum16 *)ptr = csum_fold(skb_checksum(skb, 0,
-								 skb->len, 0));
+			*(__sum16 *)ptr = gre_checksum(skb);
 		}
 	}
 }
@@ -493,7 +506,7 @@ static void __gre_xmit(struct sk_buff *skb, struct net_device *dev,
 static struct sk_buff *gre_handle_offloads(struct sk_buff *skb,
 					   bool csum)
 {
-	return iptunnel_handle_offloads(skb, csum,
+	return iptunnel_handle_offloads(skb, false,
 					csum ? SKB_GSO_GRE_CSUM : SKB_GSO_GRE);
 }
 

[PATCH net-next 7/8] net: ip_tunnel: remove 'csum_help' argument to iptunnel_handle_offloads

[Edward Cree]

All users now pass false, so we can remove it, and remove the code that
 was conditional upon it.

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 drivers/net/vxlan.c             |  4 ++--
 include/net/ip_tunnels.h        |  3 +--
 include/net/udp_tunnel.h        |  3 +--
 net/ipv4/fou.c                  |  4 ++--
 net/ipv4/ip_gre.c               |  3 +--
 net/ipv4/ip_tunnel_core.c       | 15 +--------------
 net/ipv4/ipip.c                 |  2 +-
 net/ipv6/sit.c                  |  4 ++--
 net/netfilter/ipvs/ip_vs_xmit.c |  6 ++----
 9 files changed, 13 insertions(+), 31 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index d3509a1..32ff1ef 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1731,7 +1731,7 @@ static int vxlan6_xmit_skb(struct dst_entry *dst, struct sock *sk,
 		goto err;
 	}
 
-	skb = iptunnel_handle_offloads(skb, false, type);
+	skb = iptunnel_handle_offloads(skb, type);
 	if (IS_ERR(skb)) {
 		err = -EINVAL;
 		goto err;
@@ -1809,7 +1809,7 @@ static int vxlan_xmit_skb(struct rtable *rt, struct sock *sk, struct sk_buff *sk
 	if (WARN_ON(!skb))
 		return -ENOMEM;
 
-	skb = iptunnel_handle_offloads(skb, false, type);
+	skb = iptunnel_handle_offloads(skb, type);
 	if (IS_ERR(skb))
 		return PTR_ERR(skb);
 
diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index 62a750a..2c10119 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -279,8 +279,7 @@ int iptunnel_xmit(struct sock *sk, struct rtable *rt, struct sk_buff *skb,
 struct metadata_dst *iptunnel_metadata_reply(struct metadata_dst *md,
 					     gfp_t flags);
 
-struct sk_buff *iptunnel_handle_offloads(struct sk_buff *skb, bool gre_csum,
-					 int gso_type_mask);
+struct sk_buff *iptunnel_handle_offloads(struct sk_buff *skb, int gso_type_mask);
 
 static inline void iptunnel_xmit_stats(int err,
 				       struct net_device_stats *err_stats,
diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h
index 210eb90..b9dd3ed 100644
--- a/include/net/udp_tunnel.h
+++ b/include/net/udp_tunnel.h
@@ -103,8 +103,7 @@ static inline struct sk_buff *udp_tunnel_handle_offloads(struct sk_buff *skb,
 {
 	int type = udp_csum ? SKB_GSO_UDP_TUNNEL_CSUM : SKB_GSO_UDP_TUNNEL;
 
-	/* As we're a UDP tunnel, we support LCO, so don't need csum_help */
-	return iptunnel_handle_offloads(skb, false, type);
+	return iptunnel_handle_offloads(skb, type);
 }
 
 static inline void udp_tunnel_gro_complete(struct sk_buff *skb, int nhoff)
diff --git a/net/ipv4/fou.c b/net/ipv4/fou.c
index 3d4c9a7..8ea0519 100644
--- a/net/ipv4/fou.c
+++ b/net/ipv4/fou.c
@@ -786,7 +786,7 @@ int fou_build_header(struct sk_buff *skb, struct ip_tunnel_encap *e,
 						       SKB_GSO_UDP_TUNNEL;
 	__be16 sport;
 
-	skb = iptunnel_handle_offloads(skb, false, type);
+	skb = iptunnel_handle_offloads(skb, type);
 
 	if (IS_ERR(skb))
 		return PTR_ERR(skb);
@@ -819,7 +819,7 @@ int gue_build_header(struct sk_buff *skb, struct ip_tunnel_encap *e,
 
 	optlen += need_priv ? GUE_LEN_PRIV : 0;
 
-	skb = iptunnel_handle_offloads(skb, false, type);
+	skb = iptunnel_handle_offloads(skb, type);
 
 	if (IS_ERR(skb))
 		return PTR_ERR(skb);
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 8a589d3..8bac0ce 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -506,8 +506,7 @@ static void __gre_xmit(struct sk_buff *skb, struct net_device *dev,
 static struct sk_buff *gre_handle_offloads(struct sk_buff *skb,
 					   bool csum)
 {
-	return iptunnel_handle_offloads(skb, false,
-					csum ? SKB_GSO_GRE_CSUM : SKB_GSO_GRE);
+	return iptunnel_handle_offloads(skb, csum ? SKB_GSO_GRE_CSUM : SKB_GSO_GRE);
 }
 
 static struct rtable *gre_get_rt(struct sk_buff *skb,
diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
index 1db8418..f98bd53 100644
--- a/net/ipv4/ip_tunnel_core.c
+++ b/net/ipv4/ip_tunnel_core.c
@@ -147,7 +147,6 @@ struct metadata_dst *iptunnel_metadata_reply(struct metadata_dst *md,
 EXPORT_SYMBOL_GPL(iptunnel_metadata_reply);
 
 struct sk_buff *iptunnel_handle_offloads(struct sk_buff *skb,
-					 bool csum_help,
 					 int gso_type_mask)
 {
 	int err;
@@ -165,19 +164,7 @@ struct sk_buff *iptunnel_handle_offloads(struct sk_buff *skb,
 		return skb;
 	}
 
-	/* If packet is not gso and we are resolving any partial checksum,
-	 * clear encapsulation flag. This allows setting CHECKSUM_PARTIAL
-	 * on the outer header without confusing devices that implement
-	 * NETIF_F_IP_CSUM with encapsulation.
-	 */
-	if (csum_help)
-		skb->encapsulation = 0;
-
-	if (skb->ip_summed == CHECKSUM_PARTIAL && csum_help) {
-		err = skb_checksum_help(skb);
-		if (unlikely(err))
-			goto error;
-	} else if (skb->ip_summed != CHECKSUM_PARTIAL)
+	if (skb->ip_summed != CHECKSUM_PARTIAL)
 		skb->ip_summed = CHECKSUM_NONE;
 
 	return skb;
diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c
index 1f06729..71045c3 100644
--- a/net/ipv4/ipip.c
+++ b/net/ipv4/ipip.c
@@ -219,7 +219,7 @@ static netdev_tx_t ipip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (unlikely(skb->protocol != htons(ETH_P_IP)))
 		goto tx_error;
 
-	skb = iptunnel_handle_offloads(skb, false, SKB_GSO_IPIP);
+	skb = iptunnel_handle_offloads(skb, SKB_GSO_IPIP);
 	if (IS_ERR(skb))
 		goto out;
 
diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index dcccae8..acda206 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -912,7 +912,7 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
 		goto tx_error;
 	}
 
-	skb = iptunnel_handle_offloads(skb, false, SKB_GSO_SIT);
+	skb = iptunnel_handle_offloads(skb, SKB_GSO_SIT);
 	if (IS_ERR(skb)) {
 		ip_rt_put(rt);
 		goto out;
@@ -1003,7 +1003,7 @@ static netdev_tx_t ipip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct ip_tunnel *tunnel = netdev_priv(dev);
 	const struct iphdr  *tiph = &tunnel->parms.iph;
 
-	skb = iptunnel_handle_offloads(skb, false, SKB_GSO_IPIP);
+	skb = iptunnel_handle_offloads(skb, SKB_GSO_IPIP);
 	if (IS_ERR(skb))
 		goto out;
 
diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
index 3264cb49..a3f5cd9 100644
--- a/net/netfilter/ipvs/ip_vs_xmit.c
+++ b/net/netfilter/ipvs/ip_vs_xmit.c
@@ -1019,8 +1019,7 @@ ip_vs_tunnel_xmit(struct sk_buff *skb, struct ip_vs_conn *cp,
 	if (IS_ERR(skb))
 		goto tx_error;
 
-	skb = iptunnel_handle_offloads(
-		skb, false, __tun_gso_type_mask(AF_INET, cp->af));
+	skb = iptunnel_handle_offloads(skb, __tun_gso_type_mask(AF_INET, cp->af));
 	if (IS_ERR(skb))
 		goto tx_error;
 
@@ -1112,8 +1111,7 @@ ip_vs_tunnel_xmit_v6(struct sk_buff *skb, struct ip_vs_conn *cp,
 	if (IS_ERR(skb))
 		goto tx_error;
 
-	skb = iptunnel_handle_offloads(
-		skb, false, __tun_gso_type_mask(AF_INET6, cp->af));
+	skb = iptunnel_handle_offloads(skb, __tun_gso_type_mask(AF_INET6, cp->af));
 	if (IS_ERR(skb))
 		goto tx_error;
 

[PATCH net-next 8/8] Documentation/networking: add checksum-offloads.txt to explain LCO

[Edward Cree]

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 Documentation/networking/00-INDEX              |   2 +
 Documentation/networking/checksum-offloads.txt | 119 +++++++++++++++++++++++++
 include/linux/skbuff.h                         |   2 +
 3 files changed, 123 insertions(+)
 create mode 100644 Documentation/networking/checksum-offloads.txt

diff --git a/Documentation/networking/00-INDEX b/Documentation/networking/00-INDEX
index df27a1a..415154a 100644
--- a/Documentation/networking/00-INDEX
+++ b/Documentation/networking/00-INDEX
@@ -44,6 +44,8 @@ can.txt
 	- documentation on CAN protocol family.
 cdc_mbim.txt
 	- 3G/LTE USB modem (Mobile Broadband Interface Model)
+checksum-offloads.txt
+	- Explanation of checksum offloads; LCO, RCO
 cops.txt
 	- info on the COPS LocalTalk Linux driver
 cs89x0.txt
diff --git a/Documentation/networking/checksum-offloads.txt b/Documentation/networking/checksum-offloads.txt
new file mode 100644
index 0000000..de2a327
--- /dev/null
+++ b/Documentation/networking/checksum-offloads.txt
@@ -0,0 +1,119 @@
+Checksum Offloads in the Linux Networking Stack
+
+
+Introduction
+============
+
+This document describes a set of techniques in the Linux networking stack
+ to take advantage of checksum offload capabilities of various NICs.
+
+The following technologies are described:
+ * TX Checksum Offload
+ * LCO: Local Checksum Offload
+ * RCO: Remote Checksum Offload
+
+Things that should be documented here but aren't yet:
+ * RX Checksum Offload
+ * CHECKSUM_UNNECESSARY conversion
+
+
+TX Checksum Offload
+===================
+
+The interface for offloading a transmit checksum to a device is explained
+ in detail in comments near the top of include/linux/skbuff.h.
+In brief, it allows to request the device fill in a single ones-complement
+ checksum defined by the sk_buff fields skb->csum_start and
+ skb->csum_offset.  The device should compute the 16-bit ones-complement
+ checksum (i.e. the 'IP-style' checksum) from csum_start to the end of the
+ packet, and fill in the result at (csum_start + csum_offset).
+Because csum_offset cannot be negative, this ensures that the previous
+ value of the checksum field is included in the checksum computation, thus
+ it can be used to supply any needed corrections to the checksum (such as
+ the sum of the pseudo-header for UDP or TCP).
+This interface only allows a single checksum to be offloaded.  Where
+ encapsulation is used, the packet may have multiple checksum fields in
+ different header layers, and the rest will have to be handled by another
+ mechanism such as LCO or RCO.
+No offloading of the IP header checksum is performed; it is always done in
+ software.  This is OK because when we build the IP header, we obviously
+ have it in cache, so summing it isn't expensive.  It's also rather short.
+The requirements for GSO are more complicated, because when segmenting an
+ encapsulated packet both the inner and outer checksums may need to be
+ edited or recomputed for each resulting segment.  See the skbuff.h comment
+ (section 'E') for more details.
+
+A driver declares its offload capabilities in netdev->hw_features; see
+ Documentation/networking/netdev-features for more.  Note that a device
+ which only advertises NETIF_F_IP[V6]_CSUM must still obey the csum_start
+ and csum_offset given in the SKB; if it tries to deduce these itself in
+ hardware (as some NICs do) the driver should check that the values in the
+ SKB match those which the hardware will deduce, and if not, fall back to
+ checksumming in software instead (with skb_checksum_help or one of the
+ skb_csum_off_chk* functions as mentioned in include/linux/skbuff.h).  This
+ is a pain, but that's what you get when hardware tries to be clever.
+
+The stack should, for the most part, assume that checksum offload is
+ supported by the underlying device.  The only place that should check is
+ validate_xmit_skb(), and the functions it calls directly or indirectly.
+ That function compares the offload features requested by the SKB (which
+ may include other offloads besides TX Checksum Offload) and, if they are
+ not supported or enabled on the device (determined by netdev->features),
+ performs the corresponding offload in software.  In the case of TX
+ Checksum Offload, that means calling skb_checksum_help(skb).
+
+
+LCO: Local Checksum Offload
+===========================
+
+LCO is a technique for efficiently computing the outer checksum of an
+ encapsulated datagram when the inner checksum is due to be offloaded.
+The ones-complement sum of a correctly checksummed TCP or UDP packet is
+ equal to the sum of the pseudo header, because everything else gets
+ 'cancelled out' by the checksum field.  This is because the sum was
+ complemented before being written to the checksum field.
+More generally, this holds in any case where the 'IP-style' ones complement
+ checksum is used, and thus any checksum that TX Checksum Offload supports.
+That is, if we have set up TX Checksum Offload with a start/offset pair, we
+ know that _after the device has filled in that checksum_, the ones
+ complement sum from csum_start to the end of the packet will be equal to
+ _whatever value we put in the checksum field beforehand_.  This allows us
+ to compute the outer checksum without looking at the payload: we simply
+ stop summing when we get to csum_start, then add the 16-bit word at
+ (csum_start + csum_offset).
+Then, when the true inner checksum is filled in (either by hardware or by
+ skb_checksum_help()), the outer checksum will become correct by virtue of
+ the arithmetic.
+
+LCO is performed by the stack when constructing an outer UDP header for an
+ encapsulation such as VXLAN or GENEVE, in udp_set_csum().  Similarly for
+ the IPv6 equivalents, in udp6_set_csum().
+It is also performed when constructing an IPv4 GRE header, in
+ net/ipv4/ip_gre.c:build_header().  It is *not* currently performed when
+ constructing an IPv6 GRE header; the GRE checksum is computed over the
+ whole packet in net/ipv6/ip6_gre.c:ip6gre_xmit2(), but it should be
+ possible to use LCO here as IPv6 GRE still uses an IP-style checksum.
+All of the LCO implementations use a helper function lco_csum(), in
+ include/linux/skbuff.h.
+
+LCO can safely be used for nested encapsulations; in this case, the outer
+ encapsulation layer will sum over both its own header and the 'middle'
+ header.  This does mean that the 'middle' header will get summed multiple
+ times, but there doesn't seem to be a way to avoid that without incurring
+ bigger costs (e.g. in SKB bloat).
+
+
+RCO: Remote Checksum Offload
+============================
+
+RCO is a technique for eliding the inner checksum of an encapsulated
+ datagram, allowing the outer checksum to be offloaded.  It does, however,
+ involve a change to the encapsulation protocols, which the receiver must
+ also support.  For this reason, it is disabled by default.
+RCO is detailed in the following Internet-Drafts:
+https://tools.ietf.org/html/draft-herbert-remotecsumoffload-00
+https://tools.ietf.org/html/draft-herbert-vxlan-rco-00
+In Linux, RCO is implemented individually in each encapsulation protocol,
+ and most tunnel types have flags controlling its use.  For instance, VXLAN
+ has the flag VXLAN_F_REMCSUM_TX (per struct vxlan_rdst) to indicate that
+ RCO should be used when transmitting to a given remote destination.
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index ee063ff..448ea2e 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3668,6 +3668,8 @@ static inline unsigned int skb_gso_network_seglen(const struct sk_buff *skb)
 /* Local Checksum Offload.
  * Compute outer checksum based on the assumption that the
  * inner checksum will be offloaded later.
+ * See Documentation/networking/checksum-offloads.txt for
+ * explanation of how this works.
  * Fill in outer checksum adjustment (e.g. with sum of outer
  * pseudo-header) before calling.
  * Also ensure that inner checksum is in linear data area.

Re: [PATCH net-next 7/8] net: ip_tunnel: remove 'csum_help' argument to iptunnel_handle_offloads

[Alexander Duyck]

On Fri, Jan 8, 2016 at 11:47 AM, Edward Cree <ecree@solarflare.com> wrote:
> All users now pass false, so we can remove it, and remove the code that
>  was conditional upon it.
>
> Signed-off-by: Edward Cree <ecree@solarflare.com>
> ---
>  drivers/net/vxlan.c             |  4 ++--
>  include/net/ip_tunnels.h        |  3 +--
>  include/net/udp_tunnel.h        |  3 +--
>  net/ipv4/fou.c                  |  4 ++--
>  net/ipv4/ip_gre.c               |  3 +--
>  net/ipv4/ip_tunnel_core.c       | 15 +--------------
>  net/ipv4/ipip.c                 |  2 +-
>  net/ipv6/sit.c                  |  4 ++--
>  net/netfilter/ipvs/ip_vs_xmit.c |  6 ++----
>  9 files changed, 13 insertions(+), 31 deletions(-)
>


> diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
> index 1db8418..f98bd53 100644
> --- a/net/ipv4/ip_tunnel_core.c
> +++ b/net/ipv4/ip_tunnel_core.c
> @@ -147,7 +147,6 @@ struct metadata_dst *iptunnel_metadata_reply(struct metadata_dst *md,
>  EXPORT_SYMBOL_GPL(iptunnel_metadata_reply);
>
>  struct sk_buff *iptunnel_handle_offloads(struct sk_buff *skb,
> -                                        bool csum_help,
>                                          int gso_type_mask)
>  {
>         int err;
> @@ -165,19 +164,7 @@ struct sk_buff *iptunnel_handle_offloads(struct sk_buff *skb,
>                 return skb;
>         }
>
> -       /* If packet is not gso and we are resolving any partial checksum,
> -        * clear encapsulation flag. This allows setting CHECKSUM_PARTIAL
> -        * on the outer header without confusing devices that implement
> -        * NETIF_F_IP_CSUM with encapsulation.
> -        */
> -       if (csum_help)
> -               skb->encapsulation = 0;
> -
> -       if (skb->ip_summed == CHECKSUM_PARTIAL && csum_help) {
> -               err = skb_checksum_help(skb);
> -               if (unlikely(err))
> -                       goto error;
> -       } else if (skb->ip_summed != CHECKSUM_PARTIAL)
> +       if (skb->ip_summed != CHECKSUM_PARTIAL)
>                 skb->ip_summed = CHECKSUM_NONE;
>

So this patch is a bit broken here.  We should be clearing
skb->encapsulation if CHECKSUM_PARTIAL is not set.  That way we don't
incorrectly pull in the inner headers when computing the outer
checksum.

- Alex

Re: [PATCH net-next 7/8] net: ip_tunnel: remove 'csum_help' argument to iptunnel_handle_offloads

[Tom Herbert]

On Fri, Jan 8, 2016 at 4:35 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Fri, Jan 8, 2016 at 11:47 AM, Edward Cree <ecree@solarflare.com> wrote:
>> All users now pass false, so we can remove it, and remove the code that
>>  was conditional upon it.
>>
>> Signed-off-by: Edward Cree <ecree@solarflare.com>
>> ---
>>  drivers/net/vxlan.c             |  4 ++--
>>  include/net/ip_tunnels.h        |  3 +--
>>  include/net/udp_tunnel.h        |  3 +--
>>  net/ipv4/fou.c                  |  4 ++--
>>  net/ipv4/ip_gre.c               |  3 +--
>>  net/ipv4/ip_tunnel_core.c       | 15 +--------------
>>  net/ipv4/ipip.c                 |  2 +-
>>  net/ipv6/sit.c                  |  4 ++--
>>  net/netfilter/ipvs/ip_vs_xmit.c |  6 ++----
>>  9 files changed, 13 insertions(+), 31 deletions(-)
>>
>
>
>> diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
>> index 1db8418..f98bd53 100644
>> --- a/net/ipv4/ip_tunnel_core.c
>> +++ b/net/ipv4/ip_tunnel_core.c
>> @@ -147,7 +147,6 @@ struct metadata_dst *iptunnel_metadata_reply(struct metadata_dst *md,
>>  EXPORT_SYMBOL_GPL(iptunnel_metadata_reply);
>>
>>  struct sk_buff *iptunnel_handle_offloads(struct sk_buff *skb,
>> -                                        bool csum_help,
>>                                          int gso_type_mask)
>>  {
>>         int err;
>> @@ -165,19 +164,7 @@ struct sk_buff *iptunnel_handle_offloads(struct sk_buff *skb,
>>                 return skb;
>>         }
>>
>> -       /* If packet is not gso and we are resolving any partial checksum,
>> -        * clear encapsulation flag. This allows setting CHECKSUM_PARTIAL
>> -        * on the outer header without confusing devices that implement
>> -        * NETIF_F_IP_CSUM with encapsulation.
>> -        */
>> -       if (csum_help)
>> -               skb->encapsulation = 0;
>> -
>> -       if (skb->ip_summed == CHECKSUM_PARTIAL && csum_help) {
>> -               err = skb_checksum_help(skb);
>> -               if (unlikely(err))
>> -                       goto error;
>> -       } else if (skb->ip_summed != CHECKSUM_PARTIAL)
>> +       if (skb->ip_summed != CHECKSUM_PARTIAL)
>>                 skb->ip_summed = CHECKSUM_NONE;
>>
>
> So this patch is a bit broken here.  We should be clearing
> skb->encapsulation if CHECKSUM_PARTIAL is not set.  That way we don't
> incorrectly pull in the inner headers when computing the outer
> checksum.
>
In the original code this is done in csum_help argument is true in
iptunnel_handle_offloads. These patches essentially imply that
csum_help would always be false so we shouldn't need to clear
skb->encapsulation any more?

Tom

> - Alex

Re: [PATCH net-next 7/8] net: ip_tunnel: remove 'csum_help' argument to iptunnel_handle_offloads

[Alexander Duyck]

On Fri, Jan 8, 2016 at 4:44 PM, Tom Herbert <tom@herbertland.com> wrote:
> On Fri, Jan 8, 2016 at 4:35 PM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> On Fri, Jan 8, 2016 at 11:47 AM, Edward Cree <ecree@solarflare.com> wrote:
>>> All users now pass false, so we can remove it, and remove the code that
>>>  was conditional upon it.
>>>
>>> Signed-off-by: Edward Cree <ecree@solarflare.com>
>>> ---
>>>  drivers/net/vxlan.c             |  4 ++--
>>>  include/net/ip_tunnels.h        |  3 +--
>>>  include/net/udp_tunnel.h        |  3 +--
>>>  net/ipv4/fou.c                  |  4 ++--
>>>  net/ipv4/ip_gre.c               |  3 +--
>>>  net/ipv4/ip_tunnel_core.c       | 15 +--------------
>>>  net/ipv4/ipip.c                 |  2 +-
>>>  net/ipv6/sit.c                  |  4 ++--
>>>  net/netfilter/ipvs/ip_vs_xmit.c |  6 ++----
>>>  9 files changed, 13 insertions(+), 31 deletions(-)
>>>
>>
>>
>>> diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
>>> index 1db8418..f98bd53 100644
>>> --- a/net/ipv4/ip_tunnel_core.c
>>> +++ b/net/ipv4/ip_tunnel_core.c
>>> @@ -147,7 +147,6 @@ struct metadata_dst *iptunnel_metadata_reply(struct metadata_dst *md,
>>>  EXPORT_SYMBOL_GPL(iptunnel_metadata_reply);
>>>
>>>  struct sk_buff *iptunnel_handle_offloads(struct sk_buff *skb,
>>> -                                        bool csum_help,
>>>                                          int gso_type_mask)
>>>  {
>>>         int err;
>>> @@ -165,19 +164,7 @@ struct sk_buff *iptunnel_handle_offloads(struct sk_buff *skb,
>>>                 return skb;
>>>         }
>>>
>>> -       /* If packet is not gso and we are resolving any partial checksum,
>>> -        * clear encapsulation flag. This allows setting CHECKSUM_PARTIAL
>>> -        * on the outer header without confusing devices that implement
>>> -        * NETIF_F_IP_CSUM with encapsulation.
>>> -        */
>>> -       if (csum_help)
>>> -               skb->encapsulation = 0;
>>> -
>>> -       if (skb->ip_summed == CHECKSUM_PARTIAL && csum_help) {
>>> -               err = skb_checksum_help(skb);
>>> -               if (unlikely(err))
>>> -                       goto error;
>>> -       } else if (skb->ip_summed != CHECKSUM_PARTIAL)
>>> +       if (skb->ip_summed != CHECKSUM_PARTIAL)
>>>                 skb->ip_summed = CHECKSUM_NONE;
>>>
>>
>> So this patch is a bit broken here.  We should be clearing
>> skb->encapsulation if CHECKSUM_PARTIAL is not set.  That way we don't
>> incorrectly pull in the inner headers when computing the outer
>> checksum.
>>
> In the original code this is done in csum_help argument is true in
> iptunnel_handle_offloads. These patches essentially imply that
> csum_help would always be false so we shouldn't need to clear
> skb->encapsulation any more?

Actually it is causing me to throw warnings on an ixgbe NIC because it
implies that we want to offload the inner checksum, but we are setting
things up to offload the outer checksum.  For example if the inner is
just an ARP or ICMP frame I don't need to checksum the inner headers,
but skb->encapsulation is still set so the inner headers are being
evaluated instead of the outer ones in the Tx checksum routine in the
driver.

If we clear skb->encapsulation if we don't have CHECKSUM_PARTIAL set
then we don't have the issue.  Really the addition of the line
clearing skb->encapsulation should probably be added to the first
patch so that we don't leave skb->encapsulation set when we aren't
requesting offloads.

- Alex

Re: [PATCH net-next 7/8] net: ip_tunnel: remove 'csum_help' argument to iptunnel_handle_offloads

[Tom Herbert]

On Fri, Jan 8, 2016 at 6:05 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Fri, Jan 8, 2016 at 4:44 PM, Tom Herbert <tom@herbertland.com> wrote:
>> On Fri, Jan 8, 2016 at 4:35 PM, Alexander Duyck
>> <alexander.duyck@gmail.com> wrote:
>>> On Fri, Jan 8, 2016 at 11:47 AM, Edward Cree <ecree@solarflare.com> wrote:
>>>> All users now pass false, so we can remove it, and remove the code that
>>>>  was conditional upon it.
>>>>
>>>> Signed-off-by: Edward Cree <ecree@solarflare.com>
>>>> ---
>>>>  drivers/net/vxlan.c             |  4 ++--
>>>>  include/net/ip_tunnels.h        |  3 +--
>>>>  include/net/udp_tunnel.h        |  3 +--
>>>>  net/ipv4/fou.c                  |  4 ++--
>>>>  net/ipv4/ip_gre.c               |  3 +--
>>>>  net/ipv4/ip_tunnel_core.c       | 15 +--------------
>>>>  net/ipv4/ipip.c                 |  2 +-
>>>>  net/ipv6/sit.c                  |  4 ++--
>>>>  net/netfilter/ipvs/ip_vs_xmit.c |  6 ++----
>>>>  9 files changed, 13 insertions(+), 31 deletions(-)
>>>>
>>>
>>>
>>>> diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
>>>> index 1db8418..f98bd53 100644
>>>> --- a/net/ipv4/ip_tunnel_core.c
>>>> +++ b/net/ipv4/ip_tunnel_core.c
>>>> @@ -147,7 +147,6 @@ struct metadata_dst *iptunnel_metadata_reply(struct metadata_dst *md,
>>>>  EXPORT_SYMBOL_GPL(iptunnel_metadata_reply);
>>>>
>>>>  struct sk_buff *iptunnel_handle_offloads(struct sk_buff *skb,
>>>> -                                        bool csum_help,
>>>>                                          int gso_type_mask)
>>>>  {
>>>>         int err;
>>>> @@ -165,19 +164,7 @@ struct sk_buff *iptunnel_handle_offloads(struct sk_buff *skb,
>>>>                 return skb;
>>>>         }
>>>>
>>>> -       /* If packet is not gso and we are resolving any partial checksum,
>>>> -        * clear encapsulation flag. This allows setting CHECKSUM_PARTIAL
>>>> -        * on the outer header without confusing devices that implement
>>>> -        * NETIF_F_IP_CSUM with encapsulation.
>>>> -        */
>>>> -       if (csum_help)
>>>> -               skb->encapsulation = 0;
>>>> -
>>>> -       if (skb->ip_summed == CHECKSUM_PARTIAL && csum_help) {
>>>> -               err = skb_checksum_help(skb);
>>>> -               if (unlikely(err))
>>>> -                       goto error;
>>>> -       } else if (skb->ip_summed != CHECKSUM_PARTIAL)
>>>> +       if (skb->ip_summed != CHECKSUM_PARTIAL)
>>>>                 skb->ip_summed = CHECKSUM_NONE;
>>>>
>>>
>>> So this patch is a bit broken here.  We should be clearing
>>> skb->encapsulation if CHECKSUM_PARTIAL is not set.  That way we don't
>>> incorrectly pull in the inner headers when computing the outer
>>> checksum.
>>>
>> In the original code this is done in csum_help argument is true in
>> iptunnel_handle_offloads. These patches essentially imply that
>> csum_help would always be false so we shouldn't need to clear
>> skb->encapsulation any more?
>
> Actually it is causing me to throw warnings on an ixgbe NIC because it
> implies that we want to offload the inner checksum, but we are setting
> things up to offload the outer checksum.  For example if the inner is
> just an ARP or ICMP frame I don't need to checksum the inner headers,
> but skb->encapsulation is still set so the inner headers are being
> evaluated instead of the outer ones in the Tx checksum routine in the
> driver.
>
> If we clear skb->encapsulation if we don't have CHECKSUM_PARTIAL set
> then we don't have the issue.  Really the addition of the line
> clearing skb->encapsulation should probably be added to the first
> patch so that we don't leave skb->encapsulation set when we aren't
> requesting offloads.
>
As I pointed out previously drivers that use skb->encapsulation to
determine that an inner checksum is being offloaded are not correct.
The checksum to be offloaded is indicated solely in csum_start and
csum_offset, skb->encapulation really should have nothing to do with
checksum. This is just and indication that the inner headers are valid
for the skbuf. We can retain clearing of skb->encapasulation for
compatibility until the drivers that handle encapsulated checksums
based on skb->encapsulation (I think there's like six of them) are
converted to read csum_start/csum_offset (i.e. using
skb_csum_offload_chk). New driver implemenation should not be using
skb->encapsulation in the checksum offload interface.

Thanks,
Tom

> - Alex

Re: [PATCH net-next 7/8] net: ip_tunnel: remove 'csum_help' argument to iptunnel_handle_offloads

[Alexander Duyck]

On Fri, Jan 8, 2016 at 7:00 PM, Tom Herbert <tom@herbertland.com> wrote:
> On Fri, Jan 8, 2016 at 6:05 PM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> On Fri, Jan 8, 2016 at 4:44 PM, Tom Herbert <tom@herbertland.com> wrote:
>>> On Fri, Jan 8, 2016 at 4:35 PM, Alexander Duyck
>>> <alexander.duyck@gmail.com> wrote:
>>>> On Fri, Jan 8, 2016 at 11:47 AM, Edward Cree <ecree@solarflare.com> wrote:
>>>>> All users now pass false, so we can remove it, and remove the code that
>>>>>  was conditional upon it.
>>>>>
>>>>> Signed-off-by: Edward Cree <ecree@solarflare.com>
>>>>> ---
>>>>>  drivers/net/vxlan.c             |  4 ++--
>>>>>  include/net/ip_tunnels.h        |  3 +--
>>>>>  include/net/udp_tunnel.h        |  3 +--
>>>>>  net/ipv4/fou.c                  |  4 ++--
>>>>>  net/ipv4/ip_gre.c               |  3 +--
>>>>>  net/ipv4/ip_tunnel_core.c       | 15 +--------------
>>>>>  net/ipv4/ipip.c                 |  2 +-
>>>>>  net/ipv6/sit.c                  |  4 ++--
>>>>>  net/netfilter/ipvs/ip_vs_xmit.c |  6 ++----
>>>>>  9 files changed, 13 insertions(+), 31 deletions(-)
>>>>>
>>>>
>>>>
>>>>> diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
>>>>> index 1db8418..f98bd53 100644
>>>>> --- a/net/ipv4/ip_tunnel_core.c
>>>>> +++ b/net/ipv4/ip_tunnel_core.c
>>>>> @@ -147,7 +147,6 @@ struct metadata_dst *iptunnel_metadata_reply(struct metadata_dst *md,
>>>>>  EXPORT_SYMBOL_GPL(iptunnel_metadata_reply);
>>>>>
>>>>>  struct sk_buff *iptunnel_handle_offloads(struct sk_buff *skb,
>>>>> -                                        bool csum_help,
>>>>>                                          int gso_type_mask)
>>>>>  {
>>>>>         int err;
>>>>> @@ -165,19 +164,7 @@ struct sk_buff *iptunnel_handle_offloads(struct sk_buff *skb,
>>>>>                 return skb;
>>>>>         }
>>>>>
>>>>> -       /* If packet is not gso and we are resolving any partial checksum,
>>>>> -        * clear encapsulation flag. This allows setting CHECKSUM_PARTIAL
>>>>> -        * on the outer header without confusing devices that implement
>>>>> -        * NETIF_F_IP_CSUM with encapsulation.
>>>>> -        */
>>>>> -       if (csum_help)
>>>>> -               skb->encapsulation = 0;
>>>>> -
>>>>> -       if (skb->ip_summed == CHECKSUM_PARTIAL && csum_help) {
>>>>> -               err = skb_checksum_help(skb);
>>>>> -               if (unlikely(err))
>>>>> -                       goto error;
>>>>> -       } else if (skb->ip_summed != CHECKSUM_PARTIAL)
>>>>> +       if (skb->ip_summed != CHECKSUM_PARTIAL)
>>>>>                 skb->ip_summed = CHECKSUM_NONE;
>>>>>
>>>>
>>>> So this patch is a bit broken here.  We should be clearing
>>>> skb->encapsulation if CHECKSUM_PARTIAL is not set.  That way we don't
>>>> incorrectly pull in the inner headers when computing the outer
>>>> checksum.
>>>>
>>> In the original code this is done in csum_help argument is true in
>>> iptunnel_handle_offloads. These patches essentially imply that
>>> csum_help would always be false so we shouldn't need to clear
>>> skb->encapsulation any more?
>>
>> Actually it is causing me to throw warnings on an ixgbe NIC because it
>> implies that we want to offload the inner checksum, but we are setting
>> things up to offload the outer checksum.  For example if the inner is
>> just an ARP or ICMP frame I don't need to checksum the inner headers,
>> but skb->encapsulation is still set so the inner headers are being
>> evaluated instead of the outer ones in the Tx checksum routine in the
>> driver.
>>
>> If we clear skb->encapsulation if we don't have CHECKSUM_PARTIAL set
>> then we don't have the issue.  Really the addition of the line
>> clearing skb->encapsulation should probably be added to the first
>> patch so that we don't leave skb->encapsulation set when we aren't
>> requesting offloads.
>>
> As I pointed out previously drivers that use skb->encapsulation to
> determine that an inner checksum is being offloaded are not correct.
> The checksum to be offloaded is indicated solely in csum_start and
> csum_offset, skb->encapulation really should have nothing to do with
> checksum. This is just and indication that the inner headers are valid
> for the skbuf. We can retain clearing of skb->encapasulation for
> compatibility until the drivers that handle encapsulated checksums
> based on skb->encapsulation (I think there's like six of them) are
> converted to read csum_start/csum_offset (i.e. using
> skb_csum_offload_chk). New driver implemenation should not be using
> skb->encapsulation in the checksum offload interface.

Well part of the problem is that the stack is doing some of this as
well.  In validate_xmit_skb what is happening is that the
inner_transport_hdr is being reset with the csum_start value.

Odds are the quickest way to fix most of the drivers would be to drop
their checks for skb->encapsulation and instead add a function to
determine if skb_checksum_start_offset is equal to
skb_transport_offset.  If it is not then we can assume we are doing
checksums for an inner header instead of the outer one.

I was actually looking over the code for the Intel drivers tonight
while sorting this out.  I realized the drivers were far more complex
then they needed to be based on their datasheets.  I should have
patches next week that will convert igb, ixgbe, igbvf, and ixgbevf
over to using NETIF_F_HW_CSUM instead of the IP/IPV6_CSUM logic they
do now.  The only real snag I am sorting out is how to deal with
SCTP_CRC.  I'm wondering if it is okay for me to simply assume that if
skb->csum_offset is 8 we are dealing with an SCTP crc32c request or do
you think I should look into another way of identifying SCTP frames?

- Alex

RE: [PATCH net-next 6/8] net: gre: Implement LCO for GRE over IPv4

[David Laight]

From: Edward Cree
> Sent: 08 January 2016 19:47
...
> +	if (skb->ip_summed == CHECKSUM_PARTIAL) {
> +		csum = csum_fold(lco_csum(skb));
> +		if (csum == 0)
> +			csum = CSUM_MANGLED_0;
> +		return csum;
> +	} else {
> +		return csum_fold(skb_checksum(skb, 0, skb->len, 0));
> +	}

You see to be worried about csum_fold() returning 0 in one
path, but not in the other.
I'm guessing that 0 can only happen if all the bytes that have
been checksummed are zero.
This is almost certainly never true if any bytes have been summed,
and might be better avoided by initialising any such checksum to
0xffff (instead of 0) much earlier on.

	David

Re: [PATCH net-next 6/8] net: gre: Implement LCO for GRE over IPv4

[Edward Cree]

On 11/01/16 10:09, David Laight wrote:
> From: Edward Cree
>> Sent: 08 January 2016 19:47
> ...
>> +	if (skb->ip_summed == CHECKSUM_PARTIAL) {
>> +		csum = csum_fold(lco_csum(skb));
>> +		if (csum == 0)
>> +			csum = CSUM_MANGLED_0;
>> +		return csum;
>> +	} else {
>> +		return csum_fold(skb_checksum(skb, 0, skb->len, 0));
>> +	}
> You see to be worried about csum_fold() returning 0 in one
> path, but not in the other.
> I'm guessing that 0 can only happen if all the bytes that have
> been checksummed are zero.
csum_fold complements, so if the sum comes to (say) 0x1fffe, it will return ~0xffff which is 0.

Next version of patch will mangle 0 for both branches.

-ed

Re: [PATCH net-next 7/8] net: ip_tunnel: remove 'csum_help' argument to iptunnel_handle_offloads

[Edward Cree]

On 09/01/16 02:05, Alexander Duyck wrote:
> If we clear skb->encapsulation if we don't have CHECKSUM_PARTIAL set
> then we don't have the issue.  Really the addition of the line
> clearing skb->encapsulation should probably be added to the first
> patch so that we don't leave skb->encapsulation set when we aren't
> requesting offloads.
Next version of series will have (in this patch)
  if (skb->ip_summed != CHECKSUM_PARTIAL) {
    skb->ip_summed = CHECKSUM_NONE;
    skb->encapsulation = 0;
  }

Will that do, or does it need to be squeezed into an earlier patch in the series (which one?)

-ed

Re: [PATCH net-next 7/8] net: ip_tunnel: remove 'csum_help' argument to iptunnel_handle_offloads

[Alexander Duyck]

On Mon, Jan 11, 2016 at 5:24 AM, Edward Cree <ecree@solarflare.com> wrote:
> On 09/01/16 02:05, Alexander Duyck wrote:
>> If we clear skb->encapsulation if we don't have CHECKSUM_PARTIAL set
>> then we don't have the issue.  Really the addition of the line
>> clearing skb->encapsulation should probably be added to the first
>> patch so that we don't leave skb->encapsulation set when we aren't
>> requesting offloads.
> Next version of series will have (in this patch)
>   if (skb->ip_summed != CHECKSUM_PARTIAL) {
>     skb->ip_summed = CHECKSUM_NONE;
>     skb->encapsulation = 0;
>   }
>
> Will that do, or does it need to be squeezed into an earlier patch in the series (which one?)

Your first patch is probably the best place for it.  Then when you
start setting false it doesn't introduce any errors.

Also I was doing a bit more work on the lco_csum function and think I
have come up with something a bit more elegant that allows for the
function to be easily used in the GSO routine for UDP tunnels.  Here
is a snippet from the patch I am working with below.  Odds are the
formatting will get trashed due to my mail client.  What I will do is
email you the full patch and the GSO patch I have as RFCs to look over
and possibly incorporate into your own.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>

---

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 6b6bd42d6134..94244ab47e1b 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2161,6 +2161,11 @@ static inline int
skb_checksum_start_offset(const struct sk_buff *skb)
        return skb->csum_start - skb_headroom(skb);
 }

+static inline unsigned char *skb_checksum_start(const struct sk_buff *skb)
+{
+       return skb->head + skb->csum_start;
+}
+
 static inline int skb_transport_offset(const struct sk_buff *skb)
 {
        return skb_transport_header(skb) - skb->data;
@@ -3578,6 +3583,17 @@ static inline __sum16 gso_make_checksum(struct
sk_buff *skb, __wsum res)
        return csum_fold(partial);
 }

+static inline __wsum lco_csum(struct sk_buff *skb)
+{
+       unsigned char *csum_start = skb_checksum_start(skb);
+       unsigned char *l4_hdr = skb_transport_header(skb);
+       __wsum partial;
+
+       partial = ~csum_unfold(*(__force __sum16 *)(csum_start +
+                                                   skb->csum_offset));
+       return csum_partial(l4_hdr, csum_start - l4_hdr, partial);
+}
+
 static inline bool skb_is_gso(const struct sk_buff *skb)
 {
        return skb_shinfo(skb)->gso_size;

Re: [PATCH net-next 7/8] net: ip_tunnel: remove 'csum_help' argument to iptunnel_handle_offloads

[Edward Cree]

On 11/01/16 16:39, Alexander Duyck wrote:
> Your first patch is probably the best place for it.  Then when you
> start setting false it doesn't introduce any errors.
Will do.
> Also I was doing a bit more work on the lco_csum function and think I
> have come up with something a bit more elegant[...]  What I will do is
> email you the full patch and the GSO patch I have as RFCs to look over
> and possibly incorporate into your own.
<snip>
> +static inline __wsum lco_csum(struct sk_buff *skb)
> +{
> +       unsigned char *csum_start = skb_checksum_start(skb);
> +       unsigned char *l4_hdr = skb_transport_header(skb);
> +       __wsum partial;
> +
> +       partial = ~csum_unfold(*(__force __sum16 *)(csum_start +
> +                                                   skb->csum_offset));
> +       return csum_partial(l4_hdr, csum_start - l4_hdr, partial);
> +}
> +
Looks OK to me.  I'd rather tack both of your patches onto the endof the
 series, rather than incorporating your patch [1/2] directly into my patch
 [1/8]; that way (a) the history allows to understand regular LCO before
 adding in the GSO flavour, (b) you're credited for your improved lco_csum.
As for your patch [2/2], I don't pretend to understand GSO right now but it
 looks plausible enough.  Perhaps you could add a document about GSO to go
 alongside the checksum-offloads.txt one?
What testing haveyou done on your series?  When rebasing it I'll focus on
 the tunnel types you haven't already tested.

-ed

Re: [PATCH net-next 7/8] net: ip_tunnel: remove 'csum_help' argument to iptunnel_handle_offloads

[Alexander Duyck]

On Mon, Jan 11, 2016 at 9:31 AM, Edward Cree <ecree@solarflare.com> wrote:
> On 11/01/16 16:39, Alexander Duyck wrote:
>> Your first patch is probably the best place for it.  Then when you
>> start setting false it doesn't introduce any errors.
> Will do.
>> Also I was doing a bit more work on the lco_csum function and think I
>> have come up with something a bit more elegant[...]  What I will do is
>> email you the full patch and the GSO patch I have as RFCs to look over
>> and possibly incorporate into your own.
> <snip>
>> +static inline __wsum lco_csum(struct sk_buff *skb)
>> +{
>> +       unsigned char *csum_start = skb_checksum_start(skb);
>> +       unsigned char *l4_hdr = skb_transport_header(skb);
>> +       __wsum partial;
>> +
>> +       partial = ~csum_unfold(*(__force __sum16 *)(csum_start +
>> +                                                   skb->csum_offset));
>> +       return csum_partial(l4_hdr, csum_start - l4_hdr, partial);
>> +}
>> +
> Looks OK to me.  I'd rather tack both of your patches onto the endof the
>  series, rather than incorporating your patch [1/2] directly into my patch
>  [1/8]; that way (a) the history allows to understand regular LCO before
>  adding in the GSO flavour, (b) you're credited for your improved lco_csum.

Actually if you don't plan to incorporate the function I am fine with
you just leaving it out for now.  I can focus on the GSO portions of
all this and rewrite this to just be an update of your patch.  Then
when you submit your patches and they are accepted I will submit the
GSO implementations.

> As for your patch [2/2], I don't pretend to understand GSO right now but it
>  looks plausible enough.  Perhaps you could add a document about GSO to go
>  alongside the checksum-offloads.txt one?

I don't know if we really need to.  The logic is essentially the same
as what you already have for the local checksum offload.  The only
difference is there is the GSO logic floating around in there to also
compute the outer checksum offload based on the inner that GSO had
already retained when it did the inner offload via software.

Feel free to just exclude it if you want.  I think there may be some
more work to be done on it, for example I am not sure if I actually
needed to move oflload_csum.  If I am not mistaken I think we might be
able to drop the skb->encap_hdr_csum the same way you dropped the
value from iptunnel_handle_offloads.  I might also spend some time
working on the GRE version of the patch as well.  Maybe I can see if
skb->encap_hdr_csum can be dropped.

> What testing haveyou done on your series?  When rebasing it I'll focus on
>  the tunnel types you haven't already tested.

I have been testing with the two Intel NICs I have using just VXLAN as
that was my focus.  I just submitted some patches to the Intel guys
that enable NETIF_F_HW_CSUM for igb, ixgbe, igbvf, and ixgbevf.  With
those patches in place I have been passing traffic without seeing any
issues.  I've been using wireshark to monitor the packets and verify
checksums on the receive side periodically.  It all seems to be
working correctly.

- Alex

Re: [PATCH net-next 6/8] net: gre: Implement LCO for GRE over IPv4

[Alexander Duyck]

On Mon, Jan 11, 2016 at 5:21 AM, Edward Cree <ecree@solarflare.com> wrote:
> On 11/01/16 10:09, David Laight wrote:
>> From: Edward Cree
>>> Sent: 08 January 2016 19:47
>> ...
>>> +    if (skb->ip_summed == CHECKSUM_PARTIAL) {
>>> +            csum = csum_fold(lco_csum(skb));
>>> +            if (csum == 0)
>>> +                    csum = CSUM_MANGLED_0;
>>> +            return csum;
>>> +    } else {
>>> +            return csum_fold(skb_checksum(skb, 0, skb->len, 0));
>>> +    }
>> You see to be worried about csum_fold() returning 0 in one
>> path, but not in the other.
>> I'm guessing that 0 can only happen if all the bytes that have
>> been checksummed are zero.
> csum_fold complements, so if the sum comes to (say) 0x1fffe, it will return ~0xffff which is 0.
>
> Next version of patch will mangle 0 for both branches.

Actually you may want to go the other way on that.  If they weren't
flipping the checksum value for GRE before why should we start doing
that now?  I'm pretty sure the checksum mangling is a very UDP centric
thing.  There is no need to introduce it to other protocols.

- Alex

Re: [PATCH net-next 6/8] net: gre: Implement LCO for GRE over IPv4

[Edward Cree]

On 11/01/16 18:39, Alexander Duyck wrote:
> On Mon, Jan 11, 2016 at 5:21 AM, Edward Cree <ecree@solarflare.com> wrote:
>> csum_fold complements, so if the sum comes to (say) 0x1fffe, it will return ~0xffff which is 0.
>>
>> Next version of patch will mangle 0 for both branches.
> Actually you may want to go the other way on that.  If they weren't
> flipping the checksum value for GRE before why should we start doing
> that now?  I'm pretty sure the checksum mangling is a very UDP centric
> thing.  There is no need to introduce it to other protocols.
Good catch, you're quite right.

-ed

Re: [PATCH net-next 7/8] net: ip_tunnel: remove 'csum_help' argument to iptunnel_handle_offloads

[Edward Cree]

On 11/01/16 18:15, Alexander Duyck wrote:
> On Mon, Jan 11, 2016 at 9:31 AM, Edward Cree <ecree@solarflare.com> wrote:
>> Looks OK to me.  I'd rather tack both of your patches onto the endof the
>>  series, rather than incorporating your patch [1/2] directly into my patch
>>  [1/8]; that way (a) the history allows to understand regular LCO before
>>  adding in the GSO flavour, (b) you're credited for your improved lco_csum.
> Actually if you don't plan to incorporate the function I am fine with
> you just leaving it out for now.  I can focus on the GSO portions of
> all this and rewrite this to just be an update of your patch.  Then
> when you submit your patches and they are accepted I will submit the
> GSO implementations.
Ok, sounds good.
>> As for your patch [2/2], I don't pretend to understand GSO right now but it
>>  looks plausible enough.  Perhaps you could add a document about GSO to go
>>  alongside the checksum-offloads.txt one?
> I don't know if we really need to.  The logic is essentially the same
> as what you already have for the local checksum offload.  The only
> difference is there is the GSO logic floating around in there to also
> compute the outer checksum offload based on the inner that GSO had
> already retained when it did the inner offload via software.
I didn't mean a GSO LCO document, I just meant something to explain GSOas
 a whole.  There doesn't appear to be any documentation in the tree
 defining what e.g. gso_size or gso_segs mean, and yet they are part of
 the driver API for TSO.  Even some comments on struct skb_shared_info
 would be an improvement.
The comment in skbuff.h seems very much to gloss over how GSO and checksum
 offload interact in general.  It also says two checksums can be offloaded
 with UDP tunnels - you might want to update that in your patch.
But hopefully the GSO stack can be fully LCOified at which point the
 checksum offload semantics for a GSO skb will be the same as a regular
 skb.  As for TSO, if the driver/hw has enough information to do TSO, it
 must know where all the headers are, so it must be able to do the right
 checksum corrections on all of them.  (I think outer checksums are only
 affected by the Length and pseudo-header Length field changes.)
However, this does raise the question - will TSO be possible with nested
 encap?  And if not, will current drivers do the right thing in that case,
 or will they try to do encap TSO and only get two of the three layers?

-ed

Re: [PATCH net-next 7/8] net: ip_tunnel: remove 'csum_help' argument to iptunnel_handle_offloads

[Alexander Duyck]

On Mon, Jan 11, 2016 at 11:03 AM, Edward Cree <ecree@solarflare.com> wrote:
> On 11/01/16 18:15, Alexander Duyck wrote:
>> On Mon, Jan 11, 2016 at 9:31 AM, Edward Cree <ecree@solarflare.com> wrote:
>>> Looks OK to me.  I'd rather tack both of your patches onto the endof the
>>>  series, rather than incorporating your patch [1/2] directly into my patch
>>>  [1/8]; that way (a) the history allows to understand regular LCO before
>>>  adding in the GSO flavour, (b) you're credited for your improved lco_csum.
>> Actually if you don't plan to incorporate the function I am fine with
>> you just leaving it out for now.  I can focus on the GSO portions of
>> all this and rewrite this to just be an update of your patch.  Then
>> when you submit your patches and they are accepted I will submit the
>> GSO implementations.
> Ok, sounds good.
>>> As for your patch [2/2], I don't pretend to understand GSO right now but it
>>>  looks plausible enough.  Perhaps you could add a document about GSO to go
>>>  alongside the checksum-offloads.txt one?
>> I don't know if we really need to.  The logic is essentially the same
>> as what you already have for the local checksum offload.  The only
>> difference is there is the GSO logic floating around in there to also
>> compute the outer checksum offload based on the inner that GSO had
>> already retained when it did the inner offload via software.
> I didn't mean a GSO LCO document, I just meant something to explain GSOas
>  a whole.  There doesn't appear to be any documentation in the tree
>  defining what e.g. gso_size or gso_segs mean, and yet they are part of
>  the driver API for TSO.  Even some comments on struct skb_shared_info
>  would be an improvement.

I'm not that much of an expert on GSO, I'm just able to read the
functions and sort out what they are doing.  From the looks of things
with the updated lco_csum I provided you we could probably even use it
in drivers if needed in order to provide an offload.  Most of this is
actually pretty straight forward since the functionality for GSO doing
checksums is pretty much the same as for the standard path.

> The comment in skbuff.h seems very much to gloss over how GSO and checksum
>  offload interact in general.  It also says two checksums can be offloaded
>  with UDP tunnels - you might want to update that in your patch.

>From what I can tell this is sort-of true.  It is offloaded in that
the protocol itself doesn't handle it.  Instead what happens is that
GSO maintains a running checksum in skb->csum and places checksums in
the headers as it goes.  So in essence it is doing something similar
to LCO, but it requires that that we let software offload the inner
checksum instead of handling it in hardware.  Now that I think about
it I might rebase some of my current GSO work for these tunnels to
incorporate the existing gso_make_checksum call instead of lco since
that may make better use of existing infrastructure.

> But hopefully the GSO stack can be fully LCOified at which point the
>  checksum offload semantics for a GSO skb will be the same as a regular
>  skb.  As for TSO, if the driver/hw has enough information to do TSO, it
>  must know where all the headers are, so it must be able to do the right
>  checksum corrections on all of them.  (I think outer checksums are only
>  affected by the Length and pseudo-header Length field changes.)
> However, this does raise the question - will TSO be possible with nested
>  encap?  And if not, will current drivers do the right thing in that case,
>  or will they try to do encap TSO and only get two of the three layers?

We shouldn't need to worry about multiple layers.  The
hw_encap_features is meant to prevent that.  Once you have a tunnel in
a tunnel the inner-most tunnel would lose all offload support.  I
believe the only feature that is passed through all the way is
NETIF_F_SG.  I believe the same is currently true for vlans

Re: [PATCH net-next 6/8] net: gre: Implement LCO for GRE over IPv4

[Rustad, Mark D]

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

Alexander Duyck <alexander.duyck@gmail.com> wrote:

> Actually you may want to go the other way on that.  If they weren't
> flipping the checksum value for GRE before why should we start doing
> that now?  I'm pretty sure the checksum mangling is a very UDP centric
> thing.  There is no need to introduce it to other protocols.

If different checksum representations are needed, then there really should  
be an explicit indication of whether it is a UDP-style checksum or other in  
the skb I would think rather than guessing it based on the offset. Of  
course it would be convenient if all the protocols that use a one's  
complement checksum would tolerate the UDP representation. I have a long  
(and now old) history working with real one's complement machines, and so I  
would want to believe that any correct implementation would tolerate it,  
but I don't know for a fact that they do.

Re: [PATCH net-next 6/8] net: gre: Implement LCO for GRE over IPv4

[Alexander Duyck]

On Wed, Jan 20, 2016 at 11:11 AM, Rustad, Mark D
<mark.d.rustad@intel.com> wrote:
> Alexander Duyck <alexander.duyck@gmail.com> wrote:
>
>> Actually you may want to go the other way on that.  If they weren't
>> flipping the checksum value for GRE before why should we start doing
>> that now?  I'm pretty sure the checksum mangling is a very UDP centric
>> thing.  There is no need to introduce it to other protocols.
>
>
> If different checksum representations are needed, then there really should
> be an explicit indication of whether it is a UDP-style checksum or other in
> the skb I would think rather than guessing it based on the offset. Of course
> it would be convenient if all the protocols that use a one's complement
> checksum would tolerate the UDP representation. I have a long (and now old)
> history working with real one's complement machines, and so I would want to
> believe that any correct implementation would tolerate it, but I don't know
> for a fact that they do.

The only reason why UDP does the bit flip is because it has reserved a
checksum of 0 as a special value.  For the checksum math itself either
0xFFFF or 0 are interchangeable.  The only time they would make any
difference would be if we had a value of 0 that we were checksumming,
but since that is not the case the values will always end up
converging back onto 0xFFFF as the final result in the case of a
correct checksum.

- Alex

Re: [PATCH net-next 6/8] net: gre: Implement LCO for GRE over IPv4

[Tom Herbert]

On Wed, Jan 20, 2016 at 11:35 AM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Wed, Jan 20, 2016 at 11:11 AM, Rustad, Mark D
> <mark.d.rustad@intel.com> wrote:
>> Alexander Duyck <alexander.duyck@gmail.com> wrote:
>>
>>> Actually you may want to go the other way on that.  If they weren't
>>> flipping the checksum value for GRE before why should we start doing
>>> that now?  I'm pretty sure the checksum mangling is a very UDP centric
>>> thing.  There is no need to introduce it to other protocols.
>>
>>
>> If different checksum representations are needed, then there really should
>> be an explicit indication of whether it is a UDP-style checksum or other in
>> the skb I would think rather than guessing it based on the offset. Of course
>> it would be convenient if all the protocols that use a one's complement
>> checksum would tolerate the UDP representation. I have a long (and now old)
>> history working with real one's complement machines, and so I would want to
>> believe that any correct implementation would tolerate it, but I don't know
>> for a fact that they do.
>
> The only reason why UDP does the bit flip is because it has reserved a
> checksum of 0 as a special value.  For the checksum math itself either
> 0xFFFF or 0 are interchangeable.  The only time they would make any
> difference would be if we had a value of 0 that we were checksumming,
> but since that is not the case the values will always end up
> converging back onto 0xFFFF as the final result in the case of a
> correct checksum.
>
0xffff is mathematically equivalent to 0x0 for checksum. I would
rather always flip 0 to 0xffff in LCO rather than adding an explicit
indication (i.e. another flag) in SKB that it has a UDP checksum.

Tom

> - Alex

Re: [PATCH net-next 6/8] net: gre: Implement LCO for GRE over IPv4

[Alexander Duyck]

On Wed, Jan 20, 2016 at 11:58 AM, Tom Herbert <tom@herbertland.com> wrote:
> On Wed, Jan 20, 2016 at 11:35 AM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> On Wed, Jan 20, 2016 at 11:11 AM, Rustad, Mark D
>> <mark.d.rustad@intel.com> wrote:
>>> Alexander Duyck <alexander.duyck@gmail.com> wrote:
>>>
>>>> Actually you may want to go the other way on that.  If they weren't
>>>> flipping the checksum value for GRE before why should we start doing
>>>> that now?  I'm pretty sure the checksum mangling is a very UDP centric
>>>> thing.  There is no need to introduce it to other protocols.
>>>
>>>
>>> If different checksum representations are needed, then there really should
>>> be an explicit indication of whether it is a UDP-style checksum or other in
>>> the skb I would think rather than guessing it based on the offset. Of course
>>> it would be convenient if all the protocols that use a one's complement
>>> checksum would tolerate the UDP representation. I have a long (and now old)
>>> history working with real one's complement machines, and so I would want to
>>> believe that any correct implementation would tolerate it, but I don't know
>>> for a fact that they do.
>>
>> The only reason why UDP does the bit flip is because it has reserved a
>> checksum of 0 as a special value.  For the checksum math itself either
>> 0xFFFF or 0 are interchangeable.  The only time they would make any
>> difference would be if we had a value of 0 that we were checksumming,
>> but since that is not the case the values will always end up
>> converging back onto 0xFFFF as the final result in the case of a
>> correct checksum.
>>
> 0xffff is mathematically equivalent to 0x0 for checksum. I would
> rather always flip 0 to 0xffff in LCO rather than adding an explicit
> indication (i.e. another flag) in SKB that it has a UDP checksum.

There isn't any need to add such an indication, nor do we need to
start bitflipping the return value from csum_fold in all cases.  I
think there was just some confusion about UDP checksums vs GRE or TCP
checksums.

I'd say we are better off keeping this simple.  The original patch
just needs to drop the check for the resultant checksum being 0 since
that is not needed for GRE.

- Alex

Re: [PATCH net-next 6/8] net: gre: Implement LCO for GRE over IPv4

[Rustad, Mark D]

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

Alexander Duyck <alexander.duyck@gmail.com> wrote:

> There isn't any need to add such an indication, nor do we need to
> start bitflipping the return value from csum_fold in all cases.  I
> think there was just some confusion about UDP checksums vs GRE or TCP
> checksums.

Yeah. I think I finally got there. The naive software methods will never  
generate a true 0 unless everything was zero. Real one's complement  
machines did addition in terms of subtraction so that 1 + -1 would never  
produce a -0, only a normal 0. Of course a simple adder would produce a -0,  
making it impossible to get back to a normal 0.

> I'd say we are better off keeping this simple.  The original patch
> just needs to drop the check for the resultant checksum being 0 since
> that is not needed for GRE.

I'm all in favor of simple. I had just started to worry about a possible  
change in behavior that might have interoperability problems with some  
implementations. I wonder if any implementation ever did the addition by  
subtraction, but also failed to make 0 compare equal to -0? I guess if they  
knew enough to do the former, they should not have blown the latter.

Re: [PATCH net-next 1/8] net: local checksum offload for encapsulation

[Zang MingJie]

I have also noticed that for gso, all gso segs will have exactly same 
outer udp checksum, this is also because inner checksum cancellation.

Can we also optimize that outer udp checksum should be only calculated 
once for all gso segs ?

On 01/09/2016 03:45 AM, Edward Cree wrote:
> The arithmetic properties of the ones-complement checksum mean that a
>   correctly checksummed inner packet, including its checksum, has a ones
>   complement sum depending only on whatever value was used to initialise
>   the checksum field before checksumming (in the case of TCP and UDP,
>   this is the ones complement sum of the pseudo header, complemented).
> Consequently, if we are going to offload the inner checksum with
>   CHECKSUM_PARTIAL, we can compute the outer checksum based only on the
>   packed data not covered by the inner checksum, and the initial value of
>   the inner checksum field.
>
> Signed-off-by: Edward Cree <ecree@solarflare.com>
> ---
>   include/linux/skbuff.h  | 24 ++++++++++++++++++++++++
>   net/ipv4/udp.c          | 20 ++++++++++----------
>   net/ipv6/ip6_checksum.c | 14 +++++++-------
>   3 files changed, 41 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 6b6bd42..ee063ff 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3665,5 +3665,29 @@ static inline unsigned int skb_gso_network_seglen(const struct sk_buff *skb)
>   	return hdr_len + skb_gso_transport_seglen(skb);
>   }
>
> +/* Local Checksum Offload.
> + * Compute outer checksum based on the assumption that the
> + * inner checksum will be offloaded later.
> + * Fill in outer checksum adjustment (e.g. with sum of outer
> + * pseudo-header) before calling.
> + * Also ensure that inner checksum is in linear data area.
> + */
> +static inline __wsum lco_csum(struct sk_buff *skb)
> +{
> +	char *inner_csum_field;
> +	__wsum csum;
> +
> +	/* Start with complement of inner checksum adjustment */
> +	inner_csum_field = skb->data + skb_checksum_start_offset(skb) +
> +				skb->csum_offset;
> +	csum = ~csum_unfold(*(__force __sum16 *)inner_csum_field);
> +	/* Add in checksum of our headers (incl. outer checksum
> +	 * adjustment filled in by caller)
> +	 */
> +	csum = skb_checksum(skb, 0, skb_checksum_start_offset(skb), csum);
> +	/* The result is the checksum from skb->data to end of packet */
> +	return csum;
> +}
> +
>   #endif	/* __KERNEL__ */
>   #endif	/* _LINUX_SKBUFF_H */
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 8841e98..f9bd5db 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -767,16 +767,18 @@ void udp_set_csum(bool nocheck, struct sk_buff *skb,
>   {
>   	struct udphdr *uh = udp_hdr(skb);
>
> -	if (nocheck)
> +	if (nocheck) {
>   		uh->check = 0;
> -	else if (skb_is_gso(skb))
> +	} else if (skb_is_gso(skb)) {
>   		uh->check = ~udp_v4_check(len, saddr, daddr, 0);
> -	else if (skb_dst(skb) && skb_dst(skb)->dev &&
> -		 (skb_dst(skb)->dev->features &
> -		  (NETIF_F_IP_CSUM | NETIF_F_HW_CSUM))) {
> -
> -		BUG_ON(skb->ip_summed == CHECKSUM_PARTIAL);
> -
> +	} else if (skb->ip_summed == CHECKSUM_PARTIAL) {
> +		uh->check = 0;
> +		uh->check = ~udp_v4_check(len, saddr, daddr, lco_csum(skb));
> +		if (uh->check == 0)
> +			uh->check = CSUM_MANGLED_0;
> +	} else if (skb_dst(skb) && skb_dst(skb)->dev &&
> +		   (skb_dst(skb)->dev->features &
> +		    (NETIF_F_IP_CSUM | NETIF_F_HW_CSUM))) {
>   		skb->ip_summed = CHECKSUM_PARTIAL;
>   		skb->csum_start = skb_transport_header(skb) - skb->head;
>   		skb->csum_offset = offsetof(struct udphdr, check);
> @@ -784,8 +786,6 @@ void udp_set_csum(bool nocheck, struct sk_buff *skb,
>   	} else {
>   		__wsum csum;
>
> -		BUG_ON(skb->ip_summed == CHECKSUM_PARTIAL);
> -
>   		uh->check = 0;
>   		csum = skb_checksum(skb, 0, len, 0);
>   		uh->check = udp_v4_check(len, saddr, daddr, csum);
> diff --git a/net/ipv6/ip6_checksum.c b/net/ipv6/ip6_checksum.c
> index 9a4d732..eba525d 100644
> --- a/net/ipv6/ip6_checksum.c
> +++ b/net/ipv6/ip6_checksum.c
> @@ -98,11 +98,13 @@ void udp6_set_csum(bool nocheck, struct sk_buff *skb,
>   		uh->check = 0;
>   	else if (skb_is_gso(skb))
>   		uh->check = ~udp_v6_check(len, saddr, daddr, 0);
> -	else if (skb_dst(skb) && skb_dst(skb)->dev &&
> -		 (skb_dst(skb)->dev->features & NETIF_F_IPV6_CSUM)) {
> -
> -		BUG_ON(skb->ip_summed == CHECKSUM_PARTIAL);
> -
> +	else if (skb->ip_summed == CHECKSUM_PARTIAL) {
> +		uh->check = 0;
> +		uh->check = ~udp_v6_check(len, saddr, daddr, csum_fold(lco_csum(skb));
> +		if (uh->check == 0)
> +			uh->check = CSUM_MANGLED_0;
> +	} else if (skb_dst(skb) && skb_dst(skb)->dev &&
> +		   (skb_dst(skb)->dev->features & NETIF_F_IPV6_CSUM) {
>   		skb->ip_summed = CHECKSUM_PARTIAL;
>   		skb->csum_start = skb_transport_header(skb) - skb->head;
>   		skb->csum_offset = offsetof(struct udphdr, check);
> @@ -110,8 +112,6 @@ void udp6_set_csum(bool nocheck, struct sk_buff *skb,
>   	} else {
>   		__wsum csum;
>
> -		BUG_ON(skb->ip_summed == CHECKSUM_PARTIAL);
> -
>   		uh->check = 0;
>   		csum = skb_checksum(skb, 0, len, 0);
>   		uh->check = udp_v6_check(len, saddr, daddr, csum);
>
>

Re: [PATCH net-next 1/8] net: local checksum offload for encapsulation

[Alexander Duyck]

On Wed, Jan 27, 2016 at 11:04 PM, Zang MingJie <zealot0630@gmail.com> wrote:
> I have also noticed that for gso, all gso segs will have exactly same outer
> udp checksum, this is also because inner checksum cancellation.
>
> Can we also optimize that outer udp checksum should be only calculated once
> for all gso segs ?

Actually that is a good point.  It isn't as if anything really changes
in the tunnel headers between frames so we probably can just compute
the outer once and then adjust it on the last frame to account for the
fact that the length will be different.  I'll see if we can do that in
the GSO patches I have been working on.

- Alex

Re: [PATCH net-next 1/8] net: local checksum offload for encapsulation

[Tom Herbert]

On Thu, Jan 28, 2016 at 1:00 AM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Wed, Jan 27, 2016 at 11:04 PM, Zang MingJie <zealot0630@gmail.com> wrote:
>> I have also noticed that for gso, all gso segs will have exactly same outer
>> udp checksum, this is also because inner checksum cancellation.
>>
>> Can we also optimize that outer udp checksum should be only calculated once
>> for all gso segs ?
>
> Actually that is a good point.  It isn't as if anything really changes
> in the tunnel headers between frames so we probably can just compute

This is probably true for current all implementation, but it has never
been declared as a requirement. It's conceivable that some
encapsulation layer might implement something like a sequence number
or its own CRC. The requirements should at least be documented.

Tom

> the outer once and then adjust it on the last frame to account for the
> fact that the length will be different.  I'll see if we can do that in
> the GSO patches I have been working on.
>
> - Alex

[PATCH net-next 6/8] net: gre: Implement LCO for GRE over IPv4

[Edward Cree]

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 net/ipv4/ip_gre.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 7c51c4e..9b31532 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -440,6 +440,17 @@ drop:
 	return 0;
 }
 
+static __sum16 gre_checksum(struct sk_buff *skb)
+{
+	__wsum csum;
+
+	if (skb->ip_summed == CHECKSUM_PARTIAL)
+		csum = lco_csum(skb);
+	else
+		csum = skb_checksum(skb, 0, skb->len, 0);
+	return csum_fold(csum);
+}
+
 static void build_header(struct sk_buff *skb, int hdr_len, __be16 flags,
 			 __be16 proto, __be32 key, __be32 seq)
 {
@@ -467,8 +478,7 @@ static void build_header(struct sk_buff *skb, int hdr_len, __be16 flags,
 		    !(skb_shinfo(skb)->gso_type &
 		      (SKB_GSO_GRE | SKB_GSO_GRE_CSUM))) {
 			*ptr = 0;
-			*(__sum16 *)ptr = csum_fold(skb_checksum(skb, 0,
-								 skb->len, 0));
+			*(__sum16 *)ptr = gre_checksum(skb);
 		}
 	}
 }
@@ -493,7 +503,7 @@ static void __gre_xmit(struct sk_buff *skb, struct net_device *dev,
 static struct sk_buff *gre_handle_offloads(struct sk_buff *skb,
 					   bool csum)
 {
-	return iptunnel_handle_offloads(skb, csum,
+	return iptunnel_handle_offloads(skb, false,
 					csum ? SKB_GSO_GRE_CSUM : SKB_GSO_GRE);
 }