[ovs-dev] [PATCH 2/6] datapath: compat: Use checksum offload for outer header.

[pravin shelar]

On Wed, Aug 3, 2016 at 2:07 PM, Jesse Gross <jesse at kernel.org> wrote:
> On Wed, Aug 3, 2016 at 9:00 AM, pravin shelar <pshelar at ovn.org> wrote:
>> On Tue, Aug 2, 2016 at 4:55 PM, Jesse Gross <jesse at kernel.org> wrote:
>>> On Tue, Aug 2, 2016 at 3:55 PM, pravin shelar <pshelar at ovn.org> wrote:
>>>> On Tue, Aug 2, 2016 at 3:11 PM, Jesse Gross <jesse at kernel.org> wrote:
>>>>> On Tue, Aug 2, 2016 at 2:17 PM, Pravin B Shelar <pshelar at ovn.org> wrote:
>>>>>> Signed-off-by: Pravin B Shelar <pshelar at 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 <jesse at 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.