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

[Chandran, Sugesh]

Hi Cascardo,

Thank you for reviewing the patch. 

I had sent out the new patch after incorporating all your comments, please have a look on it and let me know your inputs/suggestions/comments.

I verified that the performance drop is fixed by the patch and also I noticed there is a 10% improvement on ipv4 tunneling performance. I haven't tested any other functionalities such as sflow and areas where the tunnel metadata validation has been changed. 

Please note that the patch uses the newly introduced flag for not only tunnel metadata validation, but also to store the tunnel type(ipv4/ipv6) now. 

The wrapper functions are introduced to set the destination address and flag.  All the ipv6/ipv4 tunnel destination address assignments are now uses these wrappers to assign addresses.

Regards
_Sugesh

-----Original Message-----
From: Thadeu Lima de Souza Cascardo [mailto:cascardo at redhat.com] 
Sent: Friday, December 18, 2015 8:12 PM
To: Chandran, Sugesh <sugesh.chandran at intel.com>
Cc: dev at openvswitch.org; jbenc at redhat.com
Subject: Re: [RFC] ipv6 tunneling: Fix for performance drop introduced by ipv6 tunnel support.

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
>