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

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

[Arjan van de Ven]

On Thu, 15 Nov 2007 04:01:48 GMT
Linux Kernel Mailing List <linux-kernel@vger.kernel.org> wrote:

> Gitweb:
> http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=d90bf5a976793edfa88d3bb2393f0231eb8ce1e5
> Commit:     d90bf5a976793edfa88d3bb2393f0231eb8ce1e5 Parent:
> 66ba886254edbbd9442d30f1eef6f6fb0145027d Author:     Eric Dumazet
> <dada1@cosmosbay.com> AuthorDate: Wed Nov 14 16:14:05 2007 -0800
> Committer:  David S. Miller <davem@davemloft.net>
> CommitDate: Wed Nov 14 16:14:05 2007 -0800
> 
>     [NET]: rt_check_expire() can take a long time, add a
> cond_resched() 
>     On commit 39c90ece7565f5c47110c2fa77409d7a9478bd5b:

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

int __sched cond_resched(void)
{
        if (need_resched() &&  .....

somehow I wonder why the second if() is useful at all; it's another
spot for a branch predictor to miss... and a void function call is
really really cheap... 


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

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

[Eric Dumazet]

Arjan van de Ven a écrit :
> On Thu, 15 Nov 2007 04:01:48 GMT
> Linux Kernel Mailing List <linux-kernel@vger.kernel.org> wrote:
> 
>> Gitweb:
>> http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=d90bf5a976793edfa88d3bb2393f0231eb8ce1e5
>> Commit:     d90bf5a976793edfa88d3bb2393f0231eb8ce1e5 Parent:
>> 66ba886254edbbd9442d30f1eef6f6fb0145027d Author:     Eric Dumazet
>> <dada1@cosmosbay.com> AuthorDate: Wed Nov 14 16:14:05 2007 -0800
>> Committer:  David S. Miller <davem@davemloft.net>
>> CommitDate: Wed Nov 14 16:14:05 2007 -0800
>>
>>     [NET]: rt_check_expire() can take a long time, add a
>> cond_resched() 
>>     On commit 39c90ece7565f5c47110c2fa77409d7a9478bd5b:
> 
>>     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. 
> 
> int __sched cond_resched(void)
> {
>         if (need_resched() &&  .....
> 
> somehow I wonder why the second if() is useful at all; it's another
> spot for a branch predictor to miss... and a void function call is
> really really cheap... 

Its not that cheap. The ChangeLog included my own numbers, on a Pentium M 
machine. (i686, 1.6 GHz, 1.5 GB ram)

Without "if (need_resched())" (so calling need_resched() X.XXX.XXX times), 
each run takes 88ms

With the extra check (and *much* less function calls), each run takes 25ms

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

[Eric Dumazet]

Eric Dumazet a écrit :
> Arjan van de Ven a écrit :
>> On Thu, 15 Nov 2007 04:01:48 GMT
>> Linux Kernel Mailing List <linux-kernel@vger.kernel.org> wrote:
>>
>>> Gitweb:
>>> http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=d90bf5a976793edfa88d3bb2393f0231eb8ce1e5 
>>>
>>> Commit:     d90bf5a976793edfa88d3bb2393f0231eb8ce1e5 Parent:
>>> 66ba886254edbbd9442d30f1eef6f6fb0145027d Author:     Eric Dumazet
>>> <dada1@cosmosbay.com> AuthorDate: Wed Nov 14 16:14:05 2007 -0800
>>> Committer:  David S. Miller <davem@davemloft.net>
>>> CommitDate: Wed Nov 14 16:14:05 2007 -0800
>>>
>>>     [NET]: rt_check_expire() can take a long time, add a
>>> cond_resched()     On commit 39c90ece7565f5c47110c2fa77409d7a9478bd5b:
>>
>>>     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. 
>>
>> int __sched cond_resched(void)
>> {
>>         if (need_resched() &&  .....
>>
>> somehow I wonder why the second if() is useful at all; it's another
>> spot for a branch predictor to miss... and a void function call is
>> really really cheap... 
> 
> Its not that cheap. The ChangeLog included my own numbers, on a Pentium 
> M machine. (i686, 1.6 GHz, 1.5 GB ram)
> 
> Without "if (need_resched())" (so calling need_resched() X.XXX.XXX 
> times), each run takes 88ms
> 
> With the extra check (and *much* less function calls), each run takes 25ms
> 

Looking at cond_resched(), I think the extra cost comes from
"mov %esp,%edx ; and $0xffffe000,%edx" (current_thread_info())

I dont have oprofile numbers yet, but I suspect CPU may have some delays
to compute this pointer value, since %esp is probably 'busy' because
of the preceding "call"

(In the case the "if (need_resched())" is done in rt_check_expire(),
compiler moves this pointer computation (current_thread_info()) out of the loop)

c055f926 <cond_resched>:
c055f926:       89 e2                   mov    %esp,%edx
c055f928:       81 e2 00 e0 ff ff       and    $0xffffe000,%edx
c055f92e:       8b 42 08                mov    0x8(%edx),%eax
c055f931:       a8 04                   test   $0x4,%al
c055f933:       74 1a                   je     c055f94f <cond_resched+0x29>
c055f935:       f6 42 17 10             testb  $0x10,0x17(%edx)
c055f939:       75 14                   jne    c055f94f <cond_resched+0x29>
c055f93b:       83 3d 00 80 7c c0 01    cmpl   $0x1,0xc07c8000
c055f942:       75 0b                   jne    c055f94f <cond_resched+0x29>
c055f944:       e8 2b 80 bb ff          call   c0117974 <__cond_resched>
c055f949:       b8 01 00 00 00          mov    $0x1,%eax
c055f94e:       c3                      ret
c055f94f:       31 c0                   xor    %eax,%eax
c055f951:       c3                      ret

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

[Arjan van de Ven]

On Fri, 16 Nov 2007 05:12:21 +0100
Eric Dumazet <dada1@cosmosbay.com> wrote:

> Eric Dumazet a écrit :
> > Arjan van de Ven a écrit :
> >> On Thu, 15 Nov 2007 04:01:48 GMT
> >> Linux Kernel Mailing List <linux-kernel@vger.kernel.org> wrote:
> >>
> >>> Gitweb:
> >>> http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=d90bf5a976793edfa88d3bb2393f0231eb8ce1e5 
> >>>
> >>> Commit:     d90bf5a976793edfa88d3bb2393f0231eb8ce1e5 Parent:
> >>> 66ba886254edbbd9442d30f1eef6f6fb0145027d Author:     Eric Dumazet
> >>> <dada1@cosmosbay.com> AuthorDate: Wed Nov 14 16:14:05 2007 -0800
> >>> Committer:  David S. Miller <davem@davemloft.net>
> >>> CommitDate: Wed Nov 14 16:14:05 2007 -0800
> >>>
> >>>     [NET]: rt_check_expire() can take a long time, add a
> >>> cond_resched()     On commit
> >>> 39c90ece7565f5c47110c2fa77409d7a9478bd5b:
> >>
> >>>     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. 
> >>
> >> int __sched cond_resched(void)
> >> {
> >>         if (need_resched() &&  .....
> >>
> >> somehow I wonder why the second if() is useful at all; it's another
> >> spot for a branch predictor to miss... and a void function call is
> >> really really cheap... 
> > 
> > Its not that cheap. The ChangeLog included my own numbers, on a
> > Pentium M machine. (i686, 1.6 GHz, 1.5 GB ram)
> > 
> > Without "if (need_resched())" (so calling need_resched() X.XXX.XXX 
> > times), each run takes 88ms
> > 
> > With the extra check (and *much* less function calls), each run
> > takes 25ms
> > 
> 
> Looking at cond_resched(), I think the extra cost comes from
> "mov %esp,%edx ; and $0xffffe000,%edx" (current_thread_info())
> 
> I dont have oprofile numbers yet, but I suspect CPU may have some
> delays to compute this pointer value, since %esp is probably 'busy'
> because of the preceding "call"

yeah the explicit reference makes the stack pointer tracking engine do a
commit I suspect which then also creates a data dependency in the code
flow.

however... this is likely a good argument for making cond_resched() as a
whole a #define (or inline) that does this test and then calls the out
of line code (which then doesn't need to retest, so it avoids the
double test)...




-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

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

[Andi Kleen]

Arjan van de Ven <arjan@infradead.org> writes:
>> > 
>> > Its not that cheap. The ChangeLog included my own numbers, on a
>> > Pentium M machine. (i686, 1.6 GHz, 1.5 GB ram)
>> > 
>> > Without "if (need_resched())" (so calling need_resched() X.XXX.XXX 
>> > times), each run takes 88ms
>> > 
>> > With the extra check (and *much* less function calls), each run
>> > takes 25ms

ms?!? The numbers sound wrong. Wrong unit? 

>> > 
>> 
>> Looking at cond_resched(), I think the extra cost comes from
>> "mov %esp,%edx ; and $0xffffe000,%edx" (current_thread_info())
>> 
>> I dont have oprofile numbers yet, but I suspect CPU may have some
>> delays to compute this pointer value, since %esp is probably 'busy'
>> because of the preceding "call"
>
> yeah the explicit reference makes the stack pointer tracking engine do a
> commit I suspect which then also creates a data dependency in the code
> flow.
>
> however... this is likely a good argument for making cond_resched() as a
> whole a #define (or inline) that does this test and then calls the out
> of line code (which then doesn't need to retest, so it avoids the
> double test)...

Disadvantage would be that might_sleep would be commonly skipped then
(unless you actually need to reschedule) 
But perhaps that's not a big issue.

-Andi

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

[Herbert Xu]

Andi Kleen <andi@firstfloor.org> wrote:
>
>>> > With the extra check (and *much* less function calls), each run
>>> > takes 25ms
> 
> ms?!? The numbers sound wrong. Wrong unit? 

That's quite possible with a huge routing cache.  I think that's
the reason Eric is doing this in the first place.

>> however... this is likely a good argument for making cond_resched() as a
>> whole a #define (or inline) that does this test and then calls the out
>> of line code (which then doesn't need to retest, so it avoids the
>> double test)...
> 
> Disadvantage would be that might_sleep would be commonly skipped then
> (unless you actually need to reschedule) 
> But perhaps that's not a big issue.

On a related note, would it be possible for cond_resched() to
be compiled out completely if we're doing real preemption as
opposed to voluntary preemption?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[David Miller]

From: Andi Kleen <andi@firstfloor.org>
Date: Sat, 17 Nov 2007 13:56:08 +0100

> Arjan van de Ven <arjan@infradead.org> writes:
> >> > 
> >> > Its not that cheap. The ChangeLog included my own numbers, on a
> >> > Pentium M machine. (i686, 1.6 GHz, 1.5 GB ram)
> >> > 
> >> > Without "if (need_resched())" (so calling need_resched() X.XXX.XXX 
> >> > times), each run takes 88ms
> >> > 
> >> > With the extra check (and *much* less function calls), each run
> >> > takes 25ms
> 
> ms?!? The numbers sound wrong. Wrong unit? 

Read what Eric is saying.  He is saying "any entire run" purging
the routing cache takes that long, not just one call.

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

[David Miller]

From: Arjan van de Ven <arjan@infradead.org>
Date: Thu, 15 Nov 2007 19:38:02 -0800

> On Thu, 15 Nov 2007 04:01:48 GMT
> Linux Kernel Mailing List <linux-kernel@vger.kernel.org> wrote:
> 
> >     Using a "if (need_resched())" test before calling
> > "cond_resched();" is necessary to avoid spending too much time doing
> > the resched check. 
> 
> int __sched cond_resched(void)
> {
>         if (need_resched() &&  .....
> 
> somehow I wonder why the second if() is useful at all; it's another
> spot for a branch predictor to miss... and a void function call is
> really really cheap... 

Not by Eric's tests.  Please read the thread, he checked and it's
50ms more expensive to make the function call.

Please, when you notice something interesting in something flying
by on git-web, go check thing out we probably discussed the thing
you're interested in.

And please at least CC: netdev about networking patches, and also the
patch author, which I've corrected in this reply.