[process scheduler] Possible bug in context_swich()?

[process scheduler] Possible bug in context_swich()?

[fabio de francesco]

Dear List,

My previous mails to you have gone unheard... my last chance...

In context_switch() (in linux/kernel/sched.c), starting with release 2.6.33, 
two "unlikely" macro  have been changed to "likely". I think the previous 
logic was right while the latter is wrong.

In case I am missing something I, please, ask someone to explain the above 
mentioned inversion of logic through releases.

Thanks in advance.

fabio

Re: [process scheduler] Possible bug in context_swich()?

[Will Newton]

On Wed, Sep 8, 2010 at 4:28 PM, fabio de francesco <fabio@metanix.org> wrote:
> Dear List,
>
> My previous mails to you have gone unheard... my last chance...
>
> In context_switch() (in linux/kernel/sched.c), starting with release 2.6.33,
> two "unlikely" macro  have been changed to "likely". I think the previous
> logic was right while the latter is wrong.
>
> In case I am missing something I, please, ask someone to explain the above
> mentioned inversion of logic through releases.

Hi fabio,

The logic was changed in commit
710390d90f143a9ebb87a475215140f426792efd on the basis of a branch hint
profiling run. It would guess previous logic was more likely to be
correct, unless one is often scheduling between kernel threads, but
maybe that happens more often than I expect.

I would suggest trying the branch tracer yourself if you think the
current code is wrong for your workload, the config option is
PROFILE_ANNOTATED_BRANCHES. I would suggest not profiling the boot
process (which will have predominantly kernel threads) and not
profiling on an idle system (which may also be relatively heavy on
kernel threads).

Re: [process scheduler] Possible bug in context_swich()?

[Peter Zijlstra]

On Wed, 2010-09-08 at 17:28 +0200, fabio de francesco wrote:

> In context_switch() (in linux/kernel/sched.c), starting with release 2.6.33, 
> two "unlikely" macro  have been changed to "likely". I think the previous 
> logic was right while the latter is wrong.
> 
> In case I am missing something I, please, ask someone to explain the above 
> mentioned inversion of logic through releases.

It helps if you CC people, LKML alone is a bit of a gamble.

git blame kernel/sched.c, will tell you that the change you refer to
comes from:

commit 710390d90f143a9ebb87a475215140f426792efd
Author: Tim Blechmann <tim@klingt.org>
Date:   Tue Nov 24 11:55:27 2009 +0100

    sched: Optimize branch hint in context_switch()
    
    Branch hint profiling on my nehalem machine showed over 90%
    incorrect branch hints:
    
      10420275 170645395  94 context_switch                 sched.c
       3043
      10408421 171098521  94 context_switch                 sched.c
       3050
    
    Signed-off-by: Tim Blechmann <tim@klingt.org>
    Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
    Cc: Mike Galbraith <efault@gmx.de>
    Cc: Paul Mackerras <paulus@samba.org>
    Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
    Cc: Frederic Weisbecker <fweisbec@gmail.com>
    LKML-Reference: <4B0BBB9F.6080304@klingt.org>
    Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/kernel/sched.c b/kernel/sched.c
index 93474a7..010d5e1 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -2829,14 +2829,14 @@ context_switch(struct rq *rq, struct task_struct *prev,
 	 */
 	arch_start_context_switch(prev);
 
-	if (unlikely(!mm)) {
+	if (likely(!mm)) {
 		next->active_mm = oldmm;
 		atomic_inc(&oldmm->mm_count);
 		enter_lazy_tlb(oldmm, next);
 	} else
 		switch_mm(oldmm, mm, next);
 
-	if (unlikely(!prev->mm)) {
+	if (likely(!prev->mm)) {
 		prev->active_mm = NULL;
 		rq->prev_mm = oldmm;
 	}

Re: [process scheduler] Possible bug in context_swich()?

[Mike Galbraith]

On Wed, 2010-09-08 at 17:54 +0200, Peter Zijlstra wrote:
> On Wed, 2010-09-08 at 17:28 +0200, fabio de francesco wrote:
> 
> > In context_switch() (in linux/kernel/sched.c), starting with release 2.6.33, 
> > two "unlikely" macro  have been changed to "likely". I think the previous 
> > logic was right while the latter is wrong.
> > 
> > In case I am missing something I, please, ask someone to explain the above 
> > mentioned inversion of logic through releases.
> 
> It helps if you CC people, LKML alone is a bit of a gamble.
> 
> git blame kernel/sched.c, will tell you that the change you refer to
> comes from:
> 
> commit 710390d90f143a9ebb87a475215140f426792efd
> Author: Tim Blechmann <tim@klingt.org>
> Date:   Tue Nov 24 11:55:27 2009 +0100
> 
>     sched: Optimize branch hint in context_switch()
>     
>     Branch hint profiling on my nehalem machine showed over 90%
>     incorrect branch hints:

That change never made any sense to me, seems Tim must have been
measuring a kthread load.  I benched at the time, and saw absolutely
zero difference one way or the other wrt max ctx rate on my Q6600.

	-Mike

Re: [process scheduler] Possible bug in context_swich()?

[Peter Zijlstra]

On Thu, 2010-09-09 at 04:32 +0200, Mike Galbraith wrote:
> On Wed, 2010-09-08 at 17:54 +0200, Peter Zijlstra wrote:
> > On Wed, 2010-09-08 at 17:28 +0200, fabio de francesco wrote:
> > 
> > > In context_switch() (in linux/kernel/sched.c), starting with release 2.6.33, 
> > > two "unlikely" macro  have been changed to "likely". I think the previous 
> > > logic was right while the latter is wrong.
> > > 
> > > In case I am missing something I, please, ask someone to explain the above 
> > > mentioned inversion of logic through releases.
> > 
> > It helps if you CC people, LKML alone is a bit of a gamble.
> > 
> > git blame kernel/sched.c, will tell you that the change you refer to
> > comes from:
> > 
> > commit 710390d90f143a9ebb87a475215140f426792efd
> > Author: Tim Blechmann <tim@klingt.org>
> > Date:   Tue Nov 24 11:55:27 2009 +0100
> > 
> >     sched: Optimize branch hint in context_switch()
> >     
> >     Branch hint profiling on my nehalem machine showed over 90%
> >     incorrect branch hints:
> 
> That change never made any sense to me, seems Tim must have been
> measuring a kthread load.  I benched at the time, and saw absolutely
> zero difference one way or the other wrt max ctx rate on my Q6600.

One option is to simply remove the whole branch hint.. But lets ask Tim
what kind of workload he used..

Re: [process scheduler] Possible bug in context_swich()?

[Tim Blechmann]

[-- Attachment #1: Type: Text/Plain, Size: 1737 bytes --]

On Thursday, September 09, 2010 12:39:06 pm Peter Zijlstra wrote:
> On Thu, 2010-09-09 at 04:32 +0200, Mike Galbraith wrote:
> > On Wed, 2010-09-08 at 17:54 +0200, Peter Zijlstra wrote:
> > > On Wed, 2010-09-08 at 17:28 +0200, fabio de francesco wrote:
> > > > In context_switch() (in linux/kernel/sched.c), starting with release
> > > > 2.6.33, two "unlikely" macro  have been changed to "likely". I think
> > > > the previous logic was right while the latter is wrong.
> > > > 
> > > > In case I am missing something I, please, ask someone to explain the
> > > > above mentioned inversion of logic through releases.
> > > 
> > > It helps if you CC people, LKML alone is a bit of a gamble.
> > > 
> > > git blame kernel/sched.c, will tell you that the change you refer to
> > > comes from:
> > > 
> > > commit 710390d90f143a9ebb87a475215140f426792efd
> > > Author: Tim Blechmann <tim@klingt.org>
> > > Date:   Tue Nov 24 11:55:27 2009 +0100
> > > 
> > >     sched: Optimize branch hint in context_switch()
> > >     
> > >     Branch hint profiling on my nehalem machine showed over 90%
> > 
> > >     incorrect branch hints:
> > That change never made any sense to me, seems Tim must have been
> > measuring a kthread load.  I benched at the time, and saw absolutely
> > zero difference one way or the other wrt max ctx rate on my Q6600.
> 
> One option is to simply remove the whole branch hint.. But lets ask Tim
> what kind of workload he used..

i was using a standard desktop workload, nothing special ...

-- 
tim@klingt.org
http://tim.klingt.org

Wherever we are, what we hear is mostly noise. When we ignore it, it
disturbs us. When we listen to it, we find it fascinating.
  John Cage

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [process scheduler] Possible bug in context_swich()?

[Heiko Carstens]

On Thu, Sep 09, 2010 at 01:12:18PM +0200, Tim Blechmann wrote:
> On Thursday, September 09, 2010 12:39:06 pm Peter Zijlstra wrote:
> > On Thu, 2010-09-09 at 04:32 +0200, Mike Galbraith wrote:
> > > On Wed, 2010-09-08 at 17:54 +0200, Peter Zijlstra wrote:
> > > > On Wed, 2010-09-08 at 17:28 +0200, fabio de francesco wrote:
> > > > > In context_switch() (in linux/kernel/sched.c), starting with release
> > > > > 2.6.33, two "unlikely" macro  have been changed to "likely". I think
> > > > > the previous logic was right while the latter is wrong.
> > > > > 
> > > > > In case I am missing something I, please, ask someone to explain the
> > > > > above mentioned inversion of logic through releases.
> > > > 
> > > > It helps if you CC people, LKML alone is a bit of a gamble.
> > > > 
> > > > git blame kernel/sched.c, will tell you that the change you refer to
> > > > comes from:
> > > > 
> > > > commit 710390d90f143a9ebb87a475215140f426792efd
> > > > Author: Tim Blechmann <tim@klingt.org>
> > > > Date:   Tue Nov 24 11:55:27 2009 +0100
> > > > 
> > > >     sched: Optimize branch hint in context_switch()
> > > >     
> > > >     Branch hint profiling on my nehalem machine showed over 90%
> > > 
> > > >     incorrect branch hints:
> > > That change never made any sense to me, seems Tim must have been
> > > measuring a kthread load.  I benched at the time, and saw absolutely
> > > zero difference one way or the other wrt max ctx rate on my Q6600.
> > 
> > One option is to simply remove the whole branch hint.. But lets ask Tim
> > what kind of workload he used..
> 
> i was using a standard desktop workload, nothing special ...

Just out of curiosity I tried this with two kernel builds on a six way
machine with latest git head and got these numbers:

 correct incorrect  %        Function                  File              Line
 ------- ---------  -        --------                  ----              ----
  644387   484258  42 context_switch                 sched.c              2851
  677534   451199  39 context_switch                 sched.c              2858

So the hints seem to be rather pointless.

Re: [process scheduler] Possible bug in context_swich()?

[fabio de francesco]

On Thursday 09 September 2010 13:12:18 Tim Blechmann wrote:
> On Thursday, September 09, 2010 12:39:06 pm Peter Zijlstra wrote:
> > On Thu, 2010-09-09 at 04:32 +0200, Mike Galbraith wrote:
> > > On Wed, 2010-09-08 at 17:54 +0200, Peter Zijlstra wrote:
> > > > On Wed, 2010-09-08 at 17:28 +0200, fabio de francesco wrote:
> > > > > In context_switch() (in linux/kernel/sched.c), starting with
> > > > > release 2.6.33, two "unlikely" macro  have been changed to
> > > > > "likely". I think the previous logic was right while the latter is
> > > > > wrong.
> > > > > 
> > > > > In case I am missing something I, please, ask someone to explain
> > > > > the above mentioned inversion of logic through releases.
> > > > 
> > > > It helps if you CC people, LKML alone is a bit of a gamble.
> > > > 
> > > > git blame kernel/sched.c, will tell you that the change you refer to
> > > > comes from:
> > > > 
> > > > commit 710390d90f143a9ebb87a475215140f426792efd
> > > > Author: Tim Blechmann <tim@klingt.org>
> > > > Date:   Tue Nov 24 11:55:27 2009 +0100
> > > > 
> > > >     sched: Optimize branch hint in context_switch()
> > > >     
> > > >     Branch hint profiling on my nehalem machine showed over 90%
> > > 
> > > >     incorrect branch hints:
> > > That change never made any sense to me, seems Tim must have been
> > > measuring a kthread load.  I benched at the time, and saw absolutely
> > > zero difference one way or the other wrt max ctx rate on my Q6600.
> > 
> > One option is to simply remove the whole branch hint.. But lets ask Tim
> > what kind of workload he used..
> 
> i was using a standard desktop workload, nothing special ...

There could have been just kernel threads ready to run... Could a server or 
workstation workload behave differently?

fabio