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

[Jiri Benc]

On Thu, 10 Nov 2016 10:10:25 -0800, Pravin Shelar wrote:
> I am specifically talking about cached routes. If the dst entry is
> cached, this patch avoids checking for device loop.

Okay, true, for cached routes we do less work with this patch.

This was more a side note anyway, the real comment was increasing of
the dev stats (which you already said you would fix).

Thanks!

 Jiri

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

[Pravin Shelar]

On Thu, Nov 10, 2016 at 1:56 AM, Jiri Benc  wrote:
> On Wed, 9 Nov 2016 19:34:06 -0800, Pravin Shelar wrote:
>> Why it would not help in non-ovs vxlan egress path? It avoids checking
>> (if condition) for device loop.
>
> I may be missing something but I count the same number of conditions
> for each packet, they're just at a different place after the patch.
>
I am specifically talking about cached routes. If the dst entry is
cached, this patch avoids checking for device loop.

> E.g. for IPv4, the "if (!sock4)" is moved from vxlan_xmit_one into
> vxlan_get_route and the "rt" error handling stays logically the same
> (three if conditions in the non-error path) but is moved into
> vxlan_get_route. Similarly for IPv6.
>
>  Jiri

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

[Jiri Benc]

On Wed, 9 Nov 2016 19:34:06 -0800, Pravin Shelar wrote:
> Why it would not help in non-ovs vxlan egress path? It avoids checking
> (if condition) for device loop.

I may be missing something but I count the same number of conditions
for each packet, they're just at a different place after the patch.

E.g. for IPv4, the "if (!sock4)" is moved from vxlan_xmit_one into
vxlan_get_route and the "rt" error handling stays logically the same
(three if conditions in the non-error path) but is moved into
vxlan_get_route. Similarly for IPv6.

 Jiri

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

[Pravin Shelar]

On Wed, Nov 9, 2016 at 8:41 AM, Jiri Benc  wrote:
> On Sat,  5 Nov 2016 11:45:54 -0700, Pravin B Shelar wrote:
>> 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().
>
> The description is misleading, it applies only to one vxlan lwt use case
> (openvswitch). For other use cases, the patch has no effect.
>
Why it would not help in non-ovs vxlan egress path? It avoids checking
(if condition) for device loop.

> I found the current handling of route lookup results irritating, too.
> The reason I did not change this while doing vxlan cleanup some time
> ago was that I assumed we should not increase dev stats from
> vxlan_fill_metadata_dst. Isn't that so?
>

Thats right. I will fix it.

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

[Jiri Benc]

On Sat,  5 Nov 2016 11:45:54 -0700, Pravin B Shelar wrote:
> 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().

The description is misleading, it applies only to one vxlan lwt use case
(openvswitch). For other use cases, the patch has no effect.

I found the current handling of route lookup results irritating, too.
The reason I did not change this while doing vxlan cleanup some time
ago was that I assumed we should not increase dev stats from
vxlan_fill_metadata_dst. Isn't that so?

 Jiri

[PATCH net-next v2 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 
---
 drivers/net/vxlan.c | 71 +
 1 file changed, 34 insertions(+), 37 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index bd17ab5..f106178 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1795,7 +1795,8 @@ static int vxlan_build_skb(struct sk_buff *skb, struct 
dst_entry *dst,
return err;
 }
 
-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,
@@ -1805,6 +1806,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) {
@@ -1822,16 +1826,27 @@ static struct rtable *vxlan_get_route(struct vxlan_dev 
*vxlan,
fl4.saddr = *saddr;
 
rt = ip_route_output_key(vxlan->net, );
-   if (!IS_ERR(rt)) {
+   if (likely(!IS_ERR(rt))) {
+   if (rt->dst.dev == dev) {
+   netdev_dbg(dev, "circular route to %pI4\n", );
+   dev->stats.collisions++;
+   ip_rt_put(rt);
+   return ERR_PTR(-ELOOP);
+   }
+
*saddr = fl4.saddr;
if (use_cache)
dst_cache_set_ip4(dst_cache, >dst, fl4.saddr);
+   } else {
+   netdev_dbg(dev, "no route to %pI4\n", );
+   dev->stats.tx_carrier_errors++;
}
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,
@@ -1867,8 +1882,18 @@ static struct dst_entry *vxlan6_get_route(struct 
vxlan_dev *vxlan,
err = ipv6_stub->ipv6_dst_lookup(vxlan->net,
 sock6->sock->sk,
 , );
-   if (err < 0)
+   if (unlikely(err < 0)) {
+   netdev_dbg(dev, "no route to %pI6\n", daddr);
+   dev->stats.tx_carrier_errors++;
return ERR_PTR(err);
+   }
+
+   if (unlikely(ndst->dev == dev)) {
+   netdev_dbg(dev, "circular route to %pI6\n", daddr);
+   dst_release(ndst);
+   dev->stats.collisions++;
+   return ERR_PTR(-ELOOP);
+   }
 
*saddr = fl6.saddr;
if (use_cache)
@@ -2012,29 +2037,15 @@ 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,
 >sin.sin_addr.s_addr,
 dst_cache, info);
-   if (IS_ERR(rt)) {
-   netdev_dbg(dev, "no route to %pI4\n",
-  >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",
-  >sin.sin_addr.s_addr);
-   dev->stats.collisions++;
-   ip_rt_put(rt);
+   if (IS_ERR(rt))
goto tx_error;
-   }
 
/* Bypass encapsulation if the destination is local */
if (!info && rt->rt_flags & RTCF_LOCAL &&
@@ -2074,25 +2085,13 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct 
net_device *dev,
 
sk = sock6->sock->sk;
 
-   ndst = vxlan6_get_route(vxlan, sock6, skb,
+   ndst =