Skip to contentSkip to conversation

ipv6: per netns FIB6 walkers and garbage collector

PATCH In review Michal Kubecek 3 participants 8 messages Mar 8, 2016

Subsystems (3)
Networking Network drivers IPv4/IPv6
lore
Share & tools
lore

Michal Kubecek

author
#0 lore raw

v2 cover ipv6: per netns FIB6 walkers and garbage collector

Commit 2ac3ac8f86f2 ("ipv6: prevent fib6_run_gc() contention") reduced the risk of contention on FIB6 garbage collector lock on systems with many CPUs. However, one of our customers can still observe heavy contention on fib6_gc_lock which can even trigger the soft lockup detector.

This is caused by garbage collector running in forced mode from a timer. While there is one timer per network namespace, the instances of fib6_run_gc() running from them are protected by one global spinlock so that only one garbage collector can run at any moment and other namespaces have to wait. As most relevant data structures are separated per netns, there is little reason for garbage collectors blocking each other.

Similar problem exists for walkers: changes in one tree do not need to adjust (and block) walkers traversing FIB trees in other namespaces.

This series separates both the walkers infrastructure and garbage collector so that they work independently in network namespaces.

v2: get rid of ifdef in ipv6_route_seq_setup_walk(), pass net from callers instead

Michal Kubecek (3):
ipv6: replace global gc_args with local variable
ipv6: per netns fib6 walkers
ipv6: per netns FIB garbage collection

 include/net/netns/ipv6.h |  3 ++
 net/ipv6/ip6_fib.c       | 91 +++++++++++++++++++++++++-----------------------
 2 files changed, 51 insertions(+), 43 deletions(-)
Signature · 1 line
2.7.2
author
#1 lore raw

PATCH v2 1/3 ipv6: replace global gc_args with local variable

Global variable gc_args is only used in fib6_run_gc() and functions called from it. As fib6_run_gc() makes sure there is at most one instance of fib6_clean_all() running at any moment, we can replace gc_args with a local variable which will be needed once multiple instances (per netns) of garbage collector are allowed.

Signed-off-by Michal Kubecek <[email protected]>
---
 net/ipv6/ip6_fib.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)
net/ipv6/ip6_fib.c +8 -6
index 0c7e276c230e..d7c715accac9 100644
@@ -1725,14 +1725,15 @@ static void fib6_flush_trees(struct net *net)
  *	Garbage collection
  */
 
-static struct fib6_gc_args
+struct fib6_gc_args
 {
 	int			timeout;
 	int			more;
-} gc_args;
+};
 
 static int fib6_age(struct rt6_info *rt, void *arg)
 {
+	struct fib6_gc_args *gc_args = arg;
 	unsigned long now = jiffies;
 
 	/*

@@ -1748,10 +1749,10 @@ static int fib6_age(struct rt6_info *rt, void *arg)
 			RT6_TRACE("expiring %p\n", rt);
 			return -1;
 		}
-		gc_args.more++;
+		gc_args->more++;
 	} else if (rt->rt6i_flags & RTF_CACHE) {
 		if (atomic_read(&rt->dst.__refcnt) == 0 &&
-		    time_after_eq(now, rt->dst.lastuse + gc_args.timeout)) {
+		    time_after_eq(now, rt->dst.lastuse + gc_args->timeout)) {
 			RT6_TRACE("aging clone %p\n", rt);
 			return -1;
 		} else if (rt->rt6i_flags & RTF_GATEWAY) {

@@ -1769,7 +1770,7 @@ static int fib6_age(struct rt6_info *rt, void *arg)
 				return -1;
 			}
 		}
-		gc_args.more++;
+		gc_args->more++;
 	}
 
 	return 0;

@@ -1779,6 +1780,7 @@ static DEFINE_SPINLOCK(fib6_gc_lock);
 
 void fib6_run_gc(unsigned long expires, struct net *net, bool force)
 {
+	struct fib6_gc_args gc_args;
 	unsigned long now;
 
 	if (force) {

@@ -1792,7 +1794,7 @@ void fib6_run_gc(unsigned long expires, struct net *net, bool force)
 
 	gc_args.more = icmp6_dst_gc();
 
-	fib6_clean_all(net, fib6_age, NULL);
+	fib6_clean_all(net, fib6_age, &gc_args);
 	now = jiffies;
 	net->ipv6.ip6_rt_last_gc = now;
Signature · 1 line
2.7.2
Reviewed-by on v2 1/3
#2 lore raw

Re: [PATCH net-next v2 1/3] ipv6: replace global gc_args with local variable

Quoted 7 lines · Michal Kubecek

Global variable gc_args is only used in fib6_run_gc() and functions called from it. As fib6_run_gc() makes sure there is at most one instance of fib6_clean_all() running at any moment, we can replace gc_args with a local variable which will be needed once multiple instances (per netns) of garbage collector are allowed.

Signed-off-by Michal Kubecek <[email protected]>
Reviewed-by Cong Wang <[email protected]>

Thanks.

author
#3 lore raw

PATCH v2 2/3 ipv6: per netns fib6 walkers

In reply to Michal Kubecek

The IPv6 FIB data structures are separated per network namespace but there is still only one global walkers list and one global walker list lock. This means changes in one namespace unnecessarily interfere with walkers in other namespaces.

Replace the global list with per-netns lists (and give each its own lock).

Signed-off-by Michal Kubecek <[email protected]>

---
v2: get rid of ifdef in ipv6_route_seq_setup_walk(), pass net from callers instead

---
 include/net/netns/ipv6.h |  2 ++
 net/ipv6/ip6_fib.c       | 68 +++++++++++++++++++++++++-----------------------
 2 files changed, 38 insertions(+), 32 deletions(-)
include/net/netns/ipv6.h +2 -0
index c0368db6df54..f0109b973648 100644
@@ -58,7 +58,9 @@ struct netns_ipv6 {
 	struct timer_list       ip6_fib_timer;
 	struct hlist_head       *fib_table_hash;
 	struct fib6_table       *fib6_main_tbl;
+	struct list_head	fib6_walkers;
 	struct dst_ops		ip6_dst_ops;
+	rwlock_t		fib6_walker_lock;
 	unsigned int		 ip6_rt_gc_expire;
 	unsigned long		 ip6_rt_last_gc;
 #ifdef CONFIG_IPV6_MULTIPLE_TABLES
net/ipv6/ip6_fib.c +36 -32

211 lines not on this page · raw message

Signature · 1 line
2.7.2
Reviewed-by on v2 2/3
#4 lore raw

Re: [PATCH net-next v2 2/3] ipv6: per netns fib6 walkers

Quoted 5 lines · Michal Kubecek

The IPv6 FIB data structures are separated per network namespace but there is still only one global walkers list and one global walker list lock. This means changes in one namespace unnecessarily interfere with walkers in other namespaces.

Quoted 7 lines

Replace the global list with per-netns lists (and give each its own lock).

Signed-off-by Michal Kubecek <[email protected]>

---
v2: get rid of ifdef in ipv6_route_seq_setup_walk(), pass net from callers instead

Reviewed-by Cong Wang <[email protected]>
author
#5 lore raw

PATCH v2 3/3 ipv6: per netns FIB garbage collection

In reply to Michal Kubecek

One of our customers observed issues with FIB6 garbage collectors running in different network namespaces blocking each other, resulting in soft lockups (fib6_run_gc() initiated from timer runs always in forced mode).

Now that FIB6 walkers are separated per namespace, there is no more need for instances of fib6_run_gc() in different namespaces blocking each other. There is still a call to icmp6_dst_gc() which operates on shared data but this function is protected by its own shared lock.

Signed-off-by Michal Kubecek <[email protected]>
---
 include/net/netns/ipv6.h | 1 +
 net/ipv6/ip6_fib.c       | 9 ++++-----
 2 files changed, 5 insertions(+), 5 deletions(-)
include/net/netns/ipv6.h +1 -0
index f0109b973648..10d0848f5b8a 100644
@@ -61,6 +61,7 @@ struct netns_ipv6 {
 	struct list_head	fib6_walkers;
 	struct dst_ops		ip6_dst_ops;
 	rwlock_t		fib6_walker_lock;
+	spinlock_t		fib6_gc_lock;
 	unsigned int		 ip6_rt_gc_expire;
 	unsigned long		 ip6_rt_last_gc;
 #ifdef CONFIG_IPV6_MULTIPLE_TABLES
net/ipv6/ip6_fib.c +4 -5
index 883f2836beab..ea071fad67a0 100644
@@ -1776,16 +1776,14 @@ static int fib6_age(struct rt6_info *rt, void *arg)
 	return 0;
 }
 
-static DEFINE_SPINLOCK(fib6_gc_lock);
-
 void fib6_run_gc(unsigned long expires, struct net *net, bool force)
 {
 	struct fib6_gc_args gc_args;
 	unsigned long now;
 
 	if (force) {
-		spin_lock_bh(&fib6_gc_lock);
-	} else if (!spin_trylock_bh(&fib6_gc_lock)) {
+		spin_lock_bh(&net->ipv6.fib6_gc_lock);
+	} else if (!spin_trylock_bh(&net->ipv6.fib6_gc_lock)) {
 		mod_timer(&net->ipv6.ip6_fib_timer, jiffies + HZ);
 		return;
 	}

@@ -1804,7 +1802,7 @@ void fib6_run_gc(unsigned long expires, struct net *net, bool force)
 					+ net->ipv6.sysctl.ip6_rt_gc_interval));
 	else
 		del_timer(&net->ipv6.ip6_fib_timer);
-	spin_unlock_bh(&fib6_gc_lock);
+	spin_unlock_bh(&net->ipv6.fib6_gc_lock);
 }
 
 static void fib6_gc_timer_cb(unsigned long arg)

@@ -1816,6 +1814,7 @@ static int __net_init fib6_net_init(struct net *net)
 {
 	size_t size = sizeof(struct hlist_head) * FIB6_TABLE_HASHSZ;
 
+	spin_lock_init(&net->ipv6.fib6_gc_lock);
 	rwlock_init(&net->ipv6.fib6_walker_lock);
 	INIT_LIST_HEAD(&net->ipv6.fib6_walkers);
 	setup_timer(&net->ipv6.ip6_fib_timer, fib6_gc_timer_cb, (unsigned long)net);
Signature · 1 line
2.7.2
Reviewed-by on v2 3/3
#6 lore raw

Re: [PATCH net-next v2 3/3] ipv6: per netns FIB garbage collection

Quoted 11 lines · Michal Kubecek

One of our customers observed issues with FIB6 garbage collectors running in different network namespaces blocking each other, resulting in soft lockups (fib6_run_gc() initiated from timer runs always in forced mode).

Now that FIB6 walkers are separated per namespace, there is no more need for instances of fib6_run_gc() in different namespaces blocking each other. There is still a call to icmp6_dst_gc() which operates on shared data but this function is protected by its own shared lock.

Signed-off-by Michal Kubecek <[email protected]>
Reviewed-by Cong Wang <[email protected]>
maintainer
#7 lore raw
In reply to Michal Kubecek

From: Michal Kubecek <[email protected]>
Date: Tue, 8 Mar 2016 14:44:15 +0100 (CET)

Quoted 14 lines

Commit 2ac3ac8f86f2 ("ipv6: prevent fib6_run_gc() contention") reduced the risk of contention on FIB6 garbage collector lock on systems with many CPUs. However, one of our customers can still observe heavy contention on fib6_gc_lock which can even trigger the soft lockup detector.

This is caused by garbage collector running in forced mode from a timer. While there is one timer per network namespace, the instances of fib6_run_gc() running from them are protected by one global spinlock so that only one garbage collector can run at any moment and other namespaces have to wait. As most relevant data structures are separated per netns, there is little reason for garbage collectors blocking each other.

Quoted 8 lines

Similar problem exists for walkers: changes in one tree do not need to adjust (and block) walkers traversing FIB trees in other namespaces.

This series separates both the walkers infrastructure and garbage collector so that they work independently in network namespaces.

v2: get rid of ifdef in ipv6_route_seq_setup_walk(), pass net from callers instead

Series applied, thanks.