Local Checksum Offload

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 <[email protected]>

---
  include/linux/skbuff.h  | 24 ++++++++++++++++++++++++
  net/ipv4/udp.c          | 20 ++++++++++----------
  net/ipv6/ip6_checksum.c | 14 +++++++-------
  3 files changed, 41 insertions(+), 17 deletions(-)

include/linux/skbuff.h +24 -0

index 6b6bd42..ee063ff 100644
@@ -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 */

net/ipv4/udp.c +10 -10

index 8841e98..f9bd5db 100644
@@ -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);

net/ipv6/ip6_checksum.c +7 -7

index 9a4d732..eba525d 100644
@@ -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);

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 ?

Quoted 5 lines · 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 ?

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

Sent: 08 January 2016 19:47

From: Edward Cree

Quoted 1 line

Sent: 08 January 2016 19:47

...

net/ipv4/ip_gre.c drop: · line 454

Quoted 8 lines

net/ipv4/ip_gre.c +8 -0

+	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.

Quoted 11 lines · David Laight

From: Edward Cree

Quoted 1 line

Sent: 08 January 2016 19:47

...

Quoted 8 lines

net/ipv4/ip_gre.c +8 -0

+    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));
+    }

Quoted 4 lines

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.

Quoted 3 lines · Edward Cree

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.

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.

Quoted 4 lines · Alexander Duyck

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.

Quoted 8 lines · Mark D

Quoted 4 lines · Alexander Duyck

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.

Quoted 8 lines

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

Quoted 19 lines · Alexander Duyck

Quoted 8 lines · Mark D

Quoted 4 lines · Alexander Duyck

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.

Quoted 8 lines

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.

Quoted 8 lines

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.

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

Signed-off-by Edward Cree <[email protected]>

---
 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(-)

net/ipv4/ip_tunnel_core.c +0 -7

index 1db8418..f98bd53 100644
@@ -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)

Quoted 47 lines · 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 <[email protected]>

---
 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(-)

net/ipv4/ip_tunnel_core.c +0 -14

index 1db8418..f98bd53 100644
@@ -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)

net/ipv4/ip_tunnel_core.c struct sk_buff *iptunnel_handle_offloads(struct sk_buff *skb, · line 169

Quoted 3 lines

net/ipv4/ip_tunnel_core.c +1 -0

+       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

Quoted 48 lines · Alexander Duyck

Quoted 47 lines · 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 <[email protected]>

---
 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(-)

net/ipv4/ip_tunnel_core.c +0 -14

index 1db8418..f98bd53 100644
@@ -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)

Quoted 9 lines

net/ipv4/ip_tunnel_core.c struct sk_buff *iptunnel_handle_offloads(struct sk_buff *skb, · line 169

Quoted 3 lines

net/ipv4/ip_tunnel_core.c +1 -0

+       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?

Quoted 59 lines · Tom Herbert

Quoted 48 lines · Alexander Duyck

Quoted 47 lines · 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 <[email protected]>

---
 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(-)

net/ipv4/ip_tunnel_core.c +0 -14

index 1db8418..f98bd53 100644
@@ -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)

Quoted 9 lines

net/ipv4/ip_tunnel_core.c struct sk_buff *iptunnel_handle_offloads(struct sk_buff *skb, · line 169

Quoted 3 lines

net/ipv4/ip_tunnel_core.c +1 -0

+       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.

Quoted 4 lines

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

Quoted 73 lines · Alexander Duyck

Quoted 59 lines · Tom Herbert

Quoted 48 lines · Alexander Duyck

Quoted 47 lines · 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 <[email protected]>

---
 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(-)

net/ipv4/ip_tunnel_core.c +0 -14

index 1db8418..f98bd53 100644
@@ -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)

Quoted 9 lines

net/ipv4/ip_tunnel_core.c struct sk_buff *iptunnel_handle_offloads(struct sk_buff *skb, · line 169

Quoted 3 lines

net/ipv4/ip_tunnel_core.c +1 -0

+       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.

Quoted 4 lines

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.

Quoted 6 lines

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.

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.

Quoted 5 lines · Alexander Duyck

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[...] 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.

Quoted 2 lines · Alexander Duyck

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

Will do.

Quoted 4 lines

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>

Review

Quoted 11 lines

(quoted diff) +11 -0

+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.

Quoted 4 lines · Edward Cree

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.

Quoted 3 lines

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.

Quoted 10 lines · Alexander Duyck

Quoted 4 lines · Edward Cree

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.

Quoted 8 lines

Quoted 3 lines

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.

From what I can tell this is sort-of true. It is offloaded in that

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?