net/ipv4/route.c GC patch: is this insane?

net/ipv4/route.c GC patch: is this insane?

[Daniel Stone]

[-- Attachment #1.1: Type: text/plain, Size: 1090 bytes --]

Hi guys,
Attached is a patch to net/ipv4/route.c (against 2.4.23, sorry) to
combat the 'dst cache full' issue. Essentially, there's a machine that I
have access to (a router) that will report 'dst cache full', and
immediately cease dealing with any IPv4 traffic. I found the attached
patch against 2.0, and forward-ported it to 2.4. As this issue only
crops up randomly every couple of weeks, I can't tell you whether it's
worked or not. All I know is that it hasn't eaten my firstborn so far:
given a quick spin, it seemed to work OK, but whether or not it solves
the problem is a different matter.

I also have /proc/net dumps of the machine from before and after it
died, if anyone wants them.

So, is this patch at all sane?

Please CC me: I don't subscribe.

Cheers!
:) d (to borrow from Michael M: 'hacker, iterant idiot')

-- 
Daniel Stone                                              <daniel@fooishbar.org>
"The programs are documented fully by _The Rise and Fall of a Fooish Bar_,
available by the Info system." -- debian/manpage.sgml.ex, dh_make template

[-- Attachment #1.2: route-fix-gc.diff --]
[-- Type: text/plain, Size: 2105 bytes --]

--- net/ipv4/route.c.orig	2004-05-11 23:56:10.000000000 +1000
+++ net/ipv4/route.c	2004-05-27 16:16:23.000000000 +1000
@@ -101,7 +101,7 @@
 
 #define IP_MAX_MTU	0xFFF0
 
-#define RT_GC_TIMEOUT (300*HZ)
+#define RT_GC_TIMEOUT (120*HZ)
 
 int ip_rt_min_delay		= 2 * HZ;
 int ip_rt_max_delay		= 10 * HZ;
@@ -138,7 +138,8 @@
 static struct dst_entry *ipv4_negative_advice(struct dst_entry *dst);
 static void		 ipv4_link_failure(struct sk_buff *skb);
 static int rt_garbage_collect(void);
-
+static int rt_garbage_docollect(void);
+static int rt_delete_now(void);
 
 struct dst_ops ipv4_dst_ops = {
 	family:			AF_INET,
@@ -526,7 +527,7 @@
    and when load increases it reduces to limit cache size.
  */
 
-static int rt_garbage_collect(void)
+static int rt_garbage_docollect(void)
 {
 	static unsigned long expire = RT_GC_TIMEOUT;
 	static unsigned long last_gc;
@@ -630,8 +631,11 @@
 
 	if (atomic_read(&ipv4_dst_ops.entries) < ip_rt_max_size)
 		goto out;
+	/*
+	 * don't bitch, just silently attempt to correct
 	if (net_ratelimit())
 		printk(KERN_WARNING "dst cache overflow\n");
+	*/
 	rt_cache_stat[smp_processor_id()].gc_dst_overflow++;
 	return 1;
 
@@ -646,6 +650,40 @@
 #endif
 out:	return 0;
 }
+static int rt_delete_now(void){
+	struct rtable *rth, **rthp;
+	int i = 0, ent1 = 0, ent2 = 0, c = 0;
+
+	ent1 = atomic_read(&ipv4_dst_ops.entries);
+	local_bh_disable();
+	while (i < rt_hash_mask) {
+		rthp = &(rt_hash_table[i].chain);
+		while ((rth = *rthp) != NULL) {
+			*rthp = rth->u.rt_next;
+			rth->u.rt_next = NULL;
+			c++;
+			rt_free(rth);
+		}
+		i++;
+	}
+
+	atomic_set(&ipv4_dst_ops.entries, 0);
+	local_bh_enable();
+	ent2 = atomic_read(&ipv4_dst_ops.entries);
+
+	if (net_ratelimit()){
+		printk("dst cache overflow\n");
+		printk("rt_delete_now(): s:%d e:%d t:%d\n", ent1, ent2, c);
+	}
+	
+	return 0;
+}
+
+static int rt_garbage_collect(void){
+	if (rt_garbage_docollect())
+		rt_delete_now();
+	return 0;
+}
 
 static int rt_intern_hash(unsigned hash, struct rtable *rt, struct rtable **rp)
 {

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

net/ipv4/route.c GC patch: is this insane?

[Robert Olsson]

Daniel Stone writes:
 > Hi guys,
 > Attached is a patch to net/ipv4/route.c (against 2.4.23, sorry) to
 > combat the 'dst cache full' issue. Essentially, there's a machine that I
 > have access to (a router) that will report 'dst cache full', and
 > immediately cease dealing with any IPv4 traffic. I found the attached
 > patch against 2.0, and forward-ported it to 2.4. As this issue only
 > crops up randomly every couple of weeks, I can't tell you whether it's
 > worked or not. All I know is that it hasn't eaten my firstborn so far:
 > given a quick spin, it seemed to work OK, but whether or not it solves
 > the problem is a different matter.

 Hello!

 If your traffic is sane and you got dst cache overflow you should 
 probably try to reduce gc_min_interval so your GC can release a 
 higher numbers of dst entries per second. Flushing active entries
 means that they are created again. This decreases your throughput
 maybe just enough not to reach the limit where you reach overflow.


 > +	atomic_set(&ipv4_dst_ops.entries, 0);

 These counters should only be handled by the freeing process rt_free etc.

 Cheers.
						--ro

Re: net/ipv4/route.c GC patch: is this insane?b4Db3tL

[Daniel Stone]

[-- Attachment #1: Type: text/plain, Size: 1592 bytes --]

On Mon, May 31, 2004 at 02:21:02PM +0200, Robert Olsson wrote:
> Daniel Stone writes:
>  > Hi guys,
>  > Attached is a patch to net/ipv4/route.c (against 2.4.23, sorry) to
>  > combat the 'dst cache full' issue. Essentially, there's a machine that I
>  > have access to (a router) that will report 'dst cache full', and
>  > immediately cease dealing with any IPv4 traffic. I found the attached
>  > patch against 2.0, and forward-ported it to 2.4. As this issue only
>  > crops up randomly every couple of weeks, I can't tell you whether it's
>  > worked or not. All I know is that it hasn't eaten my firstborn so far:
>  > given a quick spin, it seemed to work OK, but whether or not it solves
>  > the problem is a different matter.
> 
>  Hello!
> 
>  If your traffic is sane and you got dst cache overflow you should 
>  probably try to reduce gc_min_interval so your GC can release a 
>  higher numbers of dst entries per second. Flushing active entries
>  means that they are created again. This decreases your throughput
>  maybe just enough not to reach the limit where you reach overflow.

gc_min_interval was zero - should this increase, or what?.

>  > +	atomic_set(&ipv4_dst_ops.entries, 0);
> 
>  These counters should only be handled by the freeing process rt_free etc.

OK.

Thanks for the pointers!
:) d

-- 
Daniel Stone                                              <daniel@fooishbar.org>
"The programs are documented fully by _The Rise and Fall of a Fooish Bar_,
available by the Info system." -- debian/manpage.sgml.ex, dh_make template

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: net/ipv4/route.c GC patch: is this insane?b4Db3tL

[Robert Olsson]

Daniel Stone writes:

 > gc_min_interval was zero - should this increase, or what?.

 If I remember correctly this was an artifact when used by /proc. Try to 
 set this value via source and avoid /proc for this. In 2.6 it's something 
 like:

 int ip_rt_gc_min_interval	= HZ / 2;

 Cheers.
						--ro

Re: net/ipv4/route.c GC patch: is this insane?

[Daniel Stone]

[-- Attachment #1: Type: text/plain, Size: 692 bytes --]

On Mon, May 31, 2004 at 02:43:33PM +0200, Robert Olsson wrote:
> Daniel Stone writes:
>  > gc_min_interval was zero - should this increase, or what?.
> 
>  If I remember correctly this was an artifact when used by /proc. Try to 
>  set this value via source and avoid /proc for this. In 2.6 it's something 
>  like:
> 
>  int ip_rt_gc_min_interval	= HZ / 2;

This appears to already be the case, in 2.4.23's net/ipv4/route.c, at
any rate.

-- 
Daniel Stone                                              <daniel@fooishbar.org>
"The programs are documented fully by _The Rise and Fall of a Fooish Bar_,
available by the Info system." -- debian/manpage.sgml.ex, dh_make template

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: net/ipv4/route.c GC patch: is this insane?

[Robert Olsson]

Daniel Stone writes:

 > >  int ip_rt_gc_min_interval	= HZ / 2;
 > 
 > This appears to already be the case, in 2.4.23's net/ipv4/route.c, at
 > any rate.

 OK! 

 Well take a look at route stats /proc/net/rt_cache_stat (you can use 
 rtstat app with a vmstat-like output) to ensure things are normal.
 If so you can try to decrease ip_rt_gc_min_interval further. 
 
 Cheers.
						--ro