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

On Mon, Jan 11, 2016 at 9:31 AM, Edward Cree [off-list ref] wrote:

On 11/01/16 16:39, Alexander Duyck wrote:

quoted

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

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>

quoted

+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