[ovs-dev] [RFC] ipv6 tunneling: Fix for performance drop introduced by ipv6 tunnel support.

Thadeu Lima de Souza Cascardo cascardo at redhat.com
Fri Dec 18 20:12:10 UTC 2015


On Mon, Dec 14, 2015 at 11:06:40PM +0000, Sugesh Chandran wrote:
> Adding a new flag to validate if the tunnel metadata is valid/not. This flag avoids
> the need of resetting and validating the entire ipv4/ipv6 tunnel destination
> address which caused a serious performance drop.
> 
> Signed-off-by: Sugesh Chandran <sugesh.chandran at intel.com>

Hi, Sugesh.

You missed some spots. Tunnel destination is also set at
ofproto/tunnel.c:tnl_port_send. At lib/packets.h:flow_tnl_size, I suggest you
optimize it to not copy any of the destinations if !flow_tnl_dst_is_set. That
is, change the first offset to ip_dst instead of ip_src.

Then, you need to fix other cases where dst might be used without checking for
flow_tnl_dst_is_set. By the way, it might be a good thing to just rename it to
flow_tnl_is_valid.

These other cases include ofproto/ofproto-dpif-ipfix.c and
ofproto/ofproto-dpif-sflow.c. Notice these only support IPv4 as of now.

Also, if we are not initializing the addresses, you might as well check for
is_tnl_valid at lib/packets.c:flow_tnl_dst.

I am not sure, but I suspect we might screw up at lib/meta-flow.c:mf_get_value.
Same for lib/flow.c:flow_get_metadata.

match_set_tun_ipv6_dst_masked was not updated.

By the way, how do you tell if the tunnel is IPv4 or IPv6? Since we are wasting
a lot of bits with this single boolean, why not do what the original patch did,
and use it the bits for protocol as well? It could get really messy with a lot
of identation around the code. But I guess that we almost everything using IPv6,
you could just hide it behind flow_tnl_dst, maybe use a wrapper for setting up
IPv4 and IPv6.

Thanks.
Cascardo.

> ---
>  lib/match.c        | 2 ++
>  lib/netdev-vport.c | 3 ++-
>  lib/packets.h      | 8 ++++----
>  3 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/match.c b/lib/match.c
> index 95d34bc..79a561d 100644
> --- a/lib/match.c
> +++ b/lib/match.c
> @@ -188,6 +188,7 @@ match_set_tun_dst_masked(struct match *match, ovs_be32 dst, ovs_be32 mask)
>  {
>      match->wc.masks.tunnel.ip_dst = mask;
>      match->flow.tunnel.ip_dst = dst & mask;
> +    match->flow.tunnel.is_tnl_valid = 1;
>  }
>  
>  void
> @@ -210,6 +211,7 @@ match_set_tun_ipv6_dst(struct match *match, const struct in6_addr *dst)
>  {
>      match->flow.tunnel.ipv6_dst = *dst;
>      match->wc.masks.tunnel.ipv6_dst = in6addr_exact;
> +    match->flow.tunnel.is_tnl_valid = 1;
>  }
>  
>  void
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 88f5022..1cf37d7 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -920,6 +920,7 @@ ip_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl,
>  
>          tnl->ip_src = ip_src;
>          tnl->ip_dst = ip_dst;
> +        tnl->is_tnl_valid = 1;
>          tnl->ip_tos = ip->ip_tos;
>          tnl->ip_ttl = ip->ip_ttl;
>  
> @@ -931,7 +932,7 @@ ip_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl,
>          memcpy(tnl->ipv6_dst.s6_addr, ip6->ip6_dst.be16, sizeof ip6->ip6_dst);
>          tnl->ip_tos = 0;
>          tnl->ip_ttl = ip6->ip6_hlim;
> -
> +        tnl->is_tnl_valid = 1;
>          *hlen += IPV6_HEADER_LEN;
>  
>      } else {
> diff --git a/lib/packets.h b/lib/packets.h
> index edf140b..06c1e19 100644
> --- a/lib/packets.h
> +++ b/lib/packets.h
> @@ -37,6 +37,7 @@ struct ds;
>  
>  /* Tunnel information used in flow key and metadata. */
>  struct flow_tnl {
> +    bool is_tnl_valid;
>      ovs_be32 ip_dst;
>      struct in6_addr ipv6_dst;
>      ovs_be32 ip_src;
> @@ -49,7 +50,7 @@ struct flow_tnl {
>      ovs_be16 tp_dst;
>      ovs_be16 gbp_id;
>      uint8_t  gbp_flags;
> -    uint8_t  pad1[5];        /* Pad to 64 bits. */
> +    uint8_t  pad1[1];        /* Pad to 64 bits. */
>      struct tun_metadata metadata;
>  };
>  
> @@ -79,7 +80,7 @@ static inline bool ipv6_addr_is_set(const struct in6_addr *addr);
>  static inline bool
>  flow_tnl_dst_is_set(const struct flow_tnl *tnl)
>  {
> -    return tnl->ip_dst || ipv6_addr_is_set(&tnl->ipv6_dst);
> +    return tnl->is_tnl_valid;
>  }
>  
>  struct in6_addr flow_tnl_dst(const struct flow_tnl *tnl);
> @@ -157,8 +158,7 @@ pkt_metadata_init(struct pkt_metadata *md, odp_port_t port)
>       * we can just zero out ip_dst and the rest of the data will never be
>       * looked at. */
>      memset(md, 0, offsetof(struct pkt_metadata, in_port));
> -    md->tunnel.ip_dst = 0;
> -    md->tunnel.ipv6_dst = in6addr_any;
> +    md->tunnel.is_tnl_valid = 0;
>  
>      md->in_port.odp_port = port;
>  }
> -- 
> 1.9.1
> 



More information about the dev mailing list