| Message ID | 4A138CFE.5070901@cosmosbay.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Eric Dumazet <dada1@cosmosbay.com> Date: Wed, 20 May 2009 06:54:22 +0200 > I would like to split patches because we have two bugs indeed, and > I prefer to get attention for both problems, I dont remember Neil acknowledged > the length computation problem. > > First and small patch, candidate for net-2.6 and stable (for 2.6.29) : > > Thank you > > [PATCH] net: fix length computation in rt_check_expire() > > rt_check_expire() computes average and standard deviation of chain lengths, > but not correclty reset length to 0 at beginning of each chain. > This probably gives overflows for sum2 (and sum) on loaded machines instead > of meaningful results. > > Signed-off-by: Eric Dumazet <dada1@cosmosbay.com> Yes I definitely agree this length initialization is a bug. I'll integrate this fix soon, thanks Eric. You can send me the other part of the bug fix relative to this if you like. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 20, 2009 at 06:54:22AM +0200, Eric Dumazet wrote: > David Miller a écrit : > > From: Neil Horman <nhorman@tuxdriver.com> > > Date: Tue, 19 May 2009 15:24:50 -0400 > > > >>> Moving whole group in front would defeat the purpose of move, actually, > >>> since rank in chain is used to decay the timeout in garbage collector. > >>> (search for tmo >>= 1; ) > >>> > >> Argh, so the list is implicitly ordered by expiration time. That > >> really defeats the entire purpose of doing grouping in the ilst at > >> all. If thats the case, then I agree, its probably better to to > >> take the additional visitation hit in in check_expire above than to > >> try and preserve ordering. > > > > Yes, this seems best. > > > > I was worried that somehow the ordering also influences lookups, > > because the TOS bits don't go into the hash so I worried that it would > > be important that explicit TOS values appear before wildcard ones. > > But it doesn't appear that this is an issue, we don't have wildcard > > TOSs in the rtable entries, they are always explicit. > > > > So I would like to see an explicit final patch from Eric so we can get > > this fixed now. > > > > I would like to split patches because we have two bugs indeed, and > I prefer to get attention for both problems, I dont remember Neil acknowledged > the length computation problem. > > First and small patch, candidate for net-2.6 and stable (for 2.6.29) : > > Thank you > > [PATCH] net: fix length computation in rt_check_expire() > > rt_check_expire() computes average and standard deviation of chain lengths, > but not correclty reset length to 0 at beginning of each chain. > This probably gives overflows for sum2 (and sum) on loaded machines instead > of meaningful results. > > Signed-off-by: Eric Dumazet <dada1@cosmosbay.com> Acked-by: Neil Horman <nhorman@tuxdriver.com> > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Neil Horman <nhorman@tuxdriver.com> Date: Wed, 20 May 2009 06:27:54 -0400 > On Wed, May 20, 2009 at 06:54:22AM +0200, Eric Dumazet wrote: >> [PATCH] net: fix length computation in rt_check_expire() >> >> rt_check_expire() computes average and standard deviation of chain lengths, >> but not correclty reset length to 0 at beginning of each chain. >> This probably gives overflows for sum2 (and sum) on loaded machines instead >> of meaningful results. >> >> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com> > Acked-by: Neil Horman <nhorman@tuxdriver.com> > Applied and queued up for -stable. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/route.c b/net/ipv4/route.c index c4c60e9..869cf1c 100644 --- a/net/ipv4/route.c +++ b/net/ipv4/route.c @@ -785,7 +785,7 @@ static void rt_check_expire(void) static unsigned int rover; unsigned int i = rover, goal; struct rtable *rth, **rthp; - unsigned long length = 0, samples = 0; + unsigned long samples = 0; unsigned long sum = 0, sum2 = 0; u64 mult; @@ -795,9 +795,9 @@ static void rt_check_expire(void) goal = (unsigned int)mult; if (goal > rt_hash_mask) goal = rt_hash_mask + 1; - length = 0; for (; goal > 0; goal--) { unsigned long tmo = ip_rt_gc_timeout; + unsigned long length; i = (i + 1) & rt_hash_mask; rthp = &rt_hash_table[i].chain; @@ -809,6 +809,7 @@ static void rt_check_expire(void) if (*rthp == NULL) continue; + length = 0; spin_lock_bh(rt_hash_lock_addr(i)); while ((rth = *rthp) != NULL) { if (rt_is_expired(rth)) {