Date:	Mon, 11 Jan 2016 13:00:05 -0800
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	Edward Cree <ecree@...arflare.com>
Cc:	Tom Herbert <tom@...bertland.com>,
	David Miller <davem@...emloft.net>,
	Netdev <netdev@...r.kernel.org>, linux-net-drivers@...arflare.com
Subject: Re: [PATCH net-next 7/8] net: ip_tunnel: remove 'csum_help' argument
 to iptunnel_handle_offloads

On Mon, Jan 11, 2016 at 11:03 AM, Edward Cree <ecree@...arflare.com> wrote:
> On 11/01/16 18:15, Alexander Duyck wrote:
>> On Mon, Jan 11, 2016 at 9:31 AM, Edward Cree <ecree@...arflare.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

Powered by blists - more mailing lists