| Message ID | CAKgT0Ufw9djq-V8YAXjq21gg7rE5=-Dp4o4GmdGk+6Diu4mDNw@mail.gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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) {