Skip to contentSkip to conversation

NET : rt_check_expire() can take a long time, add a cond_resched()

PATCH In review Eric Dumazet 4 participants 11 messages Nov 14, 2007 – Nov 18, 2007

Subsystems (3)
Networking Network drivers IPv4/IPv6
lore
Share & tools
lore

Eric Dumazet

author
#0 lore raw

PATCH 1/1 NET : rt_check_expire() can take a long time, add a cond_resched()

On commit 39c90ece7565f5c47110c2fa77409d7a9478bd5b we converted rt_check_expire() from softirq to workqueue, allowing the function to perform all work it was supposed to do.

When the IP route cache is big, rt_check_expire() can take a long time to run. (default settings : 20% of the hash table is scanned at each invocation)

Adding cond_resched() helps giving cpu to higher priority tasks if necessary.

Using a "if (need_resched())" test before calling "cond_resched();" is necessary to avoid spending too much time doing the resched check. (My tests gave a time reduction from 88 ms to 25 ms per rt_check_expire() run on my i686 test machine)

Signed-off-by Eric Dumazet <[email protected]>
net/ipv4/route.c +3 -0
index a9471b7..f0b28f9 100644
@@ -580,6 +580,9 @@ static void rt_check_expire(struct work_struct *work)
 		i = (i + 1) & rt_hash_mask;
 		rthp = &rt_hash_table[i].chain;
 
+		if (need_resched())
+			cond_resched();
+
 		if (*rthp == NULL)
 			continue;
 		spin_lock_bh(rt_hash_lock_addr(i));
maintainer
#1 lore raw

From: Eric Dumazet <[email protected]>
Date: Wed, 14 Nov 2007 22:34:13 +0100

Quoted 7 lines

On commit 39c90ece7565f5c47110c2fa77409d7a9478bd5b we converted rt_check_expire() from softirq to workqueue, allowing the function to perform all work it was supposed to do.

When the IP route cache is big, rt_check_expire() can take a long time to run. (default settings : 20% of the hash table is scanned at each invocation)

Quoted 8 lines

Adding cond_resched() helps giving cpu to higher priority tasks if necessary.

Using a "if (need_resched())" test before calling "cond_resched();" is necessary to avoid spending too much time doing the resched check. (My tests gave a time reduction from 88 ms to 25 ms per rt_check_expire() run on my i686 test machine)

Signed-off-by Eric Dumazet <[email protected]>

Patch applied, thanks!

#2 lore raw
In reply to Eric Dumazet
Quoted 3 lines · Eric Dumazet

Using a "if (need_resched())" test before calling "cond_resched();" is necessary to avoid spending too much time doing the resched check.

The only difference between cond_resched() and if (need_resched()) cond_resched() is one function call less and one might_sleep less. If the might_sleep or the function call are really problems (did you measure it? -- i doubt it somewhat) then it would be better to fix the generic code to either inline that or supply a __cond_resched() without might_sleep.

A cheaper change might have been to just limit the number of buckets scanned.

-Andi

#3 lore raw
Quoted 3 lines · Andi Kleen

A cheaper change might have been to just limit the number of buckets scanned.

Actually the whole point of moving it out to process context is so that we don't have to worry about keeping track of the number of buckets since deciding on how many buckets to process is black magic.

IMHO preemption is the answer :) But failing that, a resched is the next best thing.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#4 lore raw
In reply to Andi Kleen
Quoted 10 lines · Andi Kleen
Quoted 2 lines · Eric Dumazet

Using a "if (need_resched())" test before calling "cond_resched();" is necessary to avoid spending too much time doing the resched check.

The only difference between cond_resched() and if (need_resched()) cond_resched() is one function call less and one might_sleep less. If the might_sleep or the function call are really problems (did you measure it? -- i doubt it somewhat) then it would be better to fix the generic code to either inline that or supply a __cond_resched() without might_sleep.

Please note that :

if (need_resched())

    cond_resched();

will re-test need_resched() once cond_resched() is called.

So it may sound unnecessary but in the rt_check_expire() case, with a loop potentially doing XXX.XXX iterations, being able to bypass the function call is a clear win (in my bench case, 25 ms instead of 88 ms). Impact on I-cache is irrelevant here as this rt_check_expires() runs once every 60 sec.

I think the actual cond_resched() is fine for other uses in the kernel, that are not used in a loop : In the general case, kernel text size should be as small as possible to reduce I-cache pressure, so a function call is better than an inline.

Quoted 4 lines

A cheaper change might have been to just limit the number of buckets scanned.

Well, not in some particular cases, when there are 3 millions of routes for example in the cache. We really want to scan/free them eventually :)

An admin already has the possibility to tune /proc/sys/net/ipv4/route/gc_interval and /proc/sys/net/ipv4/route/gc_timeout, so on a big cache, it will probably set gc_interval to 1 instead of 60

Next step will be to move "ip route flush cache" and rt_secret_rebuild() handling from softirq to process context too, since this still can kill a machine.

maintainer
#5 lore raw

From: Eric Dumazet <[email protected]>
Date: Thu, 15 Nov 2007 09:25:59 +0100

Quoted 11 lines

Please note that :

if (need_resched())

     cond_resched();

will re-test need_resched() once cond_resched() is called.

So it may sound unnecessary but in the rt_check_expire() case, with a loop potentially doing XXX.XXX iterations, being able to bypass the function call is a clear win (in my bench case, 25 ms instead of 88 ms). Impact on I-cache is irrelevant here as this rt_check_expires() runs once every 60 sec.

BTW, Eric, initially I was going to recommend that you do something like:

	if ((goal % SOME_POWER_OF_2) == 0)
		cond_resched();

to mitigate this cost but decided it wasn't worth the bother.

#6 lore raw
In reply to Eric Dumazet
Quoted 4 lines · Eric Dumazet

So it may sound unnecessary but in the rt_check_expire() case, with a loop potentially doing XXX.XXX iterations, being able to bypass the function call is a clear win (in my bench case, 25 ms instead of 88 ms). Impact on I-cache is irrelevant here as this rt_check_expires()

Measuring what? And really milli-seconds? The number does not sound plausible to me.

-Andi

author
#7 lore raw
Quoted 8 lines · Andi Kleen
Quoted 4 lines · Eric Dumazet

So it may sound unnecessary but in the rt_check_expire() case, with a loop potentially doing XXX.XXX iterations, being able to bypass the function call is a clear win (in my bench case, 25 ms instead of 88 ms). Impact on I-cache is irrelevant here as this rt_check_expires()

Measuring what? And really milli-seconds? The number does not sound plausible to me.

You know Andi, I have seen production servers that needed several seconds to perform the flush. When you have millions of entries on this table, can you imagine the number of memory transactions (including atomic ops) needed to flush them all ?

The 25.000.000 ns and 88.000.000 ns numbers where on an empty table, but large (16 MB of memory)

#8 lore raw
Quoted 2 lines

The 25.000.000 ns and 88.000.000 ns numbers where on an empty table, but large (16 MB of memory)

This would mean that cond_resched() needs ~4x as much time as checking an empty bucket. I find that somewhat hard to believe.

-Andi

maintainer
#9 lore raw

From: Andi Kleen <[email protected]>
Date: Sat, 17 Nov 2007 22:46:40 +0100

Quoted 5 lines
Quoted 2 lines

The 25.000.000 ns and 88.000.000 ns numbers where on an empty table, but large (16 MB of memory)

This would mean that cond_resched() needs ~4x as much time as checking an empty bucket. I find that somewhat hard to believe.

Based upon Arjan's analysis of the stall created by the cond_resched() assembler, this doesn't surprise me.

I also don't believe Eric would make up such numbers or measure them carelessly and then present them as fact.

maintainer
#10 lore raw
In reply to Andi Kleen

From: Andi Kleen <[email protected]>
Date: Thu, 15 Nov 2007 08:30:16 +0100

Quoted 12 lines
Quoted 3 lines · Eric Dumazet

Using a "if (need_resched())" test before calling "cond_resched();" is necessary to avoid spending too much time doing the resched check.

The only difference between cond_resched() and if (need_resched()) cond_resched() is one function call less and one might_sleep less. If the might_sleep or the function call are really problems (did you measure it? -- i doubt it somewhat) then it would be better to fix the generic code to either inline that or supply a __cond_resched() without might_sleep.

Quoted 2 lines

A cheaper change might have been to just limit the number of buckets scanned.

Fix up unmap_vmas() too if this is done as it does a similar need_resched() check too.