[PATCH net-next v3 0/7] vxlan: xmit improvements.

[PATCH net-next v3 0/7] vxlan: xmit improvements.

[Pravin B Shelar]

Following patch series improves vxlan fast path, removes
duplicate code and simplifies vxlan xmit code path.

v2-v3:
Removed unrelated warning fix from patch 2.
rearranged error handling from patch 3
Fixed stats updates in vxlan route lookup in patch 4

v1-v2:
Fix compilation error when IPv6 support is not enabled.


Pravin B Shelar (7):
  vxlan: avoid vlan processing in vxlan device.
  vxlan: avoid checking socket multiple times.
  vxlan: simplify exception handling
  vxlan: improve vxlan route lookup checks.
  vxlan: simplify RTF_LOCAL handling.
  vxlan: simplify vxlan xmit
  vxlan: remove unsed vxlan_dev_dst_port()

 drivers/net/vxlan.c     | 285 +++++++++++++++++++++++-------------------------
 include/linux/if_vlan.h |  16 ---
 include/net/vxlan.h     |  10 --
 3 files changed, 137 insertions(+), 174 deletions(-)

-- 
1.9.1

[PATCH net-next v3 1/7] vxlan: avoid vlan processing in vxlan device.

[Pravin B Shelar]

VxLan device does not have special handling for vlan taging on egress.
Therefore it does not make sense to expose vlan offloading feature.
This patch does not change vxlan functinality.

Signed-off-by: Pravin B Shelar <pshelar@ovn.org>
Acked-by: Jiri Benc <jbenc@redhat.com>
---
 drivers/net/vxlan.c     |  9 +--------
 include/linux/if_vlan.h | 16 ----------------
 2 files changed, 1 insertion(+), 24 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index cb5cc7c..756d826 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1748,18 +1748,13 @@ static int vxlan_build_skb(struct sk_buff *skb, struct dst_entry *dst,
 	}
 
 	min_headroom = LL_RESERVED_SPACE(dst->dev) + dst->header_len
-			+ VXLAN_HLEN + iphdr_len
-			+ (skb_vlan_tag_present(skb) ? VLAN_HLEN : 0);
+			+ VXLAN_HLEN + iphdr_len;
 
 	/* Need space for new headers (invalidates iph ptr) */
 	err = skb_cow_head(skb, min_headroom);
 	if (unlikely(err))
 		goto out_free;
 
-	skb = vlan_hwaccel_push_inside(skb);
-	if (WARN_ON(!skb))
-		return -ENOMEM;
-
 	err = iptunnel_handle_offloads(skb, type);
 	if (err)
 		goto out_free;
@@ -2527,10 +2522,8 @@ static void vxlan_setup(struct net_device *dev)
 	dev->features   |= NETIF_F_GSO_SOFTWARE;
 
 	dev->vlan_features = dev->features;
-	dev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;
 	dev->hw_features |= NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
 	dev->hw_features |= NETIF_F_GSO_SOFTWARE;
-	dev->hw_features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;
 	netif_keep_dst(dev);
 	dev->priv_flags |= IFF_NO_QUEUE;
 
diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 3319d97..8d5fcd6 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -399,22 +399,6 @@ static inline struct sk_buff *__vlan_hwaccel_push_inside(struct sk_buff *skb)
 		skb->vlan_tci = 0;
 	return skb;
 }
-/*
- * vlan_hwaccel_push_inside - pushes vlan tag to the payload
- * @skb: skbuff to tag
- *
- * Checks is tag is present in @skb->vlan_tci and if it is, it pushes the
- * VLAN tag from @skb->vlan_tci inside to the payload.
- *
- * Following the skb_unshare() example, in case of error, the calling function
- * doesn't have to worry about freeing the original skb.
- */
-static inline struct sk_buff *vlan_hwaccel_push_inside(struct sk_buff *skb)
-{
-	if (skb_vlan_tag_present(skb))
-		skb = __vlan_hwaccel_push_inside(skb);
-	return skb;
-}
 
 /**
  * __vlan_hwaccel_put_tag - hardware accelerated VLAN inserting
-- 
1.9.1

[PATCH net-next v3 2/7] vxlan: avoid checking socket multiple times.

[Pravin B Shelar]

Check the vxlan socket in vxlan6_getroute().

Signed-off-by: Pravin B Shelar <pshelar@ovn.org>
---
 drivers/net/vxlan.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 756d826..9adeff9 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1830,6 +1830,7 @@ static struct rtable *vxlan_get_route(struct vxlan_dev *vxlan,
 
 #if IS_ENABLED(CONFIG_IPV6)
 static struct dst_entry *vxlan6_get_route(struct vxlan_dev *vxlan,
+					  struct vxlan_sock *sock6,
 					  struct sk_buff *skb, int oif, u8 tos,
 					  __be32 label,
 					  const struct in6_addr *daddr,
@@ -1837,7 +1838,6 @@ static struct dst_entry *vxlan6_get_route(struct vxlan_dev *vxlan,
 					  struct dst_cache *dst_cache,
 					  const struct ip_tunnel_info *info)
 {
-	struct vxlan_sock *sock6 = rcu_dereference(vxlan->vn6_sock);
 	bool use_cache = ip_tunnel_dst_cache_usable(skb, info);
 	struct dst_entry *ndst;
 	struct flowi6 fl6;
@@ -2069,11 +2069,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		struct dst_entry *ndst;
 		u32 rt6i_flags;
 
-		if (!sock6)
-			goto drop;
-		sk = sock6->sock->sk;
-
-		ndst = vxlan6_get_route(vxlan, skb,
+		ndst = vxlan6_get_route(vxlan, sock6, skb,
 					rdst ? rdst->remote_ifindex : 0, tos,
 					label, &dst->sin6.sin6_addr,
 					&src->sin6.sin6_addr,
@@ -2093,6 +2089,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 			goto tx_error;
 		}
 
+		sk = sock6->sock->sk;
 		/* Bypass encapsulation if the destination is local */
 		rt6i_flags = ((struct rt6_info *)ndst)->rt6i_flags;
 		if (!info && rt6i_flags & RTF_LOCAL &&
@@ -2432,9 +2429,10 @@ static int vxlan_fill_metadata_dst(struct net_device *dev, struct sk_buff *skb)
 		ip_rt_put(rt);
 	} else {
 #if IS_ENABLED(CONFIG_IPV6)
+		struct vxlan_sock *sock6 = rcu_dereference(vxlan->vn6_sock);
 		struct dst_entry *ndst;
 
-		ndst = vxlan6_get_route(vxlan, skb, 0, info->key.tos,
+		ndst = vxlan6_get_route(vxlan, sock6, skb, 0, info->key.tos,
 					info->key.label, &info->key.u.ipv6.dst,
 					&info->key.u.ipv6.src, NULL, info);
 		if (IS_ERR(ndst))
-- 
1.9.1

[PATCH net-next v3 3/7] vxlan: simplify exception handling

[Pravin B Shelar]

vxlan egress path error handling has became complicated, it
need to handle IPv4 and IPv6 tunnel cases.
Earlier patch removes vlan handling from vxlan_build_skb(), so
vxlan_build_skb does not need to free skb and we can simplify
the xmit path by having single error handling for both type of
tunnels.

Signed-off-by: Pravin B Shelar <pshelar@ovn.org>
---
 drivers/net/vxlan.c | 46 +++++++++++++++++++---------------------------
 1 file changed, 19 insertions(+), 27 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 9adeff9..8bb58f6 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1753,11 +1753,11 @@ static int vxlan_build_skb(struct sk_buff *skb, struct dst_entry *dst,
 	/* Need space for new headers (invalidates iph ptr) */
 	err = skb_cow_head(skb, min_headroom);
 	if (unlikely(err))
-		goto out_free;
+		return err;
 
 	err = iptunnel_handle_offloads(skb, type);
 	if (err)
-		goto out_free;
+		return err;
 
 	vxh = (struct vxlanhdr *) __skb_push(skb, sizeof(*vxh));
 	vxh->vx_flags = VXLAN_HF_VNI;
@@ -1781,16 +1781,12 @@ static int vxlan_build_skb(struct sk_buff *skb, struct dst_entry *dst,
 	if (vxflags & VXLAN_F_GPE) {
 		err = vxlan_build_gpe_hdr(vxh, vxflags, skb->protocol);
 		if (err < 0)
-			goto out_free;
+			return err;
 		inner_protocol = skb->protocol;
 	}
 
 	skb_set_inner_protocol(skb, inner_protocol);
 	return 0;
-
-out_free:
-	kfree_skb(skb);
-	return err;
 }
 
 static struct rtable *vxlan_get_route(struct vxlan_dev *vxlan,
@@ -1927,13 +1923,13 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 	struct ip_tunnel_info *info;
 	struct vxlan_dev *vxlan = netdev_priv(dev);
 	struct sock *sk;
-	struct rtable *rt = NULL;
 	const struct iphdr *old_iph;
 	union vxlan_addr *dst;
 	union vxlan_addr remote_ip, local_ip;
 	union vxlan_addr *src;
 	struct vxlan_metadata _md;
 	struct vxlan_metadata *md = &_md;
+	struct dst_entry *ndst = NULL;
 	__be16 src_port = 0, dst_port;
 	__be32 vni, label;
 	__be16 df = 0;
@@ -2009,6 +2005,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 
 	if (dst->sa.sa_family == AF_INET) {
 		struct vxlan_sock *sock4 = rcu_dereference(vxlan->vn4_sock);
+		struct rtable *rt;
 
 		if (!sock4)
 			goto drop;
@@ -2030,7 +2027,8 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 			netdev_dbg(dev, "circular route to %pI4\n",
 				   &dst->sin.sin_addr.s_addr);
 			dev->stats.collisions++;
-			goto rt_tx_error;
+			ip_rt_put(rt);
+			goto tx_error;
 		}
 
 		/* Bypass encapsulation if the destination is local */
@@ -2053,12 +2051,13 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		else if (info->key.tun_flags & TUNNEL_DONT_FRAGMENT)
 			df = htons(IP_DF);
 
+		ndst = &rt->dst;
 		tos = ip_tunnel_ecn_encap(tos, old_iph, skb);
 		ttl = ttl ? : ip4_dst_hoplimit(&rt->dst);
-		err = vxlan_build_skb(skb, &rt->dst, sizeof(struct iphdr),
+		err = vxlan_build_skb(skb, ndst, sizeof(struct iphdr),
 				      vni, md, flags, udp_sum);
 		if (err < 0)
-			goto xmit_tx_error;
+			goto tx_error;
 
 		udp_tunnel_xmit_skb(rt, sk, skb, src->sin.sin_addr.s_addr,
 				    dst->sin.sin_addr.s_addr, tos, ttl, df,
@@ -2066,7 +2065,6 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 #if IS_ENABLED(CONFIG_IPV6)
 	} else {
 		struct vxlan_sock *sock6 = rcu_dereference(vxlan->vn6_sock);
-		struct dst_entry *ndst;
 		u32 rt6i_flags;
 
 		ndst = vxlan6_get_route(vxlan, sock6, skb,
@@ -2078,13 +2076,13 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 			netdev_dbg(dev, "no route to %pI6\n",
 				   &dst->sin6.sin6_addr);
 			dev->stats.tx_carrier_errors++;
+			ndst = NULL;
 			goto tx_error;
 		}
 
 		if (ndst->dev == dev) {
 			netdev_dbg(dev, "circular route to %pI6\n",
 				   &dst->sin6.sin6_addr);
-			dst_release(ndst);
 			dev->stats.collisions++;
 			goto tx_error;
 		}
@@ -2096,12 +2094,12 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		    !(rt6i_flags & (RTCF_BROADCAST | RTCF_MULTICAST))) {
 			struct vxlan_dev *dst_vxlan;
 
-			dst_release(ndst);
 			dst_vxlan = vxlan_find_vni(vxlan->net, vni,
 						   dst->sa.sa_family, dst_port,
 						   vxlan->flags);
 			if (!dst_vxlan)
 				goto tx_error;
+			dst_release(ndst);
 			vxlan_encap_bypass(skb, vxlan, dst_vxlan);
 			return;
 		}
@@ -2114,11 +2112,9 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		skb_scrub_packet(skb, xnet);
 		err = vxlan_build_skb(skb, ndst, sizeof(struct ipv6hdr),
 				      vni, md, flags, udp_sum);
-		if (err < 0) {
-			dst_release(ndst);
-			dev->stats.tx_errors++;
-			return;
-		}
+		if (err < 0)
+			goto tx_error;
+
 		udp_tunnel6_xmit_skb(ndst, sk, skb, dev,
 				     &src->sin6.sin6_addr,
 				     &dst->sin6.sin6_addr, tos, ttl,
@@ -2130,17 +2126,13 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 
 drop:
 	dev->stats.tx_dropped++;
-	goto tx_free;
+	dev_kfree_skb(skb);
+	return;
 
-xmit_tx_error:
-	/* skb is already freed. */
-	skb = NULL;
-rt_tx_error:
-	ip_rt_put(rt);
 tx_error:
+	dst_release(ndst);
 	dev->stats.tx_errors++;
-tx_free:
-	dev_kfree_skb(skb);
+	kfree_skb(skb);
 }
 
 /* Transmit local packets over Vxlan
-- 
1.9.1

[PATCH net-next v3 4/7] vxlan: improve vxlan route lookup checks.

[Pravin B Shelar]

Move route sanity check to respective vxlan[4/6]_get_route functions.
This allows us to perform all sanity checks before caching the dst so
that we can avoid these checks on subsequent packets.
This give move accurate metadata information for packet from
fill_metadata_dst().

Signed-off-by: Pravin B Shelar <pshelar@ovn.org>
---
 drivers/net/vxlan.c | 77 ++++++++++++++++++++++++++---------------------------
 1 file changed, 38 insertions(+), 39 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 8bb58f6..aabb918 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1789,7 +1789,8 @@ static int vxlan_build_skb(struct sk_buff *skb, struct dst_entry *dst,
 	return 0;
 }
 
-static struct rtable *vxlan_get_route(struct vxlan_dev *vxlan,
+static struct rtable *vxlan_get_route(struct vxlan_dev *vxlan, struct net_device *dev,
+				      struct vxlan_sock *sock4,
 				      struct sk_buff *skb, int oif, u8 tos,
 				      __be32 daddr, __be32 *saddr,
 				      struct dst_cache *dst_cache,
@@ -1799,6 +1800,9 @@ static struct rtable *vxlan_get_route(struct vxlan_dev *vxlan,
 	struct rtable *rt = NULL;
 	struct flowi4 fl4;
 
+	if (!sock4)
+		return ERR_PTR(-EIO);
+
 	if (tos && !info)
 		use_cache = false;
 	if (use_cache) {
@@ -1816,16 +1820,26 @@ static struct rtable *vxlan_get_route(struct vxlan_dev *vxlan,
 	fl4.saddr = *saddr;
 
 	rt = ip_route_output_key(vxlan->net, &fl4);
-	if (!IS_ERR(rt)) {
+	if (likely(!IS_ERR(rt))) {
+		if (rt->dst.dev == dev) {
+			netdev_dbg(dev, "circular route to %pI4\n", &daddr);
+			ip_rt_put(rt);
+			return ERR_PTR(-ELOOP);
+		}
+
 		*saddr = fl4.saddr;
 		if (use_cache)
 			dst_cache_set_ip4(dst_cache, &rt->dst, fl4.saddr);
+	} else {
+		netdev_dbg(dev, "no route to %pI4\n", &daddr);
+		return ERR_PTR(-ENETUNREACH);
 	}
 	return rt;
 }
 
 #if IS_ENABLED(CONFIG_IPV6)
 static struct dst_entry *vxlan6_get_route(struct vxlan_dev *vxlan,
+					  struct net_device *dev,
 					  struct vxlan_sock *sock6,
 					  struct sk_buff *skb, int oif, u8 tos,
 					  __be32 label,
@@ -1861,8 +1875,16 @@ static struct dst_entry *vxlan6_get_route(struct vxlan_dev *vxlan,
 	err = ipv6_stub->ipv6_dst_lookup(vxlan->net,
 					 sock6->sock->sk,
 					 &ndst, &fl6);
-	if (err < 0)
-		return ERR_PTR(err);
+	if (unlikely(err < 0)) {
+		netdev_dbg(dev, "no route to %pI6\n", daddr);
+		return ERR_PTR(-ENETUNREACH);
+	}
+
+	if (unlikely(ndst->dev == dev)) {
+		netdev_dbg(dev, "circular route to %pI6\n", daddr);
+		dst_release(ndst);
+		return ERR_PTR(-ELOOP);
+	}
 
 	*saddr = fl6.saddr;
 	if (use_cache)
@@ -1929,8 +1951,8 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 	union vxlan_addr *src;
 	struct vxlan_metadata _md;
 	struct vxlan_metadata *md = &_md;
-	struct dst_entry *ndst = NULL;
 	__be16 src_port = 0, dst_port;
+	struct dst_entry *ndst = NULL;
 	__be32 vni, label;
 	__be16 df = 0;
 	__u8 tos, ttl;
@@ -2007,29 +2029,14 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		struct vxlan_sock *sock4 = rcu_dereference(vxlan->vn4_sock);
 		struct rtable *rt;
 
-		if (!sock4)
-			goto drop;
-		sk = sock4->sock->sk;
-
-		rt = vxlan_get_route(vxlan, skb,
+		rt = vxlan_get_route(vxlan, dev, sock4, skb,
 				     rdst ? rdst->remote_ifindex : 0, tos,
 				     dst->sin.sin_addr.s_addr,
 				     &src->sin.sin_addr.s_addr,
 				     dst_cache, info);
-		if (IS_ERR(rt)) {
-			netdev_dbg(dev, "no route to %pI4\n",
-				   &dst->sin.sin_addr.s_addr);
-			dev->stats.tx_carrier_errors++;
-			goto tx_error;
-		}
-
-		if (rt->dst.dev == dev) {
-			netdev_dbg(dev, "circular route to %pI4\n",
-				   &dst->sin.sin_addr.s_addr);
-			dev->stats.collisions++;
-			ip_rt_put(rt);
+		if (IS_ERR(rt))
 			goto tx_error;
-		}
+		sk = sock4->sock->sk;
 
 		/* Bypass encapsulation if the destination is local */
 		if (!info && rt->rt_flags & RTCF_LOCAL &&
@@ -2067,27 +2074,17 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		struct vxlan_sock *sock6 = rcu_dereference(vxlan->vn6_sock);
 		u32 rt6i_flags;
 
-		ndst = vxlan6_get_route(vxlan, sock6, skb,
+		ndst = vxlan6_get_route(vxlan, dev, sock6, skb,
 					rdst ? rdst->remote_ifindex : 0, tos,
 					label, &dst->sin6.sin6_addr,
 					&src->sin6.sin6_addr,
 					dst_cache, info);
 		if (IS_ERR(ndst)) {
-			netdev_dbg(dev, "no route to %pI6\n",
-				   &dst->sin6.sin6_addr);
-			dev->stats.tx_carrier_errors++;
 			ndst = NULL;
 			goto tx_error;
 		}
-
-		if (ndst->dev == dev) {
-			netdev_dbg(dev, "circular route to %pI6\n",
-				   &dst->sin6.sin6_addr);
-			dev->stats.collisions++;
-			goto tx_error;
-		}
-
 		sk = sock6->sock->sk;
+
 		/* Bypass encapsulation if the destination is local */
 		rt6i_flags = ((struct rt6_info *)ndst)->rt6i_flags;
 		if (!info && rt6i_flags & RTF_LOCAL &&
@@ -2130,6 +2127,10 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 	return;
 
 tx_error:
+	if (err == -ELOOP)
+		dev->stats.collisions++;
+	else if (err == -ENETUNREACH)
+		dev->stats.tx_carrier_errors++;
 	dst_release(ndst);
 	dev->stats.tx_errors++;
 	kfree_skb(skb);
@@ -2411,9 +2412,7 @@ static int vxlan_fill_metadata_dst(struct net_device *dev, struct sk_buff *skb)
 		struct vxlan_sock *sock4 = rcu_dereference(vxlan->vn4_sock);
 		struct rtable *rt;
 
-		if (!sock4)
-			return -EINVAL;
-		rt = vxlan_get_route(vxlan, skb, 0, info->key.tos,
+		rt = vxlan_get_route(vxlan, dev, sock4, skb, 0, info->key.tos,
 				     info->key.u.ipv4.dst,
 				     &info->key.u.ipv4.src, NULL, info);
 		if (IS_ERR(rt))
@@ -2424,7 +2423,7 @@ static int vxlan_fill_metadata_dst(struct net_device *dev, struct sk_buff *skb)
 		struct vxlan_sock *sock6 = rcu_dereference(vxlan->vn6_sock);
 		struct dst_entry *ndst;
 
-		ndst = vxlan6_get_route(vxlan, sock6, skb, 0, info->key.tos,
+		ndst = vxlan6_get_route(vxlan, dev, sock6, skb, 0, info->key.tos,
 					info->key.label, &info->key.u.ipv6.dst,
 					&info->key.u.ipv6.src, NULL, info);
 		if (IS_ERR(ndst))
-- 
1.9.1

[PATCH net-next v3 5/7] vxlan: simplify RTF_LOCAL handling.

[Pravin B Shelar]

Avoid code duplicate code for handling RTF_LOCAL routes.

Signed-off-by: Pravin B Shelar <pshelar@ovn.org>
---
 drivers/net/vxlan.c | 85 ++++++++++++++++++++++++++++++++---------------------
 1 file changed, 51 insertions(+), 34 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index aabb918..0b188d6 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1938,6 +1938,40 @@ static void vxlan_encap_bypass(struct sk_buff *skb, struct vxlan_dev *src_vxlan,
 	}
 }
 
+static int encap_bypass_if_local(struct sk_buff *skb, struct net_device *dev,
+				 struct vxlan_dev *vxlan, union vxlan_addr *daddr,
+				 __be32 dst_port, __be32 vni, struct dst_entry *dst,
+				 u32 rt_flags)
+{
+#if IS_ENABLED(CONFIG_IPV6)
+	/* IPv6 rt-flags are checked against RTF_LOCAL, but the value of
+	 * RTF_LOCAL is equal to RTCF_LOCAL. So to keep code simple
+	 * we can use RTCF_LOCAL which works for ipv4 and ipv6 route entry.
+	 */
+	BUILD_BUG_ON(RTCF_LOCAL != RTF_LOCAL);
+#endif
+	/* Bypass encapsulation if the destination is local */
+	if (rt_flags & RTCF_LOCAL &&
+	    !(rt_flags & (RTCF_BROADCAST | RTCF_MULTICAST))) {
+		struct vxlan_dev *dst_vxlan;
+
+		dst_release(dst);
+		dst_vxlan = vxlan_find_vni(vxlan->net, vni,
+					   daddr->sa.sa_family, dst_port,
+					   vxlan->flags);
+		if (!dst_vxlan) {
+			dev->stats.tx_errors++;
+			kfree_skb(skb);
+
+			return -ENOENT;
+		}
+		vxlan_encap_bypass(skb, vxlan, dst_vxlan);
+		return 1;
+	}
+
+	return 0;
+}
+
 static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 			   struct vxlan_rdst *rdst, bool did_rsc)
 {
@@ -2036,27 +2070,19 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 				     dst_cache, info);
 		if (IS_ERR(rt))
 			goto tx_error;
-		sk = sock4->sock->sk;
 
+		sk = sock4->sock->sk;
 		/* Bypass encapsulation if the destination is local */
-		if (!info && rt->rt_flags & RTCF_LOCAL &&
-		    !(rt->rt_flags & (RTCF_BROADCAST | RTCF_MULTICAST))) {
-			struct vxlan_dev *dst_vxlan;
-
-			ip_rt_put(rt);
-			dst_vxlan = vxlan_find_vni(vxlan->net, vni,
-						   dst->sa.sa_family, dst_port,
-						   vxlan->flags);
-			if (!dst_vxlan)
-				goto tx_error;
-			vxlan_encap_bypass(skb, vxlan, dst_vxlan);
-			return;
-		}
-
-		if (!info)
+		if (!info) {
+			err = encap_bypass_if_local(skb, dev, vxlan, dst,
+						    dst_port, vni, &rt->dst,
+						    rt->rt_flags);
+			if (err)
+				return;
 			udp_sum = !(flags & VXLAN_F_UDP_ZERO_CSUM_TX);
-		else if (info->key.tun_flags & TUNNEL_DONT_FRAGMENT)
+		} else if (info->key.tun_flags & TUNNEL_DONT_FRAGMENT) {
 			df = htons(IP_DF);
+		}
 
 		ndst = &rt->dst;
 		tos = ip_tunnel_ecn_encap(tos, old_iph, skb);
@@ -2072,7 +2098,6 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 #if IS_ENABLED(CONFIG_IPV6)
 	} else {
 		struct vxlan_sock *sock6 = rcu_dereference(vxlan->vn6_sock);
-		u32 rt6i_flags;
 
 		ndst = vxlan6_get_route(vxlan, dev, sock6, skb,
 					rdst ? rdst->remote_ifindex : 0, tos,
@@ -2085,24 +2110,16 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		}
 		sk = sock6->sock->sk;
 
-		/* Bypass encapsulation if the destination is local */
-		rt6i_flags = ((struct rt6_info *)ndst)->rt6i_flags;
-		if (!info && rt6i_flags & RTF_LOCAL &&
-		    !(rt6i_flags & (RTCF_BROADCAST | RTCF_MULTICAST))) {
-			struct vxlan_dev *dst_vxlan;
-
-			dst_vxlan = vxlan_find_vni(vxlan->net, vni,
-						   dst->sa.sa_family, dst_port,
-						   vxlan->flags);
-			if (!dst_vxlan)
-				goto tx_error;
-			dst_release(ndst);
-			vxlan_encap_bypass(skb, vxlan, dst_vxlan);
-			return;
-		}
+		if (!info) {
+			u32 rt6i_flags = ((struct rt6_info *)ndst)->rt6i_flags;
 
-		if (!info)
+			err = encap_bypass_if_local(skb, dev, vxlan, dst,
+						    dst_port, vni, ndst,
+						    rt6i_flags);
+			if (err)
+				return;
 			udp_sum = !(flags & VXLAN_F_UDP_ZERO_CSUM6_TX);
+		}
 
 		tos = ip_tunnel_ecn_encap(tos, old_iph, skb);
 		ttl = ttl ? : ip6_dst_hoplimit(ndst);
-- 
1.9.1

[PATCH net-next v3 6/7] vxlan: simplify vxlan xmit

[Pravin B Shelar]

Existing vxlan xmit function handles two distinct cases.
1. vxlan net device
2. vxlan lwt device.
By seperating initialization these two cases the egress path
looks better.

Signed-off-by: Pravin B Shelar <pshelar@ovn.org>
Acked-by: Jiri Benc <jbenc@redhat.com>
---
 drivers/net/vxlan.c | 78 +++++++++++++++++++++++------------------------------
 1 file changed, 34 insertions(+), 44 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 0b188d6..411534c 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1978,8 +1978,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 	struct dst_cache *dst_cache;
 	struct ip_tunnel_info *info;
 	struct vxlan_dev *vxlan = netdev_priv(dev);
-	struct sock *sk;
-	const struct iphdr *old_iph;
+	const struct iphdr *old_iph = ip_hdr(skb);
 	union vxlan_addr *dst;
 	union vxlan_addr remote_ip, local_ip;
 	union vxlan_addr *src;
@@ -1988,7 +1987,6 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 	__be16 src_port = 0, dst_port;
 	struct dst_entry *ndst = NULL;
 	__be32 vni, label;
-	__be16 df = 0;
 	__u8 tos, ttl;
 	int err;
 	u32 flags = vxlan->flags;
@@ -1998,19 +1996,40 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 	info = skb_tunnel_info(skb);
 
 	if (rdst) {
+		dst = &rdst->remote_ip;
+		if (vxlan_addr_any(dst)) {
+			if (did_rsc) {
+				/* short-circuited back to local bridge */
+				vxlan_encap_bypass(skb, vxlan, vxlan);
+				return;
+			}
+			goto drop;
+		}
+
 		dst_port = rdst->remote_port ? rdst->remote_port : vxlan->cfg.dst_port;
 		vni = rdst->remote_vni;
-		dst = &rdst->remote_ip;
 		src = &vxlan->cfg.saddr;
 		dst_cache = &rdst->dst_cache;
+		md->gbp = skb->mark;
+		ttl = vxlan->cfg.ttl;
+		if (!ttl && vxlan_addr_multicast(dst))
+			ttl = 1;
+
+		tos = vxlan->cfg.tos;
+		if (tos == 1)
+			tos = ip_tunnel_get_dsfield(old_iph, skb);
+
+		if (dst->sa.sa_family == AF_INET)
+			udp_sum = !(flags & VXLAN_F_UDP_ZERO_CSUM_TX);
+		else
+			udp_sum = !(flags & VXLAN_F_UDP_ZERO_CSUM6_TX);
+		label = vxlan->cfg.label;
 	} else {
 		if (!info) {
 			WARN_ONCE(1, "%s: Missing encapsulation instructions\n",
 				  dev->name);
 			goto drop;
 		}
-		dst_port = info->key.tp_dst ? : vxlan->cfg.dst_port;
-		vni = tunnel_id_to_key32(info->key.tun_id);
 		remote_ip.sa.sa_family = ip_tunnel_info_af(info);
 		if (remote_ip.sa.sa_family == AF_INET) {
 			remote_ip.sin.sin_addr.s_addr = info->key.u.ipv4.dst;
@@ -2020,48 +2039,24 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 			local_ip.sin6.sin6_addr = info->key.u.ipv6.src;
 		}
 		dst = &remote_ip;
+		dst_port = info->key.tp_dst ? : vxlan->cfg.dst_port;
+		vni = tunnel_id_to_key32(info->key.tun_id);
 		src = &local_ip;
 		dst_cache = &info->dst_cache;
-	}
-
-	if (vxlan_addr_any(dst)) {
-		if (did_rsc) {
-			/* short-circuited back to local bridge */
-			vxlan_encap_bypass(skb, vxlan, vxlan);
-			return;
-		}
-		goto drop;
-	}
-
-	old_iph = ip_hdr(skb);
-
-	ttl = vxlan->cfg.ttl;
-	if (!ttl && vxlan_addr_multicast(dst))
-		ttl = 1;
-
-	tos = vxlan->cfg.tos;
-	if (tos == 1)
-		tos = ip_tunnel_get_dsfield(old_iph, skb);
-
-	label = vxlan->cfg.label;
-	src_port = udp_flow_src_port(dev_net(dev), skb, vxlan->cfg.port_min,
-				     vxlan->cfg.port_max, true);
-
-	if (info) {
+		if (info->options_len)
+			md = ip_tunnel_info_opts(info);
 		ttl = info->key.ttl;
 		tos = info->key.tos;
 		label = info->key.label;
 		udp_sum = !!(info->key.tun_flags & TUNNEL_CSUM);
-
-		if (info->options_len)
-			md = ip_tunnel_info_opts(info);
-	} else {
-		md->gbp = skb->mark;
 	}
+	src_port = udp_flow_src_port(dev_net(dev), skb, vxlan->cfg.port_min,
+				     vxlan->cfg.port_max, true);
 
 	if (dst->sa.sa_family == AF_INET) {
 		struct vxlan_sock *sock4 = rcu_dereference(vxlan->vn4_sock);
 		struct rtable *rt;
+		__be16 df = 0;
 
 		rt = vxlan_get_route(vxlan, dev, sock4, skb,
 				     rdst ? rdst->remote_ifindex : 0, tos,
@@ -2071,7 +2066,6 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		if (IS_ERR(rt))
 			goto tx_error;
 
-		sk = sock4->sock->sk;
 		/* Bypass encapsulation if the destination is local */
 		if (!info) {
 			err = encap_bypass_if_local(skb, dev, vxlan, dst,
@@ -2079,7 +2073,6 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 						    rt->rt_flags);
 			if (err)
 				return;
-			udp_sum = !(flags & VXLAN_F_UDP_ZERO_CSUM_TX);
 		} else if (info->key.tun_flags & TUNNEL_DONT_FRAGMENT) {
 			df = htons(IP_DF);
 		}
@@ -2092,7 +2085,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		if (err < 0)
 			goto tx_error;
 
-		udp_tunnel_xmit_skb(rt, sk, skb, src->sin.sin_addr.s_addr,
+		udp_tunnel_xmit_skb(rt, sock4->sock->sk, skb, src->sin.sin_addr.s_addr,
 				    dst->sin.sin_addr.s_addr, tos, ttl, df,
 				    src_port, dst_port, xnet, !udp_sum);
 #if IS_ENABLED(CONFIG_IPV6)
@@ -2108,7 +2101,6 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 			ndst = NULL;
 			goto tx_error;
 		}
-		sk = sock6->sock->sk;
 
 		if (!info) {
 			u32 rt6i_flags = ((struct rt6_info *)ndst)->rt6i_flags;
@@ -2118,7 +2110,6 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 						    rt6i_flags);
 			if (err)
 				return;
-			udp_sum = !(flags & VXLAN_F_UDP_ZERO_CSUM6_TX);
 		}
 
 		tos = ip_tunnel_ecn_encap(tos, old_iph, skb);
@@ -2129,13 +2120,12 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		if (err < 0)
 			goto tx_error;
 
-		udp_tunnel6_xmit_skb(ndst, sk, skb, dev,
+		udp_tunnel6_xmit_skb(ndst, sock6->sock->sk, skb, dev,
 				     &src->sin6.sin6_addr,
 				     &dst->sin6.sin6_addr, tos, ttl,
 				     label, src_port, dst_port, !udp_sum);
 #endif
 	}
-
 	return;
 
 drop:
-- 
1.9.1

[PATCH net-next v3 7/7] vxlan: remove unsed vxlan_dev_dst_port()

[Pravin B Shelar]

Signed-off-by: Pravin B Shelar <pshelar@ovn.org>
---
 include/net/vxlan.h | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index 308adc4..49a5920 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -281,16 +281,6 @@ struct vxlan_dev {
 struct net_device *vxlan_dev_create(struct net *net, const char *name,
 				    u8 name_assign_type, struct vxlan_config *conf);
 
-static inline __be16 vxlan_dev_dst_port(struct vxlan_dev *vxlan,
-					unsigned short family)
-{
-#if IS_ENABLED(CONFIG_IPV6)
-	if (family == AF_INET6)
-		return inet_sk(vxlan->vn6_sock->sock->sk)->inet_sport;
-#endif
-	return inet_sk(vxlan->vn4_sock->sock->sk)->inet_sport;
-}
-
 static inline netdev_features_t vxlan_features_check(struct sk_buff *skb,
 						     netdev_features_t features)
 {
-- 
1.9.1

Re: [PATCH net-next v3 2/7] vxlan: avoid checking socket multiple times.

[Jiri Benc]

Pravin,

please CC reviewers of the previous version when submitting a new
version. You'll get faster reviews that way.

On Sun, 13 Nov 2016 20:43:53 -0800, Pravin B Shelar wrote:
> @@ -2069,11 +2069,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>  		struct dst_entry *ndst;
>  		u32 rt6i_flags;
>  
> -		if (!sock6)
> -			goto drop;
> -		sk = sock6->sock->sk;
> -
> -		ndst = vxlan6_get_route(vxlan, skb,
> +		ndst = vxlan6_get_route(vxlan, sock6, skb,
>  					rdst ? rdst->remote_ifindex : 0, tos,
>  					label, &dst->sin6.sin6_addr,
>  					&src->sin6.sin6_addr,
> @@ -2093,6 +2089,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>  			goto tx_error;
>  		}
>  
> +		sk = sock6->sock->sk;
>  		/* Bypass encapsulation if the destination is local */
>  		rt6i_flags = ((struct rt6_info *)ndst)->rt6i_flags;
>  		if (!info && rt6i_flags & RTF_LOCAL &&

This moves the sk assignment from one arbitrary place to a different
arbitrary place, while it would be best to just remove it and open code
sock6->sock->sk in the call to udp_tunnel6_xmit_skb. But patch 6 does
that later, so whatever.

Acked-by: Jiri Benc <jbenc@redhat.com>

Re: [PATCH net-next v3 3/7] vxlan: simplify exception handling

[Jiri Benc]

On Sun, 13 Nov 2016 20:43:54 -0800, Pravin B Shelar wrote:
> @@ -1927,13 +1923,13 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>  	struct ip_tunnel_info *info;
>  	struct vxlan_dev *vxlan = netdev_priv(dev);
>  	struct sock *sk;
> -	struct rtable *rt = NULL;
>  	const struct iphdr *old_iph;
>  	union vxlan_addr *dst;
>  	union vxlan_addr remote_ip, local_ip;
>  	union vxlan_addr *src;
>  	struct vxlan_metadata _md;
>  	struct vxlan_metadata *md = &_md;
> +	struct dst_entry *ndst = NULL;
>  	__be16 src_port = 0, dst_port;
>  	__be32 vni, label;
>  	__be16 df = 0;
> @@ -2009,6 +2005,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>  
>  	if (dst->sa.sa_family == AF_INET) {
>  		struct vxlan_sock *sock4 = rcu_dereference(vxlan->vn4_sock);
> +		struct rtable *rt;
>  
>  		if (!sock4)
>  			goto drop;
> @@ -2030,7 +2027,8 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>  			netdev_dbg(dev, "circular route to %pI4\n",
>  				   &dst->sin.sin_addr.s_addr);
>  			dev->stats.collisions++;
> -			goto rt_tx_error;
> +			ip_rt_put(rt);
> +			goto tx_error;
>  		}
>  
>  		/* Bypass encapsulation if the destination is local */
> @@ -2053,12 +2051,13 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>  		else if (info->key.tun_flags & TUNNEL_DONT_FRAGMENT)
>  			df = htons(IP_DF);
>  
> +		ndst = &rt->dst;

It would be a bit cleaner to do this assignment just after rt is
assigned (but after the IS_ERR(rt) condition), get rid of the added
ip_rt_put call above and move the existing ip_rt_put call in the bypass
case just before the vxlan_encap_bypass call...

>  		tos = ip_tunnel_ecn_encap(tos, old_iph, skb);
>  		ttl = ttl ? : ip4_dst_hoplimit(&rt->dst);
> -		err = vxlan_build_skb(skb, &rt->dst, sizeof(struct iphdr),
> +		err = vxlan_build_skb(skb, ndst, sizeof(struct iphdr),
>  				      vni, md, flags, udp_sum);
>  		if (err < 0)
> -			goto xmit_tx_error;
> +			goto tx_error;
>  
>  		udp_tunnel_xmit_skb(rt, sk, skb, src->sin.sin_addr.s_addr,
>  				    dst->sin.sin_addr.s_addr, tos, ttl, df,
> @@ -2066,7 +2065,6 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>  #if IS_ENABLED(CONFIG_IPV6)
>  	} else {
>  		struct vxlan_sock *sock6 = rcu_dereference(vxlan->vn6_sock);
> -		struct dst_entry *ndst;
>  		u32 rt6i_flags;
>  
>  		ndst = vxlan6_get_route(vxlan, sock6, skb,
> @@ -2078,13 +2076,13 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>  			netdev_dbg(dev, "no route to %pI6\n",
>  				   &dst->sin6.sin6_addr);
>  			dev->stats.tx_carrier_errors++;
> +			ndst = NULL;
>  			goto tx_error;
>  		}
>  
>  		if (ndst->dev == dev) {
>  			netdev_dbg(dev, "circular route to %pI6\n",
>  				   &dst->sin6.sin6_addr);
> -			dst_release(ndst);
>  			dev->stats.collisions++;
>  			goto tx_error;
>  		}
> @@ -2096,12 +2094,12 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>  		    !(rt6i_flags & (RTCF_BROADCAST | RTCF_MULTICAST))) {
>  			struct vxlan_dev *dst_vxlan;
>  
> -			dst_release(ndst);
>  			dst_vxlan = vxlan_find_vni(vxlan->net, vni,
>  						   dst->sa.sa_family, dst_port,
>  						   vxlan->flags);
>  			if (!dst_vxlan)
>  				goto tx_error;
> +			dst_release(ndst);
>  			vxlan_encap_bypass(skb, vxlan, dst_vxlan);
>  			return;
>  		}

...the same way you have it here, in the IPv6 part. Could you change
the IPv4 part to match it?

Looks good otherwise. Seeing it, I like this version much more than v2.

Thanks!

 Jiri

Re: [PATCH net-next v3 4/7] vxlan: improve vxlan route lookup checks.

[Jiri Benc]

On Sun, 13 Nov 2016 20:43:55 -0800, Pravin B Shelar wrote:
> @@ -1929,8 +1951,8 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>  	union vxlan_addr *src;
>  	struct vxlan_metadata _md;
>  	struct vxlan_metadata *md = &_md;
> -	struct dst_entry *ndst = NULL;
>  	__be16 src_port = 0, dst_port;
> +	struct dst_entry *ndst = NULL;
>  	__be32 vni, label;
>  	__be16 df = 0;
>  	__u8 tos, ttl;

This looks kind of arbitrary. You might want to remove this hunk or
merge it to patch 3.

Other than that,
Acked-by: Jiri Benc <jbenc@redhat.com>

Re: [PATCH net-next v3 5/7] vxlan: simplify RTF_LOCAL handling.

[Jiri Benc]

On Sun, 13 Nov 2016 20:43:56 -0800, Pravin B Shelar wrote:
> Avoid code duplicate code for handling RTF_LOCAL routes.
> 
> Signed-off-by: Pravin B Shelar <pshelar@ovn.org>

Acked-by: Jiri Benc <jbenc@redhat.com>

Re: [PATCH net-next v3 7/7] vxlan: remove unsed vxlan_dev_dst_port()

[Jiri Benc]

On Sun, 13 Nov 2016 20:43:58 -0800, Pravin B Shelar wrote:
> Signed-off-by: Pravin B Shelar <pshelar@ovn.org>

Acked-by: Jiri Benc <jbenc@redhat.com>

Re: [PATCH net-next v3 3/7] vxlan: simplify exception handling

[Pravin Shelar]

On Tue, Nov 15, 2016 at 6:30 AM, Jiri Benc <jbenc@redhat.com> wrote:
> On Sun, 13 Nov 2016 20:43:54 -0800, Pravin B Shelar wrote:
>> @@ -1927,13 +1923,13 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>>       struct ip_tunnel_info *info;
>>       struct vxlan_dev *vxlan = netdev_priv(dev);
>>       struct sock *sk;
>> -     struct rtable *rt = NULL;
>>       const struct iphdr *old_iph;
>>       union vxlan_addr *dst;
>>       union vxlan_addr remote_ip, local_ip;
>>       union vxlan_addr *src;
>>       struct vxlan_metadata _md;
>>       struct vxlan_metadata *md = &_md;
>> +     struct dst_entry *ndst = NULL;
>>       __be16 src_port = 0, dst_port;
>>       __be32 vni, label;
>>       __be16 df = 0;
>> @@ -2009,6 +2005,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>>
>>       if (dst->sa.sa_family == AF_INET) {
>>               struct vxlan_sock *sock4 = rcu_dereference(vxlan->vn4_sock);
>> +             struct rtable *rt;
>>
>>               if (!sock4)
>>                       goto drop;
>> @@ -2030,7 +2027,8 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>>                       netdev_dbg(dev, "circular route to %pI4\n",
>>                                  &dst->sin.sin_addr.s_addr);
>>                       dev->stats.collisions++;
>> -                     goto rt_tx_error;
>> +                     ip_rt_put(rt);
>> +                     goto tx_error;
>>               }
>>
>>               /* Bypass encapsulation if the destination is local */
>> @@ -2053,12 +2051,13 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>>               else if (info->key.tun_flags & TUNNEL_DONT_FRAGMENT)
>>                       df = htons(IP_DF);
>>
>> +             ndst = &rt->dst;
>
> It would be a bit cleaner to do this assignment just after rt is
> assigned (but after the IS_ERR(rt) condition), get rid of the added
> ip_rt_put call above and move the existing ip_rt_put call in the bypass
> case just before the vxlan_encap_bypass call...
>
Does it really matters given that next patches in this series moves
this duplicate code and does pretty much what you are describing?

>>               tos = ip_tunnel_ecn_encap(tos, old_iph, skb);
>>               ttl = ttl ? : ip4_dst_hoplimit(&rt->dst);
>> -             err = vxlan_build_skb(skb, &rt->dst, sizeof(struct iphdr),
>> +             err = vxlan_build_skb(skb, ndst, sizeof(struct iphdr),
>>                                     vni, md, flags, udp_sum);
>>               if (err < 0)
>> -                     goto xmit_tx_error;
>> +                     goto tx_error;
>>
>>               udp_tunnel_xmit_skb(rt, sk, skb, src->sin.sin_addr.s_addr,
>>                                   dst->sin.sin_addr.s_addr, tos, ttl, df,
>> @@ -2066,7 +2065,6 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>>  #if IS_ENABLED(CONFIG_IPV6)
>>       } else {
>>               struct vxlan_sock *sock6 = rcu_dereference(vxlan->vn6_sock);
>> -             struct dst_entry *ndst;
>>               u32 rt6i_flags;
>>
>>               ndst = vxlan6_get_route(vxlan, sock6, skb,
>> @@ -2078,13 +2076,13 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>>                       netdev_dbg(dev, "no route to %pI6\n",
>>                                  &dst->sin6.sin6_addr);
>>                       dev->stats.tx_carrier_errors++;
>> +                     ndst = NULL;
>>                       goto tx_error;
>>               }
>>
>>               if (ndst->dev == dev) {
>>                       netdev_dbg(dev, "circular route to %pI6\n",
>>                                  &dst->sin6.sin6_addr);
>> -                     dst_release(ndst);
>>                       dev->stats.collisions++;
>>                       goto tx_error;
>>               }
>> @@ -2096,12 +2094,12 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>>                   !(rt6i_flags & (RTCF_BROADCAST | RTCF_MULTICAST))) {
>>                       struct vxlan_dev *dst_vxlan;
>>
>> -                     dst_release(ndst);
>>                       dst_vxlan = vxlan_find_vni(vxlan->net, vni,
>>                                                  dst->sa.sa_family, dst_port,
>>                                                  vxlan->flags);
>>                       if (!dst_vxlan)
>>                               goto tx_error;
>> +                     dst_release(ndst);
>>                       vxlan_encap_bypass(skb, vxlan, dst_vxlan);
>>                       return;
>>               }
>
> ...the same way you have it here, in the IPv6 part. Could you change
> the IPv4 part to match it?
>
Patch 5 does this by defining encap_bypass_if_local() for IPv4 and
IPv6 vxlan tunnels.

> Looks good otherwise. Seeing it, I like this version much more than v2.
>
> Thanks!
>
>  Jiri

Re: [PATCH net-next v3 3/7] vxlan: simplify exception handling

[Jiri Benc]

On Tue, 15 Nov 2016 08:40:58 -0800, Pravin Shelar wrote:
> On Tue, Nov 15, 2016 at 6:30 AM, Jiri Benc <jbenc@redhat.com> wrote:
> > It would be a bit cleaner to do this assignment just after rt is
> > assigned (but after the IS_ERR(rt) condition), get rid of the added
> > ip_rt_put call above and move the existing ip_rt_put call in the bypass
> > case just before the vxlan_encap_bypass call...
> >
> Does it really matters given that next patches in this series moves
> this duplicate code and does pretty much what you are describing?

Okay, right. I tried to look also at patches further in the series but
it seemed to me this will leave an instance of ip_rt_put that could be
avoided. But it will not.

Acked-by: Jiri Benc <jbenc@redhat.com>

(It would make the reviewers' life easier if the individual patches
were more self contained. Ideally, each patch should be able to stand
on its own. This unrelated code shuffling makes it too easy to miss
things...)

Anyway, thanks for the cleanup!

 Jiri

Re: [PATCH net-next v3 3/7] vxlan: simplify exception handling

[Pravin Shelar]

On Tue, Nov 15, 2016 at 9:04 AM, Jiri Benc <jbenc@redhat.com> wrote:
> On Tue, 15 Nov 2016 08:40:58 -0800, Pravin Shelar wrote:
>> On Tue, Nov 15, 2016 at 6:30 AM, Jiri Benc <jbenc@redhat.com> wrote:
>> > It would be a bit cleaner to do this assignment just after rt is
>> > assigned (but after the IS_ERR(rt) condition), get rid of the added
>> > ip_rt_put call above and move the existing ip_rt_put call in the bypass
>> > case just before the vxlan_encap_bypass call...
>> >
>> Does it really matters given that next patches in this series moves
>> this duplicate code and does pretty much what you are describing?
>
> Okay, right. I tried to look also at patches further in the series but
> it seemed to me this will leave an instance of ip_rt_put that could be
> avoided. But it will not.
>
> Acked-by: Jiri Benc <jbenc@redhat.com>
>
> (It would make the reviewers' life easier if the individual patches
> were more self contained. Ideally, each patch should be able to stand
> on its own. This unrelated code shuffling makes it too easy to miss
> things...)
>
I tried to keep one patch targeting single cleanup. But all patches
targeted same code, that makes it dependent on each other.

> Anyway, thanks for the cleanup!
>

Thanks for all reviews.

Re: [PATCH net-next v3 0/7] vxlan: xmit improvements.

[David Miller]

From: Pravin B Shelar <pshelar@ovn.org>
Date: Sun, 13 Nov 2016 20:43:51 -0800

> Following patch series improves vxlan fast path, removes
> duplicate code and simplifies vxlan xmit code path.
> 
> v2-v3:
> Removed unrelated warning fix from patch 2.
> rearranged error handling from patch 3
> Fixed stats updates in vxlan route lookup in patch 4
> 
> v1-v2:
> Fix compilation error when IPv6 support is not enabled.

Series applied, thanks.

RE: [PATCH net-next v3 4/7] vxlan: improve vxlan route lookup checks.

[David Laight]

From: Of Jiri Benc
> Sent: 15 November 2016 14:40
> On Sun, 13 Nov 2016 20:43:55 -0800, Pravin B Shelar wrote:
> > @@ -1929,8 +1951,8 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
> >  	union vxlan_addr *src;
> >  	struct vxlan_metadata _md;
> >  	struct vxlan_metadata *md = &_md;
> > -	struct dst_entry *ndst = NULL;
> >  	__be16 src_port = 0, dst_port;
> > +	struct dst_entry *ndst = NULL;
> >  	__be32 vni, label;
> >  	__be16 df = 0;
> >  	__u8 tos, ttl;
> 
> This looks kind of arbitrary. You might want to remove this hunk or
> merge it to patch 3.

Worse than arbitrary, it adds 4 bytes of pad on 64bit systems.

	David

Re: [PATCH net-next v3 4/7] vxlan: improve vxlan route lookup checks.

[Jiri Benc]

On Thu, 17 Nov 2016 10:17:01 +0000, David Laight wrote:
> Worse than arbitrary, it adds 4 bytes of pad on 64bit systems.

It does not, this is not a struct.

 Jiri

Re: [PATCH net-next v3 4/7] vxlan: improve vxlan route lookup checks.

[David Miller]

From: Jiri Benc <jbenc@redhat.com>
Date: Thu, 17 Nov 2016 16:59:49 +0100

> On Thu, 17 Nov 2016 10:17:01 +0000, David Laight wrote:
>> Worse than arbitrary, it adds 4 bytes of pad on 64bit systems.
> 
> It does not, this is not a struct.

He is talking about on the function stack.

Re: [PATCH net-next v3 4/7] vxlan: improve vxlan route lookup checks.

[Pravin Shelar]

On Thu, Nov 17, 2016 at 2:17 AM, David Laight <David.Laight@aculab.com> wrote:
> From: Of Jiri Benc
>> Sent: 15 November 2016 14:40
>> On Sun, 13 Nov 2016 20:43:55 -0800, Pravin B Shelar wrote:
>> > @@ -1929,8 +1951,8 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>> >     union vxlan_addr *src;
>> >     struct vxlan_metadata _md;
>> >     struct vxlan_metadata *md = &_md;
>> > -   struct dst_entry *ndst = NULL;
>> >     __be16 src_port = 0, dst_port;
>> > +   struct dst_entry *ndst = NULL;
>> >     __be32 vni, label;
>> >     __be16 df = 0;
>> >     __u8 tos, ttl;
>>
>> This looks kind of arbitrary. You might want to remove this hunk or
>> merge it to patch 3.
>
> Worse than arbitrary, it adds 4 bytes of pad on 64bit systems.
>
OK. I will send out a patch.
But this is not real issue in vxlan module today ;)

Re: [PATCH net-next v3 4/7] vxlan: improve vxlan route lookup checks.

[Pravin Shelar]

On Thu, Nov 17, 2016 at 7:59 AM, Jiri Benc <jbenc@redhat.com> wrote:
> On Thu, 17 Nov 2016 10:17:01 +0000, David Laight wrote:
>> Worse than arbitrary, it adds 4 bytes of pad on 64bit systems.
>
> It does not, this is not a struct.
>
right.

After looking at the assembly code, it is clear that GCC and most of
modern compiler can reorder function variables for efficient storage.