net: flush neighbor entries when carrier is off

PATCH In review David Ahern 3 participants 8 messages Nov 6, 2017 – Nov 8, 2017 1 message not in the archive

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

David Ahern

author

2017-11-06 06:58 UTC #0 lore raw

cover net: flush neighbor entries when carrier is off

Commit a6db4494d218c ("net: ipv4: Consider failed nexthops in multipath routes") added support for checking neighbor state when selecting a path for multipath route lookups. It works but incurs a delay waiting for the neighbor entry to timeout. Improve the path selection by flushing non-permanent neighbor entries when carrier is off.

David Ahern (3):
net: neigh: Add helper to flush entries on carrier down
net: ipv4: flush neighbor entries when carrier is off
net: ipv6: flush neighbor entries when carrier is off

 include/net/neighbour.h |  1 +
 net/core/neighbour.c    | 26 ++++++++++++++++++++++----
 net/ipv4/fib_frontend.c |  7 +++++--
 net/ipv6/addrconf.c     |  3 +++
 4 files changed, 31 insertions(+), 6 deletions(-)

Signature · 1 line

2.1.4

David Ahern

author

2017-11-06 06:58 UTC #1 lore raw

PATCH 1/3 net: neigh: Add helper to flush entries on carrier down

Add neigh_carrier_down helper to flush non-permanent entries on carrier down.

Signed-off-by David Ahern <[email protected]>

---
 include/net/neighbour.h |  1 +
 net/core/neighbour.c    | 26 ++++++++++++++++++++++----
 2 files changed, 23 insertions(+), 4 deletions(-)

include/net/neighbour.h +1 -0

index 2492000e1035..b2eff6ffbfdc 100644
@@ -320,6 +320,7 @@ int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new, u32 flags,
 void __neigh_set_probe_once(struct neighbour *neigh);
 bool neigh_remove_one(struct neighbour *ndel, struct neigh_table *tbl);
 void neigh_changeaddr(struct neigh_table *tbl, struct net_device *dev);
+int neigh_carrier_down(struct neigh_table *tbl, struct net_device *dev);
 int neigh_ifdown(struct neigh_table *tbl, struct net_device *dev);
 int neigh_resolve_output(struct neighbour *neigh, struct sk_buff *skb);
 int neigh_connected_output(struct neighbour *neigh, struct sk_buff *skb);

net/core/neighbour.c +22 -4

index 6ea3a1a7f36a..a7012bca1ce9 100644
@@ -229,7 +229,8 @@ static void pneigh_queue_purge(struct sk_buff_head *list)
 	}
 }
 
-static void neigh_flush_dev(struct neigh_table *tbl, struct net_device *dev)
+static void neigh_flush_dev(struct neigh_table *tbl, struct net_device *dev,
+			    bool skip_perm)
 {
 	int i;
 	struct neigh_hash_table *nht;

@@ -247,6 +248,10 @@ static void neigh_flush_dev(struct neigh_table *tbl, struct net_device *dev)
 				np = &n->next;
 				continue;
 			}
+			if (skip_perm && n->nud_state & NUD_PERMANENT) {
+				np = &n->next;
+				continue;
+			}
 			rcu_assign_pointer(*np,
 				   rcu_dereference_protected(n->next,
 						lockdep_is_held(&tbl->lock)));

@@ -282,20 +287,33 @@ static void neigh_flush_dev(struct neigh_table *tbl, struct net_device *dev)
 void neigh_changeaddr(struct neigh_table *tbl, struct net_device *dev)
 {
 	write_lock_bh(&tbl->lock);
-	neigh_flush_dev(tbl, dev);
+	neigh_flush_dev(tbl, dev, false);
 	write_unlock_bh(&tbl->lock);
 }
 EXPORT_SYMBOL(neigh_changeaddr);
 
-int neigh_ifdown(struct neigh_table *tbl, struct net_device *dev)
+static void __neigh_ifdown(struct neigh_table *tbl, struct net_device *dev,
+			   bool skip_perm)
 {
 	write_lock_bh(&tbl->lock);
-	neigh_flush_dev(tbl, dev);
+	neigh_flush_dev(tbl, dev, skip_perm);
 	pneigh_ifdown(tbl, dev);
 	write_unlock_bh(&tbl->lock);
 
 	del_timer_sync(&tbl->proxy_timer);
 	pneigh_queue_purge(&tbl->proxy_queue);
+}
+
+int neigh_carrier_down(struct neigh_table *tbl, struct net_device *dev)
+{
+	__neigh_ifdown(tbl, dev, true);
+	return 0;
+}
+EXPORT_SYMBOL(neigh_carrier_down);
+
+int neigh_ifdown(struct neigh_table *tbl, struct net_device *dev)
+{
+	__neigh_ifdown(tbl, dev, false);
 	return 0;
 }
 EXPORT_SYMBOL(neigh_ifdown);

Signature · 1 line

2.1.4

David Ahern

author

2017-11-06 06:58 UTC #2 lore raw

PATCH 2/3 net: ipv4: flush neighbor entries when carrier is off

In reply to David Ahern

Signed-off-by Satish Ashok <[email protected]>

Signed-off-by David Ahern <[email protected]>

---
 net/ipv4/fib_frontend.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

net/ipv4/fib_frontend.c +5 -2

index f02819134ba2..aa8fea74858f 100644
@@ -1226,10 +1226,13 @@ static int fib_netdev_event(struct notifier_block *this, unsigned long event, vo
 		break;
 	case NETDEV_CHANGE:
 		flags = dev_get_flags(dev);
-		if (flags & (IFF_RUNNING | IFF_LOWER_UP))
+		if (flags & (IFF_RUNNING | IFF_LOWER_UP)) {
 			fib_sync_up(dev, RTNH_F_LINKDOWN);
-		else
+		} else {
 			fib_sync_down_dev(dev, event, false);
+			if (IN_DEV_IGNORE_ROUTES_WITH_LINKDOWN(in_dev))
+				neigh_carrier_down(&arp_tbl, dev);
+		}
 		/* fall through */
 	case NETDEV_CHANGEMTU:
 		rt_cache_flush(net);

Signature · 1 line

2.1.4

Ido Schimmel

2017-11-06 10:47 UTC #3 lore raw

Re: [PATCH net-next 2/3] net: ipv4: flush neighbor entries when carrier is off

Hi David,

Quoted 17 lines · David Ahern

Commit a6db4494d218c ("net: ipv4: Consider failed nexthops in multipath routes") added support for checking neighbor state when selecting a path for multipath route lookups. It works but incurs a delay waiting for the neighbor entry to timeout. Improve the path selection by flushing non-permanent neighbor entries when carrier is off.
Signed-off-by Satish Ashok <[email protected]>
Signed-off-by David Ahern <[email protected]>
---
 net/ipv4/fib_frontend.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
net/ipv4/fib_frontend.c +0 -0
index f02819134ba2..aa8fea74858f 100644

net/ipv4/fib_frontend.c static int fib_netdev_event(struct notifier_block *this, unsigned long event, vo · line 1233

Quoted 10 lines

net/ipv4/fib_frontend.c +3 -2

@@ -1226,10 +1226,13 @@ static int fib_netdev_event(struct notifier_block *this, unsigned long event, vo
 		break;
 	case NETDEV_CHANGE:
 		flags = dev_get_flags(dev);
-		if (flags & (IFF_RUNNING | IFF_LOWER_UP))
+		if (flags & (IFF_RUNNING | IFF_LOWER_UP)) {
 			fib_sync_up(dev, RTNH_F_LINKDOWN);
-		else
+		} else {
 			fib_sync_down_dev(dev, event, false);
+			if (IN_DEV_IGNORE_ROUTES_WITH_LINKDOWN(in_dev))

Can you please explain why the flushing is conditioned on this sysctl being enabled? If carrier is down and the sysctl is enabled, then the nexthop will not be used anyway.

Thanks

Quoted 8 lines

net/ipv4/fib_frontend.c +2 -0

+				neigh_carrier_down(&arp_tbl, dev);
+		}
 		/* fall through */
 	case NETDEV_CHANGEMTU:
 		rt_cache_flush(net);

Signature · 1 line

2.1.4

David Ahern

author

2017-11-06 14:50 UTC #4 lore raw

Re: [PATCH net-next 2/3] net: ipv4: flush neighbor entries when carrier is off

Quoted 26 lines · Ido Schimmel

Hi David,
Quoted 23 lines · David Ahern
Commit a6db4494d218c ("net: ipv4: Consider failed nexthops in multipath routes") added support for checking neighbor state when selecting a path for multipath route lookups. It works but incurs a delay waiting for the neighbor entry to timeout. Improve the path selection by flushing non-permanent neighbor entries when carrier is off.
Signed-off-by Satish Ashok <[email protected]>
Signed-off-by David Ahern <[email protected]>
---
 net/ipv4/fib_frontend.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
net/ipv4/fib_frontend.c +1 -1
index f02819134ba2..aa8fea74858f 100644
@@ -1226,10 +1226,13 @@ static int fib_netdev_event(struct notifier_block *this, unsigned long event, vo
 		break;
 	case NETDEV_CHANGE:
 		flags = dev_get_flags(dev);
-		if (flags & (IFF_RUNNING | IFF_LOWER_UP))
+		if (flags & (IFF_RUNNING | IFF_LOWER_UP)) {
 			fib_sync_up(dev, RTNH_F_LINKDOWN);

Quoted 8 lines

net/ipv4/fib_frontend.c static int fib_netdev_event(struct notifier_block *this, unsigned long event, vo · line 1233
Quoted 4 lines
net/ipv4/fib_frontend.c +2 -1
-		else
+		} else {
 			fib_sync_down_dev(dev, event, false);
+			if (IN_DEV_IGNORE_ROUTES_WITH_LINKDOWN(in_dev))
Can you please explain why the flushing is conditioned on this sysctl being enabled? If carrier is down and the sysctl is enabled, then the nexthop will not be used anyway.

I pulled the patch from our queue in response to a problem Jamal reported.

For the output path, the result set by fib_table_lookup is ignored and fib_select_path is invoked which for multipath can pick a nexthop with carrier down. Perhaps that is the better change ... to check dead and linkdown flags in fib_select_multipath. I'll try it out tomorrow.

David Ahern

author

2017-11-07 05:30 UTC #5 lore raw

Re: [PATCH net-next 2/3] net: ipv4: flush neighbor entries when carrier is off

Quoted 11 lines · David Ahern

Quoted 11 lines

net/ipv4/fib_frontend.c +1 -1

index f02819134ba2..aa8fea74858f 100644
@@ -1226,10 +1226,13 @@ static int fib_netdev_event(struct notifier_block *this, unsigned long event, vo
 		break;
 	case NETDEV_CHANGE:
 		flags = dev_get_flags(dev);
-		if (flags & (IFF_RUNNING | IFF_LOWER_UP))
+		if (flags & (IFF_RUNNING | IFF_LOWER_UP)) {
 			fib_sync_up(dev, RTNH_F_LINKDOWN);

Quoted 8 lines

Quoted 8 lines
net/ipv4/fib_frontend.c static int fib_netdev_event(struct notifier_block *this, unsigned long event, vo · line 1233
Quoted 4 lines
net/ipv4/fib_frontend.c +2 -1
-		else
+		} else {
 			fib_sync_down_dev(dev, event, false);
+			if (IN_DEV_IGNORE_ROUTES_WITH_LINKDOWN(in_dev))
Can you please explain why the flushing is conditioned on this sysctl being enabled? If carrier is down and the sysctl is enabled, then the nexthop will not be used anyway.

you are right, of course. ;-) With the sysctl enabled nexthops with linkdown have upper_bound set to -1 and will not get selected in fib_select_multipath.

Jamal: the existing code with the sysctl's enabled should fix your use case.

David Ahern

author

2017-11-06 06:58 UTC #6 lore raw

PATCH 3/3 net: ipv6: flush neighbor entries when carrier is off

In reply to David Ahern

Similar to IPv4, flush non-permanent neighbor entries on carrier down to improve path selection for multipath routes.

Signed-off-by Satish Ashok <[email protected]>

Signed-off-by David Ahern <[email protected]>

---
 net/ipv6/addrconf.c | 3 +++
 1 file changed, 3 insertions(+)

net/ipv6/addrconf.c +3 -0

index 5a8a10229a07..85bddff5eac6 100644
@@ -3432,6 +3432,9 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
 				run_pending = 1;
 			}
 		} else if (event == NETDEV_CHANGE) {
+			if (idev && idev->cnf.ignore_routes_with_linkdown)
+				neigh_carrier_down(&nd_tbl, dev);
+
 			if (!addrconf_link_ready(dev)) {
 				/* device is still not ready. */
 				break;

Signature · 1 line

2.1.4

A message that is not in the archive (sent off-list or not yet received): CAKYOsXz3RFqbzENy1rz8kjsr3aF0u2kxBHrSQ3uNfFfQTWbJRQ@mail.gmail.com

David Miller

maintainer

2017-11-08 03:39 UTC #8 lore raw

From: David Ahern <[email protected]>
Date: Wed, 8 Nov 2017 12:35:16 +0900

Quoted 20 lines · David Ahern

Quoted 12 lines
Commit a6db4494d218c ("net: ipv4: Consider failed nexthops in multipath routes") added support for checking neighbor state when selecting a path for multipath route lookups. It works but incurs a delay waiting for the neighbor entry to timeout. Improve the path selection by flushing non-permanent neighbor entries when carrier is off.
David Ahern (3):
net: neigh: Add helper to flush entries on carrier down
net: ipv4: flush neighbor entries when carrier is off
net: ipv6: flush neighbor entries when carrier is off
Quoted 7 lines
 include/net/neighbour.h |  1 +
 net/core/neighbour.c    | 26 ++++++++++++++++++++++----
 net/ipv4/fib_frontend.c |  7 +++++--
 net/ipv6/addrconf.c     |  3 +++
 4 files changed, 31 insertions(+), 6 deletions(-)

Quoted 2 lines

Dave: please drop this set. It's not needed for the use case mentioned above, so at a minimum the commit message needs to be updated.

Yep, sure, no problem.