[PATCH 0/6] IPV6 related gc bug + cleanups

[PATCH 0/6] IPV6 related gc bug + cleanups

[Stephen Hemminger]

First one is a bug fix, that gets GC working again, rest can wait
until sh*t settles.
-- 

[PATCH 1/6] ipv6: use timer pending

[Stephen Hemminger]

[-- Attachment #1: ipv6-timer.patch --]
[-- Type: text/plain, Size: 1178 bytes --]

This fixes the bridge reference count problem and cleanups ipv6 FIB timer management.
Don't use expires field, because it is not a proper way to test,
instead use timer_pending().

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/net/ipv6/ip6_fib.c	2008-07-21 11:25:23.000000000 -0700
+++ b/net/ipv6/ip6_fib.c	2008-07-21 11:42:30.000000000 -0700
@@ -661,17 +661,17 @@ static int fib6_add_rt2node(struct fib6_
 
 static __inline__ void fib6_start_gc(struct net *net, struct rt6_info *rt)
 {
-	if (net->ipv6.ip6_fib_timer->expires == 0 &&
+	if (!timer_pending(net->ipv6.ip6_fib_timer) &&
 	    (rt->rt6i_flags & (RTF_EXPIRES|RTF_CACHE)))
-		mod_timer(net->ipv6.ip6_fib_timer, jiffies +
-			  net->ipv6.sysctl.ip6_rt_gc_interval);
+		mod_timer(net->ipv6.ip6_fib_timer,
+			  jiffies + net->ipv6.sysctl.ip6_rt_gc_interval);
 }
 
 void fib6_force_start_gc(struct net *net)
 {
-	if (net->ipv6.ip6_fib_timer->expires == 0)
-		mod_timer(net->ipv6.ip6_fib_timer, jiffies +
-			  net->ipv6.sysctl.ip6_rt_gc_interval);
+	if (!timer_pending(net->ipv6.ip6_fib_timer))
+		mod_timer(net->ipv6.ip6_fib_timer,
+			  jiffies + net->ipv6.sysctl.ip6_rt_gc_interval);
 }
 
 /*

-- 

[PATCH 2/6] netns: dont alloc ipv6 fib timer list

[Stephen Hemminger]

[-- Attachment #1: ipv6-timer-alloc.patch --]
[-- Type: text/plain, Size: 2827 bytes --]

FIB timer list is a trivial size structure, avoid indirection and just
put it in existing ns.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/net/ipv6/ip6_fib.c	2008-07-21 11:57:29.000000000 -0700
+++ b/net/ipv6/ip6_fib.c	2008-07-21 12:02:07.000000000 -0700
@@ -661,16 +661,16 @@ static int fib6_add_rt2node(struct fib6_
 
 static void fib6_start_gc(struct net *net, struct rt6_info *rt)
 {
-	if (!timer_pending(net->ipv6.ip6_fib_timer) &&
+	if (!timer_pending(&net->ipv6.ip6_fib_timer) &&
 	    (rt->rt6i_flags & (RTF_EXPIRES|RTF_CACHE)))
-		mod_timer(net->ipv6.ip6_fib_timer, jiffies +
+		mod_timer(&net->ipv6.ip6_fib_timer, jiffies +
 			  net->ipv6.sysctl.ip6_rt_gc_interval);
 }
 
 void fib6_force_start_gc(struct net *net)
 {
-	if (!timer_pending(net->ipv6.ip6_fib_timer))
-		mod_timer(net->ipv6.ip6_fib_timer, jiffies +
+	if (!timer_pending(&net->ipv6.ip6_fib_timer))
+		mod_timer(&net->ipv6.ip6_fib_timer, jiffies +
 			  net->ipv6.sysctl.ip6_rt_gc_interval);
 }
 
@@ -1449,7 +1449,7 @@ void fib6_run_gc(unsigned long expires, 
 	} else {
 		local_bh_disable();
 		if (!spin_trylock(&fib6_gc_lock)) {
-			mod_timer(net->ipv6.ip6_fib_timer, jiffies + HZ);
+			mod_timer(&net->ipv6.ip6_fib_timer, jiffies + HZ);
 			local_bh_enable();
 			return;
 		}
@@ -1462,10 +1462,10 @@ void fib6_run_gc(unsigned long expires, 
 	fib6_clean_all(net, fib6_age, 0, NULL);
 
 	if (gc_args.more)
-		mod_timer(net->ipv6.ip6_fib_timer, jiffies +
+		mod_timer(&net->ipv6.ip6_fib_timer, jiffies +
 			  net->ipv6.sysctl.ip6_rt_gc_interval);
 	else
-		del_timer(net->ipv6.ip6_fib_timer);
+		del_timer(&net->ipv6.ip6_fib_timer);
 	spin_unlock_bh(&fib6_gc_lock);
 }
 
@@ -1476,16 +1476,7 @@ static void fib6_gc_timer_cb(unsigned lo
 
 static int fib6_net_init(struct net *net)
 {
-	int ret;
-	struct timer_list *timer;
-
-	ret = -ENOMEM;
-	timer = kzalloc(sizeof(*timer), GFP_KERNEL);
-	if (!timer)
-		goto out;
-
-	setup_timer(timer, fib6_gc_timer_cb, (unsigned long)net);
-	net->ipv6.ip6_fib_timer = timer;
+	setup_timer(&net->ipv6.ip6_fib_timer, fib6_gc_timer_cb, (unsigned long)net);
 
 	net->ipv6.rt6_stats = kzalloc(sizeof(*net->ipv6.rt6_stats), GFP_KERNEL);
 	if (!net->ipv6.rt6_stats)
@@ -1519,9 +1510,7 @@ static int fib6_net_init(struct net *net
 #endif
 	fib6_tables_init(net);
 
-	ret = 0;
-out:
-	return ret;
+	return 0;
 
 #ifdef CONFIG_IPV6_MULTIPLE_TABLES
 out_fib6_main_tbl:
@@ -1532,15 +1521,14 @@ out_fib_table_hash:
 out_rt6_stats:
 	kfree(net->ipv6.rt6_stats);
 out_timer:
-	kfree(timer);
-	goto out;
+	return -ENOMEM;
  }
 
 static void fib6_net_exit(struct net *net)
 {
 	rt6_ifdown(net, NULL);
-	del_timer_sync(net->ipv6.ip6_fib_timer);
-	kfree(net->ipv6.ip6_fib_timer);
+	del_timer_sync(&net->ipv6.ip6_fib_timer);
+
 #ifdef CONFIG_IPV6_MULTIPLE_TABLES
 	kfree(net->ipv6.fib6_local_tbl);
 #endif

-- 

[PATCH 3/6] ipv6: use round_jiffies

[Stephen Hemminger]

[-- Attachment #1: ipv6-round.patch --]
[-- Type: text/plain, Size: 821 bytes --]

This timer normally happens once a minute, there is no need to cause an
early wakeup for it, so align it to next second boundary to safe power.
It can't be deferred because then it could take too long on cleanup or DoS.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/net/ipv6/ip6_fib.c	2008-07-21 12:06:44.000000000 -0700
+++ b/net/ipv6/ip6_fib.c	2008-07-21 12:07:41.000000000 -0700
@@ -1462,8 +1462,9 @@ void fib6_run_gc(unsigned long expires, 
 	fib6_clean_all(net, fib6_age, 0, NULL);
 
 	if (gc_args.more)
-		mod_timer(&net->ipv6.ip6_fib_timer, jiffies +
-			  net->ipv6.sysctl.ip6_rt_gc_interval);
+		mod_timer(&net->ipv6.ip6_fib_timer,
+			  round_jiffies(jiffies
+					+ net->ipv6.sysctl.ip6_rt_gc_interval));
 	else
 		del_timer(&net->ipv6.ip6_fib_timer);
 	spin_unlock_bh(&fib6_gc_lock);

-- 

[PATCH 4/6] ipv6: use spin_trylock_bh

[Stephen Hemminger]

[-- Attachment #1: ipv6-spin-lock-bh.patch --]
[-- Type: text/plain, Size: 668 bytes --]

Now there is spin_trylock_bh, use it rather than open coding.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/net/ipv6/ip6_fib.c	2008-07-21 12:11:46.000000000 -0700
+++ b/net/ipv6/ip6_fib.c	2008-07-21 12:17:52.000000000 -0700
@@ -1447,10 +1447,8 @@ void fib6_run_gc(unsigned long expires, 
 		gc_args.timeout = expires ? (int)expires :
 			net->ipv6.sysctl.ip6_rt_gc_interval;
 	} else {
-		local_bh_disable();
-		if (!spin_trylock(&fib6_gc_lock)) {
+		if (!spin_trylock_bh(&fib6_gc_lock)) {
 			mod_timer(&net->ipv6.ip6_fib_timer, jiffies + HZ);
-			local_bh_enable();
 			return;
 		}
 		gc_args.timeout = net->ipv6.sysctl.ip6_rt_gc_interval;

-- 

[PATCH 5/6] ipv6: use kcalloc

[Stephen Hemminger]

[-- Attachment #1: ipv6-kcalloc.patch --]
[-- Type: text/plain, Size: 634 bytes --]

Th fib_table_hash is an array, so use kcalloc.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/net/ipv6/ip6_fib.c	2008-07-21 12:21:15.000000000 -0700
+++ b/net/ipv6/ip6_fib.c	2008-07-21 12:21:15.000000000 -0700
@@ -1481,9 +1481,9 @@ static int fib6_net_init(struct net *net
 	if (!net->ipv6.rt6_stats)
 		goto out_timer;
 
-	net->ipv6.fib_table_hash =
-		kzalloc(sizeof(*net->ipv6.fib_table_hash)*FIB_TABLE_HASHSZ,
-			GFP_KERNEL);
+	net->ipv6.fib_table_hash = kcalloc(FIB_TABLE_HASHSZ,
+					   sizeof(*net->ipv6.fib_table_hash),
+					   GFP_KERNEL);
 	if (!net->ipv6.fib_table_hash)
 		goto out_rt6_stats;
 

-- 

[PATCH 6/6] ipv6: icmp6_dst_gc return change

[Stephen Hemminger]

[-- Attachment #1: ipv6-icmp-dst.patch --]
[-- Type: text/plain, Size: 1732 bytes --]

Change icmp6_dst_gc to return the one value the caller cares about rather
than using call by reference.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/include/net/ip6_route.h	2008-07-21 12:15:35.000000000 -0700
+++ b/include/net/ip6_route.h	2008-07-21 12:15:41.000000000 -0700
@@ -68,7 +68,7 @@ extern struct rt6_info		*rt6_lookup(stru
 extern struct dst_entry *icmp6_dst_alloc(struct net_device *dev,
 					 struct neighbour *neigh,
 					 const struct in6_addr *addr);
-extern int icmp6_dst_gc(int *more);
+extern int icmp6_dst_gc(void);
 
 extern void fib6_force_start_gc(struct net *net);
 
--- a/net/ipv6/ip6_fib.c	2008-07-21 12:13:41.000000000 -0700
+++ b/net/ipv6/ip6_fib.c	2008-07-21 12:14:35.000000000 -0700
@@ -1453,9 +1453,8 @@ void fib6_run_gc(unsigned long expires, 
 		}
 		gc_args.timeout = net->ipv6.sysctl.ip6_rt_gc_interval;
 	}
-	gc_args.more = 0;
 
-	icmp6_dst_gc(&gc_args.more);
+	gc_args.more = icmp6_dst_gc();
 
 	fib6_clean_all(net, fib6_age, 0, NULL);
 
--- a/net/ipv6/route.c	2008-07-21 12:14:06.000000000 -0700
+++ b/net/ipv6/route.c	2008-07-21 12:15:14.000000000 -0700
@@ -978,13 +978,12 @@ out:
 	return &rt->u.dst;
 }
 
-int icmp6_dst_gc(int *more)
+int icmp6_dst_gc(void)
 {
 	struct dst_entry *dst, *next, **pprev;
-	int freed;
+	int more = 0;
 
 	next = NULL;
-	freed = 0;
 
 	spin_lock_bh(&icmp6_dst_lock);
 	pprev = &icmp6_dst_gc_list;
@@ -993,16 +992,15 @@ int icmp6_dst_gc(int *more)
 		if (!atomic_read(&dst->__refcnt)) {
 			*pprev = dst->next;
 			dst_free(dst);
-			freed++;
 		} else {
 			pprev = &dst->next;
-			(*more)++;
+			++more;
 		}
 	}
 
 	spin_unlock_bh(&icmp6_dst_lock);
 
-	return freed;
+	return more;
 }
 
 static int ip6_dst_gc(struct dst_ops *ops)

-- 

Re: [PATCH 1/6] ipv6: use timer pending

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 21 Jul 2008 12:28:35 -0700

> This fixes the bridge reference count problem and cleanups ipv6 FIB timer management.
> Don't use expires field, because it is not a proper way to test,
> instead use timer_pending().
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied, and I'll queue this up for -stable too.

Thanks!

Re: [PATCH 2/6] netns: dont alloc ipv6 fib timer list

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 21 Jul 2008 12:28:36 -0700

> FIB timer list is a trivial size structure, avoid indirection and just
> put it in existing ns.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

I don't see how this works only changing ip6_fib.c, isn't
there a header file change necessary to go along with
this?

In particular there needs to be some change to include/net/netns/ipv6.h

[PATCH 2/6] netns: dont alloc ipv6 fib timer list

[Stephen Hemminger]

Subject: netns: don't alloc ipv6 fib timer list

FIB timer list is a trivial size structure, avoid indirection and just
put it in existing ns.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
 include/net/netns/ipv6.h |    2 +-
 net/ipv6/ip6_fib.c       |   36 ++++++++++++------------------------
 2 files changed, 13 insertions(+), 25 deletions(-)

--- a/net/ipv6/ip6_fib.c	2008-07-21 13:29:40.000000000 -0700
+++ b/net/ipv6/ip6_fib.c	2008-07-21 13:29:42.000000000 -0700
@@ -661,16 +661,16 @@ static int fib6_add_rt2node(struct fib6_
 
 static void fib6_start_gc(struct net *net, struct rt6_info *rt)
 {
-	if (!timer_pending(net->ipv6.ip6_fib_timer) &&
+	if (!timer_pending(&net->ipv6.ip6_fib_timer) &&
 	    (rt->rt6i_flags & (RTF_EXPIRES|RTF_CACHE)))
-		mod_timer(net->ipv6.ip6_fib_timer, jiffies +
+		mod_timer(&net->ipv6.ip6_fib_timer, jiffies +
 			  net->ipv6.sysctl.ip6_rt_gc_interval);
 }
 
 void fib6_force_start_gc(struct net *net)
 {
-	if (!timer_pending(net->ipv6.ip6_fib_timer))
-		mod_timer(net->ipv6.ip6_fib_timer, jiffies +
+	if (!timer_pending(&net->ipv6.ip6_fib_timer))
+		mod_timer(&net->ipv6.ip6_fib_timer, jiffies +
 			  net->ipv6.sysctl.ip6_rt_gc_interval);
 }
 
@@ -1449,7 +1449,7 @@ void fib6_run_gc(unsigned long expires, 
 	} else {
 		local_bh_disable();
 		if (!spin_trylock(&fib6_gc_lock)) {
-			mod_timer(net->ipv6.ip6_fib_timer, jiffies + HZ);
+			mod_timer(&net->ipv6.ip6_fib_timer, jiffies + HZ);
 			local_bh_enable();
 			return;
 		}
@@ -1462,10 +1462,10 @@ void fib6_run_gc(unsigned long expires, 
 	fib6_clean_all(net, fib6_age, 0, NULL);
 
 	if (gc_args.more)
-		mod_timer(net->ipv6.ip6_fib_timer, jiffies +
+		mod_timer(&net->ipv6.ip6_fib_timer, jiffies +
 			  net->ipv6.sysctl.ip6_rt_gc_interval);
 	else
-		del_timer(net->ipv6.ip6_fib_timer);
+		del_timer(&net->ipv6.ip6_fib_timer);
 	spin_unlock_bh(&fib6_gc_lock);
 }
 
@@ -1476,16 +1476,7 @@ static void fib6_gc_timer_cb(unsigned lo
 
 static int fib6_net_init(struct net *net)
 {
-	int ret;
-	struct timer_list *timer;
-
-	ret = -ENOMEM;
-	timer = kzalloc(sizeof(*timer), GFP_KERNEL);
-	if (!timer)
-		goto out;
-
-	setup_timer(timer, fib6_gc_timer_cb, (unsigned long)net);
-	net->ipv6.ip6_fib_timer = timer;
+	setup_timer(&net->ipv6.ip6_fib_timer, fib6_gc_timer_cb, (unsigned long)net);
 
 	net->ipv6.rt6_stats = kzalloc(sizeof(*net->ipv6.rt6_stats), GFP_KERNEL);
 	if (!net->ipv6.rt6_stats)
@@ -1519,9 +1510,7 @@ static int fib6_net_init(struct net *net
 #endif
 	fib6_tables_init(net);
 
-	ret = 0;
-out:
-	return ret;
+	return 0;
 
 #ifdef CONFIG_IPV6_MULTIPLE_TABLES
 out_fib6_main_tbl:
@@ -1532,15 +1521,14 @@ out_fib_table_hash:
 out_rt6_stats:
 	kfree(net->ipv6.rt6_stats);
 out_timer:
-	kfree(timer);
-	goto out;
+	return -ENOMEM;
  }
 
 static void fib6_net_exit(struct net *net)
 {
 	rt6_ifdown(net, NULL);
-	del_timer_sync(net->ipv6.ip6_fib_timer);
-	kfree(net->ipv6.ip6_fib_timer);
+	del_timer_sync(&net->ipv6.ip6_fib_timer);
+
 #ifdef CONFIG_IPV6_MULTIPLE_TABLES
 	kfree(net->ipv6.fib6_local_tbl);
 #endif
--- a/include/net/netns/ipv6.h	2008-07-21 13:29:56.000000000 -0700
+++ b/include/net/netns/ipv6.h	2008-07-21 13:27:07.000000000 -0700
@@ -39,7 +39,7 @@ struct netns_ipv6 {
 #endif
 	struct rt6_info         *ip6_null_entry;
 	struct rt6_statistics   *rt6_stats;
-	struct timer_list       *ip6_fib_timer;
+	struct timer_list       ip6_fib_timer;
 	struct hlist_head       *fib_table_hash;
 	struct fib6_table       *fib6_main_tbl;
 	struct dst_ops		*ip6_dst_ops;

Re: [PATCH 2/6] netns: dont alloc ipv6 fib timer list

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 21 Jul 2008 13:30:37 -0700

> Subject: netns: don't alloc ipv6 fib timer list
> 
> FIB timer list is a trivial size structure, avoid indirection and just
> put it in existing ns.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Bzzt, try again.

error: patch failed: net/ipv6/ip6_fib.c:661
error: net/ipv6/ip6_fib.c: patch does not apply

[PATCH 2/3] netns: timer allocation

[Stephen Hemminger]

FIB timer list is a trivial size structure, avoid indirection and just
put it in existing ns.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
Applies to net-next-2.6 (after PATCH 1)

--- a/net/ipv6/ip6_fib.c	2008-07-21 13:40:46.000000000 -0700
+++ b/net/ipv6/ip6_fib.c	2008-07-21 13:40:48.000000000 -0700
@@ -661,16 +661,16 @@ static int fib6_add_rt2node(struct fib6_
 
 static void fib6_start_gc(struct net *net, struct rt6_info *rt)
 {
-	if (!timer_pending(net->ipv6.ip6_fib_timer) &&
+	if (!timer_pending(&net->ipv6.ip6_fib_timer) &&
 	    (rt->rt6i_flags & (RTF_EXPIRES|RTF_CACHE)))
-		mod_timer(net->ipv6.ip6_fib_timer, jiffies +
+		mod_timer(&net->ipv6.ip6_fib_timer, jiffies +
 			  net->ipv6.sysctl.ip6_rt_gc_interval);
 }
 
 void fib6_force_start_gc(struct net *net)
 {
-	if (!timer_pending(net->ipv6.ip6_fib_timer))
-		mod_timer(net->ipv6.ip6_fib_timer, jiffies +
+	if (!timer_pending(&net->ipv6.ip6_fib_timer))
+		mod_timer(&net->ipv6.ip6_fib_timer, jiffies +
 			  net->ipv6.sysctl.ip6_rt_gc_interval);
 }
 
@@ -1449,7 +1449,7 @@ void fib6_run_gc(unsigned long expires, 
 	} else {
 		local_bh_disable();
 		if (!spin_trylock(&fib6_gc_lock)) {
-			mod_timer(net->ipv6.ip6_fib_timer, jiffies + HZ);
+			mod_timer(&net->ipv6.ip6_fib_timer, jiffies + HZ);
 			local_bh_enable();
 			return;
 		}
@@ -1462,10 +1462,10 @@ void fib6_run_gc(unsigned long expires, 
 	fib6_clean_all(net, fib6_age, 0, NULL);
 
 	if (gc_args.more)
-		mod_timer(net->ipv6.ip6_fib_timer, jiffies +
+		mod_timer(&net->ipv6.ip6_fib_timer, jiffies +
 			  net->ipv6.sysctl.ip6_rt_gc_interval);
 	else
-		del_timer(net->ipv6.ip6_fib_timer);
+		del_timer(&net->ipv6.ip6_fib_timer);
 	spin_unlock_bh(&fib6_gc_lock);
 }
 
@@ -1476,16 +1476,7 @@ static void fib6_gc_timer_cb(unsigned lo
 
 static int fib6_net_init(struct net *net)
 {
-	int ret;
-	struct timer_list *timer;
-
-	ret = -ENOMEM;
-	timer = kzalloc(sizeof(*timer), GFP_KERNEL);
-	if (!timer)
-		goto out;
-
-	setup_timer(timer, fib6_gc_timer_cb, (unsigned long)net);
-	net->ipv6.ip6_fib_timer = timer;
+	setup_timer(&net->ipv6.ip6_fib_timer, fib6_gc_timer_cb, (unsigned long)net);
 
 	net->ipv6.rt6_stats = kzalloc(sizeof(*net->ipv6.rt6_stats), GFP_KERNEL);
 	if (!net->ipv6.rt6_stats)
@@ -1519,9 +1510,7 @@ static int fib6_net_init(struct net *net
 #endif
 	fib6_tables_init(net);
 
-	ret = 0;
-out:
-	return ret;
+	return 0;
 
 #ifdef CONFIG_IPV6_MULTIPLE_TABLES
 out_fib6_main_tbl:
@@ -1532,15 +1521,14 @@ out_fib_table_hash:
 out_rt6_stats:
 	kfree(net->ipv6.rt6_stats);
 out_timer:
-	kfree(timer);
-	goto out;
+	return -ENOMEM;
  }
 
 static void fib6_net_exit(struct net *net)
 {
 	rt6_ifdown(net, NULL);
-	del_timer_sync(net->ipv6.ip6_fib_timer);
-	kfree(net->ipv6.ip6_fib_timer);
+	del_timer_sync(&net->ipv6.ip6_fib_timer);
+
 #ifdef CONFIG_IPV6_MULTIPLE_TABLES
 	kfree(net->ipv6.fib6_local_tbl);
 #endif
--- a/include/net/netns/ipv6.h	2008-07-21 13:39:51.000000000 -0700
+++ b/include/net/netns/ipv6.h	2008-07-21 13:40:48.000000000 -0700
@@ -39,7 +39,7 @@ struct netns_ipv6 {
 #endif
 	struct rt6_info         *ip6_null_entry;
 	struct rt6_statistics   *rt6_stats;
-	struct timer_list       *ip6_fib_timer;
+	struct timer_list       ip6_fib_timer;
 	struct hlist_head       *fib_table_hash;
 	struct fib6_table       *fib6_main_tbl;
 	struct dst_ops		*ip6_dst_ops;

Re: [PATCH 2/3] netns: timer allocation

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 21 Jul 2008 13:44:26 -0700

> FIB timer list is a trivial size structure, avoid indirection and just
> put it in existing ns.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> ---
> Applies to net-next-2.6 (after PATCH 1)

I've applied patch 1, and it still does not apply.

fib6_start_gc() has the __inline__ tag in my tree, in
your patch it does not.

Re: [PATCH 2/3] netns: timer allocation

[Stephen Hemminger]

FIB timer list is a trivial size structure, avoid indirection and just
put it in existing ns.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
Sigh.. part 1 patch was not refreshed.

--- a/net/ipv6/ip6_fib.c	2008-07-21 14:06:22.000000000 -0700
+++ b/net/ipv6/ip6_fib.c	2008-07-21 14:06:32.000000000 -0700
@@ -661,16 +661,16 @@ static int fib6_add_rt2node(struct fib6_
 
 static __inline__ void fib6_start_gc(struct net *net, struct rt6_info *rt)
 {
-	if (!timer_pending(net->ipv6.ip6_fib_timer) &&
+	if (!timer_pending(&net->ipv6.ip6_fib_timer) &&
 	    (rt->rt6i_flags & (RTF_EXPIRES|RTF_CACHE)))
-		mod_timer(net->ipv6.ip6_fib_timer, jiffies +
+		mod_timer(&net->ipv6.ip6_fib_timer, jiffies +
 			  net->ipv6.sysctl.ip6_rt_gc_interval);
 }
 
 void fib6_force_start_gc(struct net *net)
 {
-	if (!timer_pending(net->ipv6.ip6_fib_timer))
-		mod_timer(net->ipv6.ip6_fib_timer, jiffies +
+	if (!timer_pending(&net->ipv6.ip6_fib_timer))
+		mod_timer(&net->ipv6.ip6_fib_timer, jiffies +
 			  net->ipv6.sysctl.ip6_rt_gc_interval);
 }
 
@@ -1449,7 +1449,7 @@ void fib6_run_gc(unsigned long expires, 
 	} else {
 		local_bh_disable();
 		if (!spin_trylock(&fib6_gc_lock)) {
-			mod_timer(net->ipv6.ip6_fib_timer, jiffies + HZ);
+			mod_timer(&net->ipv6.ip6_fib_timer, jiffies + HZ);
 			local_bh_enable();
 			return;
 		}
@@ -1462,10 +1462,10 @@ void fib6_run_gc(unsigned long expires, 
 	fib6_clean_all(net, fib6_age, 0, NULL);
 
 	if (gc_args.more)
-		mod_timer(net->ipv6.ip6_fib_timer, jiffies +
+		mod_timer(&net->ipv6.ip6_fib_timer, jiffies +
 			  net->ipv6.sysctl.ip6_rt_gc_interval);
 	else
-		del_timer(net->ipv6.ip6_fib_timer);
+		del_timer(&net->ipv6.ip6_fib_timer);
 	spin_unlock_bh(&fib6_gc_lock);
 }
 
@@ -1476,16 +1476,7 @@ static void fib6_gc_timer_cb(unsigned lo
 
 static int fib6_net_init(struct net *net)
 {
-	int ret;
-	struct timer_list *timer;
-
-	ret = -ENOMEM;
-	timer = kzalloc(sizeof(*timer), GFP_KERNEL);
-	if (!timer)
-		goto out;
-
-	setup_timer(timer, fib6_gc_timer_cb, (unsigned long)net);
-	net->ipv6.ip6_fib_timer = timer;
+	setup_timer(&net->ipv6.ip6_fib_timer, fib6_gc_timer_cb, (unsigned long)net);
 
 	net->ipv6.rt6_stats = kzalloc(sizeof(*net->ipv6.rt6_stats), GFP_KERNEL);
 	if (!net->ipv6.rt6_stats)
@@ -1519,9 +1510,7 @@ static int fib6_net_init(struct net *net
 #endif
 	fib6_tables_init(net);
 
-	ret = 0;
-out:
-	return ret;
+	return 0;
 
 #ifdef CONFIG_IPV6_MULTIPLE_TABLES
 out_fib6_main_tbl:
@@ -1532,15 +1521,14 @@ out_fib_table_hash:
 out_rt6_stats:
 	kfree(net->ipv6.rt6_stats);
 out_timer:
-	kfree(timer);
-	goto out;
+	return -ENOMEM;
  }
 
 static void fib6_net_exit(struct net *net)
 {
 	rt6_ifdown(net, NULL);
-	del_timer_sync(net->ipv6.ip6_fib_timer);
-	kfree(net->ipv6.ip6_fib_timer);
+	del_timer_sync(&net->ipv6.ip6_fib_timer);
+
 #ifdef CONFIG_IPV6_MULTIPLE_TABLES
 	kfree(net->ipv6.fib6_local_tbl);
 #endif
--- a/include/net/netns/ipv6.h	2008-07-21 14:05:01.000000000 -0700
+++ b/include/net/netns/ipv6.h	2008-07-21 14:06:32.000000000 -0700
@@ -39,7 +39,7 @@ struct netns_ipv6 {
 #endif
 	struct rt6_info         *ip6_null_entry;
 	struct rt6_statistics   *rt6_stats;
-	struct timer_list       *ip6_fib_timer;
+	struct timer_list       ip6_fib_timer;
 	struct hlist_head       *fib_table_hash;
 	struct fib6_table       *fib6_main_tbl;
 	struct dst_ops		*ip6_dst_ops;

Re: [PATCH 1/6] ipv6: use timer pending

[Ben Greear]

Stephen Hemminger wrote:
> This fixes the bridge reference count problem and cleanups ipv6 FIB timer management.
> Don't use expires field, because it is not a proper way to test,
> instead use timer_pending().

I patched this into my 2.6.25, and it does not seem to help
my refcount problem.  I did not apply any of your other
patches as it seemed they were mostly cleanups.

This is not to say that this patch is wrong...but
it seems we'll have to keep looking for other problems
as well.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 2/3] netns: timer allocation

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 21 Jul 2008 14:07:51 -0700

> FIB timer list is a trivial size structure, avoid indirection and just
> put it in existing ns.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> ---
> Sigh.. part 1 patch was not refreshed.

Sigh, really, 3 times?

+ git apply --check --whitespace=error-all diff
error: patch failed: net/ipv6/ip6_fib.c:661
error: net/ipv6/ip6_fib.c: patch does not apply

Would it kill you to just clone out a fresh net-2.6 tree and actually
try to apply the patch before sending it to me?

You're asking me to do that 3+ times.  So you can do it once, right?
:)

> --- a/net/ipv6/ip6_fib.c	2008-07-21 14:06:22.000000000 -0700
> +++ b/net/ipv6/ip6_fib.c	2008-07-21 14:06:32.000000000 -0700
> @@ -661,16 +661,16 @@ static int fib6_add_rt2node(struct fib6_
>  
>  static __inline__ void fib6_start_gc(struct net *net, struct rt6_info *rt)
>  {
> -	if (!timer_pending(net->ipv6.ip6_fib_timer) &&
> +	if (!timer_pending(&net->ipv6.ip6_fib_timer) &&
>  	    (rt->rt6i_flags & (RTF_EXPIRES|RTF_CACHE)))
> -		mod_timer(net->ipv6.ip6_fib_timer, jiffies +
> +		mod_timer(&net->ipv6.ip6_fib_timer, jiffies +
>  			  net->ipv6.sysctl.ip6_rt_gc_interval);
>  }
>  

In net-2.6 those mod_timer() lines read:

		mod_timer(net->ipv6.ip6_fib_timer,
			  jiffies + net->ipv6.sysctl.ip6_rt_gc_interval);

the jiffies starts on the second line, not the first.

Something is seriously wrong with whatever tree you're generating
these patches against.

Re: [PATCH 2/6] netns: dont alloc ipv6 fib timer list

[Daniel Lezcano]

Stephen Hemminger wrote:
> FIB timer list is a trivial size structure, avoid indirection and just
> put it in existing ns.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> --- a/net/ipv6/ip6_fib.c	2008-07-21 11:57:29.000000000 -0700
> +++ b/net/ipv6/ip6_fib.c	2008-07-21 12:02:07.000000000 -0700
> @@ -661,16 +661,16 @@ static int fib6_add_rt2node(struct fib6_
> 
>  static void fib6_start_gc(struct net *net, struct rt6_info *rt)
>  {
> -	if (!timer_pending(net->ipv6.ip6_fib_timer) &&
> +	if (!timer_pending(&net->ipv6.ip6_fib_timer) &&
>  	    (rt->rt6i_flags & (RTF_EXPIRES|RTF_CACHE)))
> -		mod_timer(net->ipv6.ip6_fib_timer, jiffies +
> +		mod_timer(&net->ipv6.ip6_fib_timer, jiffies +
>  			  net->ipv6.sysctl.ip6_rt_gc_interval);
>  }
> 
>  void fib6_force_start_gc(struct net *net)
>  {
> -	if (!timer_pending(net->ipv6.ip6_fib_timer))
> -		mod_timer(net->ipv6.ip6_fib_timer, jiffies +
> +	if (!timer_pending(&net->ipv6.ip6_fib_timer))
> +		mod_timer(&net->ipv6.ip6_fib_timer, jiffies +
>  			  net->ipv6.sysctl.ip6_rt_gc_interval);
>  }
> 
> @@ -1449,7 +1449,7 @@ void fib6_run_gc(unsigned long expires, 
>  	} else {
>  		local_bh_disable();
>  		if (!spin_trylock(&fib6_gc_lock)) {
> -			mod_timer(net->ipv6.ip6_fib_timer, jiffies + HZ);
> +			mod_timer(&net->ipv6.ip6_fib_timer, jiffies + HZ);
>  			local_bh_enable();
>  			return;
>  		}
> @@ -1462,10 +1462,10 @@ void fib6_run_gc(unsigned long expires, 
>  	fib6_clean_all(net, fib6_age, 0, NULL);
> 
>  	if (gc_args.more)
> -		mod_timer(net->ipv6.ip6_fib_timer, jiffies +
> +		mod_timer(&net->ipv6.ip6_fib_timer, jiffies +
>  			  net->ipv6.sysctl.ip6_rt_gc_interval);
>  	else
> -		del_timer(net->ipv6.ip6_fib_timer);
> +		del_timer(&net->ipv6.ip6_fib_timer);
>  	spin_unlock_bh(&fib6_gc_lock);
>  }
> 
> @@ -1476,16 +1476,7 @@ static void fib6_gc_timer_cb(unsigned lo
> 
>  static int fib6_net_init(struct net *net)
>  {
> -	int ret;
> -	struct timer_list *timer;
> -
> -	ret = -ENOMEM;
> -	timer = kzalloc(sizeof(*timer), GFP_KERNEL);
> -	if (!timer)
> -		goto out;
> -
> -	setup_timer(timer, fib6_gc_timer_cb, (unsigned long)net);
> -	net->ipv6.ip6_fib_timer = timer;
> +	setup_timer(&net->ipv6.ip6_fib_timer, fib6_gc_timer_cb, (unsigned long)net);

Perhaps I missed it, but I don't see in the patchset where is 
ip6_fib_timer changed to be no longer a pointer in the netns ipv6 structure.

>  	net->ipv6.rt6_stats = kzalloc(sizeof(*net->ipv6.rt6_stats), GFP_KERNEL);
>  	if (!net->ipv6.rt6_stats)
> @@ -1519,9 +1510,7 @@ static int fib6_net_init(struct net *net
>  #endif
>  	fib6_tables_init(net);
> 
> -	ret = 0;
> -out:
> -	return ret;
> +	return 0;
> 
>  #ifdef CONFIG_IPV6_MULTIPLE_TABLES
>  out_fib6_main_tbl:
> @@ -1532,15 +1521,14 @@ out_fib_table_hash:
>  out_rt6_stats:
>  	kfree(net->ipv6.rt6_stats);
>  out_timer:
> -	kfree(timer);
> -	goto out;
> +	return -ENOMEM;
>   }
> 
>  static void fib6_net_exit(struct net *net)
>  {
>  	rt6_ifdown(net, NULL);
> -	del_timer_sync(net->ipv6.ip6_fib_timer);
> -	kfree(net->ipv6.ip6_fib_timer);
> +	del_timer_sync(&net->ipv6.ip6_fib_timer);
> +
>  #ifdef CONFIG_IPV6_MULTIPLE_TABLES
>  	kfree(net->ipv6.fib6_local_tbl);
>  #endif

Re: [PATCH 1/6] ipv6: use timer pending

[Stephen Hemminger]

On Mon, 21 Jul 2008 16:16:15 -0700
Ben Greear <greearb@candelatech.com> wrote:

> Stephen Hemminger wrote:
> > This fixes the bridge reference count problem and cleanups ipv6 FIB timer management.
> > Don't use expires field, because it is not a proper way to test,
> > instead use timer_pending().
> 
> I patched this into my 2.6.25, and it does not seem to help
> my refcount problem.  I did not apply any of your other
> patches as it seemed they were mostly cleanups.
> 
> This is not to say that this patch is wrong...but
> it seems we'll have to keep looking for other problems
> as well.

I agree this patch is not a direct fix for the the problem. What happened
was that I was retesting on latest 2.6 net kernel and adding tracing
and checking.  In the process, the problem went away. Either
a Heisenbug or some other part of the configuration changed.

Re: [PATCH 1/6] ipv6: use timer pending

[Ben Greear]

Stephen Hemminger wrote:
> On Mon, 21 Jul 2008 16:16:15 -0700
> Ben Greear <greearb@candelatech.com> wrote:
> 
>> Stephen Hemminger wrote:
>>> This fixes the bridge reference count problem and cleanups ipv6 FIB timer management.
>>> Don't use expires field, because it is not a proper way to test,
>>> instead use timer_pending().
>> I patched this into my 2.6.25, and it does not seem to help
>> my refcount problem.  I did not apply any of your other
>> patches as it seemed they were mostly cleanups.
>>
>> This is not to say that this patch is wrong...but
>> it seems we'll have to keep looking for other problems
>> as well.
> 
> I agree this patch is not a direct fix for the the problem. What happened
> was that I was retesting on latest 2.6 net kernel and adding tracing
> and checking.  In the process, the problem went away. Either
> a Heisenbug or some other part of the configuration changed.

Patrick sent me a debugging patch and we're poking at the problem.

Currently, it seems we are leaking in6_dev refcounts...possibly
due to a timer leak, but it's tricky code (for me, at least).  I'm
trying to add more debugging now, as I have a test case that
easily reproduces the problem.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 1/6] ipv6: use timer pending

[Ben Greear]

Stephen Hemminger wrote:
> On Mon, 21 Jul 2008 16:16:15 -0700
> Ben Greear <greearb@candelatech.com> wrote:
> 
>> Stephen Hemminger wrote:
>>> This fixes the bridge reference count problem and cleanups ipv6 FIB timer management.
>>> Don't use expires field, because it is not a proper way to test,
>>> instead use timer_pending().
>> I patched this into my 2.6.25, and it does not seem to help
>> my refcount problem.  I did not apply any of your other
>> patches as it seemed they were mostly cleanups.
>>
>> This is not to say that this patch is wrong...but
>> it seems we'll have to keep looking for other problems
>> as well.
> 
> I agree this patch is not a direct fix for the the problem. What happened
> was that I was retesting on latest 2.6 net kernel and adding tracing
> and checking.  In the process, the problem went away. Either
> a Heisenbug or some other part of the configuration changed.

It turns out my problem was due to a patch I wrote
to ensure netlink notify of ipv6 address adds (regardless of DAD completion).

http://lists.openwall.net/netdev/2008/06/18/158

I was using ipv6_ifa_notify instead of inet6_ifa_notify.

Patrick found this, so he gets all the credit.

Of course, since my patch is not in the main tree, whatever refcount
problems you guys are seeing must be something different.

I'll keep the debug code in my tree and if I managed to reproduce it
again, will be sure to try to track it down.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com