Patchwork net: fix length computation in rt_check_expire()

login
register
mail settings
Submitter Eric Dumazet
Date May 20, 2009, 4:54 a.m.
Message ID <4A138CFE.5070901@cosmosbay.com>
Download mbox | patch
Permalink /patch/27441/
State Accepted
Delegated to: David Miller
Headers show

Comments

Eric Dumazet - May 20, 2009, 4:54 a.m.
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>
---
 net/ipv4/route.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)


--
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
David Miller - May 20, 2009, 6:13 a.m.
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
Neil Horman - May 20, 2009, 10:27 a.m.
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
David Miller - May 21, 2009, 12:19 a.m.
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

Patch

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