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

[Chandran, Sugesh]

Hi Cascardo,
Thank you for the quick review. 
I verified the throughput is improved by 25% by the given patch. There is a slight tunneling performance improvement also noticed after applying it.
This patch reverts the throughput drop introduced by ipv6 tunneling support. However I haven't tested ipv6 tunneling with the patch.

I will work on the patch to submit on the mailing-list if this approach fine for everyone.



Regards
_Sugesh


-----Original Message-----
From: Thadeu Lima de Souza Cascardo [mailto:cascardo at redhat.com] 
Sent: Tuesday, December 15, 2015 12:20 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.

This looks fine to me. Have you tested it? What is the drop in throughput when you use this patch, compared to before the IPv6 tunneling support was added?

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
>