IPV6 related gc bug + cleanups

PATCH Applied Stephen Hemminger 4 participants 20 messages Jul 21, 2008 – Jul 23, 2008

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Stephen Hemminger

author

2008-07-21 19:31 UTC #0 lore raw

cover IPV6 related gc bug + cleanups

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

Stephen Hemminger

author

2008-07-21 19:31 UTC #1 lore raw

PATCH 1/6 ipv6: use timer pending

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 <[email protected]>

net/ipv6/ip6_fib.c +6 -6

@@ -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);
 }
 
 /*

David Miller

maintainer Applied

2008-07-21 20:21 UTC #2 lore raw

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

From: Stephen Hemminger <[email protected]>
Date: Mon, 21 Jul 2008 12:28:35 -0700

Quoted 5 lines

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 <[email protected]>

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

Thanks!

Ben Greear

2008-07-21 23:16 UTC #3 lore raw

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

In reply to Stephen Hemminger

Quoted 3 lines · Stephen Hemminger

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

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Stephen Hemminger

author

2008-07-22 19:04 UTC #4 lore raw

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

Quoted 5 lines · On Mon, 21 Jul 2008 16:16:15 -0700 Ben Greear <[email protected]> wrote:

Quoted 3 lines · Stephen Hemminger
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().

Quoted 7 lines

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.

Ben Greear

2008-07-22 19:10 UTC #5 lore raw

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

Quoted 11 lines · Stephen Hemminger

Quoted 8 lines · On Mon, 21 Jul 2008 16:16:15 -0700 Ben Greear <[email protected]> wrote:
Quoted 3 lines · Stephen Hemminger
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.

Quoted 8 lines

Quoted 3 lines
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

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Ben Greear

2008-07-23 00:35 UTC #6 lore raw

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

In reply to Stephen Hemminger

Quoted 11 lines · Stephen Hemminger

Quoted 8 lines · On Mon, 21 Jul 2008 16:16:15 -0700 Ben Greear <[email protected]> wrote:
Quoted 3 lines · Stephen Hemminger
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.

Quoted 8 lines

Quoted 3 lines
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

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Stephen Hemminger

author

2008-07-21 19:31 UTC #7 lore raw

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

In reply to Stephen Hemminger

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

Signed-off-by Stephen Hemminger <[email protected]>

net/ipv6/ip6_fib.c +12 ~~-24~~

@@ -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

David Miller

maintainer

2008-07-21 20:26 UTC #8 lore raw

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

From: Stephen Hemminger <[email protected]>
Date: Mon, 21 Jul 2008 12:28:36 -0700

Quoted 4 lines

FIB timer list is a trivial size structure, avoid indirection and just put it in existing ns.
Signed-off-by Stephen Hemminger <[email protected]>

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

Stephen Hemminger

author

2008-07-21 20:30 UTC #9 lore raw

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

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 <[email protected]>

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

net/ipv6/ip6_fib.c +12 ~~-24~~

@@ -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

include/net/netns/ipv6.h +1 -1

@@ -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;

David Miller

maintainer

2008-07-21 20:33 UTC #10 lore raw

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

From: Stephen Hemminger <[email protected]>
Date: Mon, 21 Jul 2008 13:30:37 -0700

Quoted 6 lines

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 <[email protected]>

Bzzt, try again.

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

Stephen Hemminger

author

2008-07-21 20:44 UTC #11 lore raw

PATCH 2/3 netns: timer allocation

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

Signed-off-by Stephen Hemminger <[email protected]>

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

net/ipv6/ip6_fib.c +12 ~~-24~~

@@ -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

include/net/netns/ipv6.h +1 -1

@@ -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;

David Miller

maintainer

2008-07-21 20:47 UTC #12 lore raw

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

From: Stephen Hemminger <[email protected]>
Date: Mon, 21 Jul 2008 13:44:26 -0700

Quoted 7 lines

FIB timer list is a trivial size structure, avoid indirection and just put it in existing ns.
Signed-off-by Stephen Hemminger <[email protected]>
---
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.

Stephen Hemminger

author

2008-07-21 21:07 UTC #13 lore raw

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

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

Signed-off-by Stephen Hemminger <[email protected]>

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

net/ipv6/ip6_fib.c +12 ~~-24~~

@@ -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

include/net/netns/ipv6.h +1 -1

@@ -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;

David Miller

maintainer

2008-07-21 23:28 UTC #14 lore raw

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

From: Stephen Hemminger <[email protected]>
Date: Mon, 21 Jul 2008 14:07:51 -0700

Quoted 7 lines

FIB timer list is a trivial size structure, avoid indirection and just put it in existing ns.
Signed-off-by Stephen Hemminger <[email protected]>
---
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? :)

Quoted 4 lines

net/ipv6/ip6_fib.c +0 -0

@@ -661,16 +661,16 @@ static int fib6_add_rt2node(struct fib6_

net/ipv6/ip6_fib.c static int fib6_add_rt2node(struct fib6_ · line 669

Quoted 10 lines

net/ipv6/ip6_fib.c +2 -2

 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.

Daniel Lezcano

2008-07-22 02:21 UTC #15 lore raw

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

In reply to Stephen Hemminger

Quoted 55 lines · Stephen Hemminger

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

Signed-off-by Stephen Hemminger <[email protected]>

net/ipv6/ip6_fib.c +7 -8

@@ -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;

net/ipv6/ip6_fib.c static void fib6_gc_timer_cb(unsigned lo · line 1479

Quoted 10 lines

net/ipv6/ip6_fib.c +1 -9

-	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.

Quoted 32 lines

net/ipv6/ip6_fib.c +4 -7

 	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

Stephen Hemminger

author

2008-07-21 19:31 UTC #16 lore raw

PATCH 3/6 ipv6: use round_jiffies

In reply to Stephen Hemminger

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 <[email protected]>

net/ipv6/ip6_fib.c +3 -2

@@ -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);

Stephen Hemminger

author

2008-07-21 19:31 UTC #17 lore raw

PATCH 4/6 ipv6: use spin_trylock_bh

In reply to Stephen Hemminger

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

Signed-off-by Stephen Hemminger <[email protected]>

net/ipv6/ip6_fib.c +1 -3

@@ -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;

Stephen Hemminger

author

2008-07-21 19:31 UTC #18 lore raw

PATCH 5/6 ipv6: use kcalloc

In reply to Stephen Hemminger

Th fib_table_hash is an array, so use kcalloc.

Signed-off-by Stephen Hemminger <[email protected]>

net/ipv6/ip6_fib.c +3 -3

@@ -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;

Stephen Hemminger

author

2008-07-21 19:31 UTC #19 lore raw

PATCH 6/6 ipv6: icmp6_dst_gc return change

In reply to Stephen Hemminger

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

Signed-off-by Stephen Hemminger <[email protected]>

include/net/ip6_route.h +1 -1

@@ -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);

net/ipv6/ip6_fib.c +1 -2

@@ -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);

net/ipv6/route.c +4 -6

@@ -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)