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. >> 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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
