On Wed, Aug 3, 2016 at 2:07 PM, Jesse Gross <je...@kernel.org> wrote: > On Wed, Aug 3, 2016 at 9:00 AM, pravin shelar <pshe...@ovn.org> wrote: >> On Tue, Aug 2, 2016 at 4:55 PM, Jesse Gross <je...@kernel.org> wrote: >>> On Tue, Aug 2, 2016 at 3:55 PM, pravin shelar <pshe...@ovn.org> wrote: >>>> On Tue, Aug 2, 2016 at 3:11 PM, Jesse Gross <je...@kernel.org> wrote: >>>>> On Tue, Aug 2, 2016 at 2:17 PM, Pravin B Shelar <pshe...@ovn.org> wrote: >>>>>> Signed-off-by: Pravin B Shelar <pshe...@ovn.org> >>>>>> --- >>>>>> datapath/linux/compat/include/net/udp.h | 2 +- >>>>>> datapath/linux/compat/udp.c | 19 ++----------------- >>>>>> datapath/linux/compat/udp_tunnel.c | 17 +---------------- >>>>>> 3 files changed, 4 insertions(+), 34 deletions(-) >>>>> >>>>> Can you give some more information on this? Is it a bug fix, >>>>> performance improvement, or cleanup? >>>> >>>> How about following commit msg. >>>> Following patch simplifies UDP-checksum routine by unconditionally >>>> using checksum offload for non GSO packets. We might get some >>>> performance improvement due to code simplification. >>> >>> This is just the check for whether the device has the >>> NETIF_F_V[46]_CSUM feature? I wonder whether that is really enough of >>> a benefit to deviate from upstream. If it is noticeable then it seems >>> like this should be an upstream patch. >>> >> >> In UDP xmit dst_entry is set after calling this function, so for non >> goo packets this function always calculate checksum in software. > > OK, I understand now. That's definitely a much bigger deal. I also see > that the upstream version of the function has already been changed, so > this is only an issue for backports. Can you add this information ot > the commit message as well? With that change: > Acked-by: Jesse Gross <je...@kernel.org> > > At this point, the main difference between this function and the > current upstream one is the lack of LCO. Currently, this isn't a > regression since LCO was introduced in 4.6 and that's where we start > using upstream tunnels. However, I'm a bit concerned that we might > need to continue to bump up the kernel version where we use our own > tunnels in the future and we will lose this optimization. (Though > maybe things have stabilized upstream at this point, which would be > nice.) > > Independent of the possible future regression, it seems like we could > backport LCO as an optimization. On kernels with > USE_UPSTREAM_TUNNEL_GSO and are using outer checksums, we could delay > and possibly avoid computing the inner checksum. > right, I am planing on working on LCO. But first I want to work on receive side and fix any GRO related issues.
>>> I think the biggest checksum related improvement for compat code would >>> be to make the checksum computation conditional on skb->encapsulation >>> being set in rpl_ip_local_out. >> >> I am not sure how that will help, In rpl_ip_local_out() GSO packet we >> can not generate checksum partial packets, udp_set_csum() can not >> handle it. non GSO packet's checksum is calculated in >> ovs_iptunnel_handle_offloads() and the skb is not touched in >> rpl_ip_local_out(). > > I'm not sure if it was clear but I was talking about outer UDP > checksums on kernels where we don't have USE_UPSTREAM_TUNNEL_GSO. > > In the GSO case, it looks to me like we are already generating > checksum partial packets. tnl_skb_gso_segment() will first resolve the > inner checksum and then call ovs_udp_csum_gso(), which calls > udp_set_csum() to create a new checksum partial outer header. All of > that seems OK to me. > > For the non-GSO case, I agree that we are currently allowing all > packets to be computed by the stack since fix_segment will always be > NULL in this case. However, as we talked about in relation to one of > the previous patches, this might not be safe on older kernels where > drivers make assumptions about encapsulated packets being VXLAN. If we > were to change this so that we computed checksum for packets with > skb->encapsulation set, then we should still allow packets to be > offloaded without that set. ok, I will sort out this issue. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
