[ovs-dev] [PATCH 02/12] datapath: compat: Update iptunnel_pull_header

Jesse Gross jesse at kernel.org
Sat May 21 00:53:59 UTC 2016


On Wed, May 4, 2016 at 4:35 PM, Pravin B Shelar <pshelar at ovn.org> wrote:
> Following patch backports updated iptunnel pull function.
> Also brings in following upstream fix:
>
>     commit a09a4c8dd1ec7f830e1fb9e59eb72bddc965d168
>     Author: Jesse Gross <jesse at kernel.org>
>     Date:   Sat Mar 19 09:32:02 2016 -0700
>
>     tunnels: Remove encapsulation offloads on decap.
>
>     If a packet is either locally encapsulated or processed through GRO
>     it is marked with the offloads that it requires. However, when it is
>     decapsulated these tunnel offload indications are not removed. This
>     means that if we receive an encapsulated TCP packet, aggregate it with
>     GRO, decapsulate, and retransmit the resulting frame on a NIC that does
>     not support encapsulation, we won't be able to take advantage of hardware
>     offloads even though it is just a simple TCP packet at this point.
>
>     This fixes the problem by stripping off encapsulation offload indications
>     when packets are decapsulated.
>
>     The performance impacts of this bug are significant. In a test where a
>     Geneve encapsulated TCP stream is sent to a hypervisor, GRO'ed, decapsulated,
>     and bridged to a VM performance is improved by 60% (5Gbps->8Gbps) as a
>     result of avoiding unnecessary segmentation at the VM tap interface.
>
>     Reported-by: Ramu Ramamurthy <sramamur at linux.vnet.ibm.com>
>     Fixes: 68c33163 ("v4 GRE: Add TCP segmentation offload for GRE")
>     Signed-off-by: Jesse Gross <jesse at kernel.org>
>     Signed-off-by: David S. Miller <davem at davemloft.net>
>
> Signed-off-by: Pravin B Shelar <pshelar at ovn.org>

I think in order for this bug fix to be fully useful, we'll need to
bump up the range of kernels where we backport tunnel implementations
to < 4.7. That's kind of annoying but it's probably important since it
comes up in common OVS use cases.

> diff --git a/datapath/linux/compat/geneve.c b/datapath/linux/compat/geneve.c
> index f38d4a3..0f6bda0 100644
> --- a/datapath/linux/compat/geneve.c
> +++ b/datapath/linux/compat/geneve.c
> @@ -247,7 +247,7 @@ static int geneve_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
>
>         opts_len = geneveh->opt_len * 4;
>         if (iptunnel_pull_header(skb, GENEVE_BASE_HLEN + opts_len,
> -                                htons(ETH_P_TEB)))
> +                                htons(ETH_P_TEB), false))

Even though these devices are created by OVS, I think it's possible
that someone could move them to another namespace - the same as with
the upstream version.

>  static struct gre_cisco_protocol __rcu *gre_cisco_proto;
> diff --git a/datapath/linux/compat/include/linux/skbuff.h b/datapath/linux/compat/include/linux/skbuff.h
> index 376dfda..03ea707 100644
> --- a/datapath/linux/compat/include/linux/skbuff.h
> +++ b/datapath/linux/compat/include/linux/skbuff.h
> +#ifndef HAVE_SKB_CLEAR_HASH_IF_NOT_L4
> +static inline void skb_clear_hash_if_not_l4(struct sk_buff *skb)
> +{
> +       skb_clear_hash(skb);
> +}

3.10 actually has skb->l4_rxhash, so we should be able to implement
this fully even on kernels that don't otherwise support it.

> diff --git a/datapath/linux/compat/ip_tunnels_core.c b/datapath/linux/compat/ip_tunnels_core.c
> index 0858d02..2d91abb 100644
> --- a/datapath/linux/compat/ip_tunnels_core.c
> +++ b/datapath/linux/compat/ip_tunnels_core.c
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(4,7,0)
[...]
> +static inline int iptunnel_pull_offloads(struct sk_buff *skb)

Should this go in the header file to be consistent with upstream?



More information about the dev mailing list