[PATCH 1/5] process_64: remove branch hint

[PATCH 1/5] process_64: remove branch hint

[Tim Blechmann]

[-- Attachment #1: Type: text/plain, Size: 1009 bytes --]

branch hint profiling of my nehalem machine, showed 96% incorrect branch
hints:

 6548732 174664120  96 __switch_to                    process_64.c
   406
 6548745 174565593  96 __switch_to                    process_64.c
   410

Signed-off-by: Tim Blechmann <tim@klingt.org>
---
 arch/x86/kernel/process_64.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index c8d0ece..1e2e1fe 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -403,11 +403,11 @@ __switch_to(struct task_struct *prev_p, struct
task_struct *next_p)
 	 * This won't pick up thread selector changes, but I guess that is ok.
 	 */
 	savesegment(es, prev->es);
-	if (unlikely(next->es | prev->es))
+	if (next->es | prev->es)
 		loadsegment(es, next->es);
  	savesegment(ds, prev->ds);
-	if (unlikely(next->ds | prev->ds))
+	if (next->ds | prev->ds)
 		loadsegment(ds, next->ds);
  -- 1.6.4.2



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 2/5] sched.c: change branch hint

[Tim Blechmann]

[-- Attachment #1: Type: text/plain, Size: 935 bytes --]

branch hint profiling of 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>
---
 kernel/sched.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/sched.c b/kernel/sched.c
index c78dcfe..2a78b38 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -3040,14 +3040,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;
 	}
-- 
1.6.4.2



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 3/5] slab.c: remove branch hint

[Tim Blechmann]

[-- Attachment #1: Type: text/plain, Size: 692 bytes --]

branch profiling on my nehalem machine showed 99% incorrect branch hints:

   28459  7678524  99 __cache_alloc_node             slab.c
  3551

Signed-off-by: Tim Blechmann <tim@klingt.org>
---
 mm/slab.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index f70b326..4125fcd 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3548,7 +3548,7 @@ __cache_alloc_node(struct kmem_cache *cachep,
gfp_t flags, int nodeid,
 	slab_irq_save(save_flags, this_cpu);
  	this_node = cpu_to_node(this_cpu);
-	if (unlikely(nodeid == -1))
+	if (nodeid == -1)
 		nodeid = this_node;
  	if (unlikely(!cachep->nodelists[nodeid])) {
-- 
1.6.4.2



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 4/5] sched_fair.c: remove branch hint

[Tim Blechmann]

[-- Attachment #1: Type: text/plain, Size: 703 bytes --]

branch hint profiling of my nehalem machine, showed 90% incorrect
branch hints:

15728471 158903754  90 pick_next_task_fair            sched_fair.c
   1555

Signed-off-by: Tim Blechmann <tim@klingt.org>
---
 kernel/sched_fair.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
index e5945df..5a0965c 100644
--- a/kernel/sched_fair.c
+++ b/kernel/sched_fair.c
@@ -1552,7 +1552,7 @@ static struct task_struct
*pick_next_task_fair(struct rq *rq)
 	struct cfs_rq *cfs_rq = &rq->cfs;
 	struct sched_entity *se;
 -	if (unlikely(!cfs_rq->nr_running))
+	if (!cfs_rq->nr_running)
 		return NULL;
  	do {
-- 
1.6.4.2



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 5/5] workqueue.c: remove branch hint

[Tim Blechmann]

[-- Attachment #1: Type: text/plain, Size: 735 bytes --]

branch hint profiling of my nehalem machine, showed 96% incorrect branch
hints:

131609  3840434  96 queue_delayed_work_on          workqueue.c          258

Signed-off-by: Tim Blechmann <tim@klingt.org>
---
 kernel/workqueue.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 0a98bef..6985ab0 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -255,7 +255,7 @@ int queue_delayed_work_on(int cpu, struct
workqueue_struct *wq,
 		timer->data = (unsigned long)dwork;
 		timer->function = delayed_work_timer_fn;
 -		if (unlikely(cpu >= 0))
+		if (cpu >= 0)
 			add_timer_on(timer, cpu);
 		else
 			add_timer(timer);
-- 
1.6.4.2



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 3/5] slab.c: remove branch hint

[Ingo Molnar]

(Pekka Cc:-ed)

* Tim Blechmann <tim@klingt.org> wrote:

> branch profiling on my nehalem machine showed 99% incorrect branch hints:
> 
>    28459  7678524  99 __cache_alloc_node             slab.c
>   3551
> 
> Signed-off-by: Tim Blechmann <tim@klingt.org>
> ---
>  mm/slab.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index f70b326..4125fcd 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -3548,7 +3548,7 @@ __cache_alloc_node(struct kmem_cache *cachep,
> gfp_t flags, int nodeid,
>  	slab_irq_save(save_flags, this_cpu);
>   	this_node = cpu_to_node(this_cpu);
> -	if (unlikely(nodeid == -1))
> +	if (nodeid == -1)
>  		nodeid = this_node;
>   	if (unlikely(!cachep->nodelists[nodeid])) {
> -- 
> 1.6.4.2
> 
> 

Re: [PATCH 3/5] slab.c: remove branch hint

[Pekka Enberg]

On Tue, Nov 24, 2009 at 1:20 PM, Ingo Molnar <mingo@elte.hu> wrote:
> (Pekka Cc:-ed)
>
> * Tim Blechmann <tim@klingt.org> wrote:
>
>> branch profiling on my nehalem machine showed 99% incorrect branch hints:
>>
>>    28459  7678524  99 __cache_alloc_node             slab.c
>>   3551
>>
>> Signed-off-by: Tim Blechmann <tim@klingt.org>
>> ---
>>  mm/slab.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/mm/slab.c b/mm/slab.c
>> index f70b326..4125fcd 100644
>> --- a/mm/slab.c
>> +++ b/mm/slab.c
>> @@ -3548,7 +3548,7 @@ __cache_alloc_node(struct kmem_cache *cachep,
>> gfp_t flags, int nodeid,
>>       slab_irq_save(save_flags, this_cpu);
>>       this_node = cpu_to_node(this_cpu);
>> -     if (unlikely(nodeid == -1))
>> +     if (nodeid == -1)
>>               nodeid = this_node;
>>       if (unlikely(!cachep->nodelists[nodeid])) {

That sounds odd to me. Can you see where the incorrectly predicted
calls are coming from? Calling kmem_cache_alloc_node() with node set
to -1 most of the time could be a real bug somewhere.

Re: [PATCH 3/5] slab.c: remove branch hint

[Ingo Molnar]

* Pekka Enberg <penberg@cs.helsinki.fi> wrote:

> On Tue, Nov 24, 2009 at 1:20 PM, Ingo Molnar <mingo@elte.hu> wrote:
> > (Pekka Cc:-ed)
> >
> > * Tim Blechmann <tim@klingt.org> wrote:
> >
> >> branch profiling on my nehalem machine showed 99% incorrect branch hints:
> >>
> >> ? ?28459 ?7678524 ?99 __cache_alloc_node ? ? ? ? ? ? slab.c
> >> ? 3551
> >>
> >> Signed-off-by: Tim Blechmann <tim@klingt.org>
> >> ---
> >> ?mm/slab.c | ? ?2 +-
> >> ?1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/mm/slab.c b/mm/slab.c
> >> index f70b326..4125fcd 100644
> >> --- a/mm/slab.c
> >> +++ b/mm/slab.c
> >> @@ -3548,7 +3548,7 @@ __cache_alloc_node(struct kmem_cache *cachep,
> >> gfp_t flags, int nodeid,
> >> ? ? ? slab_irq_save(save_flags, this_cpu);
> >> ? ? ? this_node = cpu_to_node(this_cpu);
> >> - ? ? if (unlikely(nodeid == -1))
> >> + ? ? if (nodeid == -1)
> >> ? ? ? ? ? ? ? nodeid = this_node;
> >> ? ? ? if (unlikely(!cachep->nodelists[nodeid])) {
> 
> That sounds odd to me. Can you see where the incorrectly predicted
> calls are coming from? Calling kmem_cache_alloc_node() with node set
> to -1 most of the time could be a real bug somewhere.

I think it could occur in too limited tests - the branch prediction 
looks 'wrong' in certain tests - while it's OK in general.

Is there some easy to run workload you consider more or less 
representative of typical SLAB patterns?

<plug> You might want to pull even with the scheduler subsystem and in 
       addition to 'perf bench sched', add a 'perf bench slab' set of 
       interesting testcases for SLAB performance testing. :-) 
</plug>

	Ingo

Re: [PATCH 3/5] slab.c: remove branch hint

[Tim Blechmann]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/24/2009 12:28 PM, Pekka Enberg wrote:
> On Tue, Nov 24, 2009 at 1:20 PM, Ingo Molnar <mingo@elte.hu> wrote:
>> (Pekka Cc:-ed)
>>
>> * Tim Blechmann <tim@klingt.org> wrote:
>>
>>> branch profiling on my nehalem machine showed 99% incorrect branch hints:
>>>
>>>    28459  7678524  99 __cache_alloc_node             slab.c
>>>   3551
>>>
>>> Signed-off-by: Tim Blechmann <tim@klingt.org>
>>> ---
>>>  mm/slab.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/mm/slab.c b/mm/slab.c
>>> index f70b326..4125fcd 100644
>>> --- a/mm/slab.c
>>> +++ b/mm/slab.c
>>> @@ -3548,7 +3548,7 @@ __cache_alloc_node(struct kmem_cache *cachep,
>>> gfp_t flags, int nodeid,
>>>       slab_irq_save(save_flags, this_cpu);
>>>       this_node = cpu_to_node(this_cpu);
>>> -     if (unlikely(nodeid == -1))
>>> +     if (nodeid == -1)
>>>               nodeid = this_node;
>>>       if (unlikely(!cachep->nodelists[nodeid])) {
> 
> That sounds odd to me. Can you see where the incorrectly predicted
> calls are coming from? Calling kmem_cache_alloc_node() with node set
> to -1 most of the time could be a real bug somewhere.

i don't know, if there is any facility in the ftrace branch profiler to
get call graph information, but i can try to manually dump backtraces in
this condition path ...
could be a specific situation on my machine, though ...

tim

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

You don't have to call it music if the term shocks you.
  John Cage
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)

iEYEARECAAYFAksLx0kACgkQdL+4qsZfVsskOQCglFQG3eYAfdgXoOAHAGTqaLcU
8e0AoIQNbzSRxttGFaXTF3PEh5O4aGEB
=3nmT
-----END PGP SIGNATURE-----

[tip:sched/core] sched: Optimize branch hint in pick_next_task_fair()

[tip-bot for Tim Blechmann]

Commit-ID:  36ace27e3e60d44ea69ce394b2e45386ae98d9d9
Gitweb:     http://git.kernel.org/tip/36ace27e3e60d44ea69ce394b2e45386ae98d9d9
Author:     Tim Blechmann <tim@klingt.org>
AuthorDate: Tue, 24 Nov 2009 11:55:45 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 24 Nov 2009 12:18:12 +0100

sched: Optimize branch hint in pick_next_task_fair()

Branch hint profiling on my nehalem machine showed 90%
incorrect branch hints:

  15728471 158903754  90 pick_next_task_fair
  sched_fair.c    1555

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: <4B0BBBB1.2050100@klingt.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/sched_fair.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
index f28a267..24086e7 100644
--- a/kernel/sched_fair.c
+++ b/kernel/sched_fair.c
@@ -1704,7 +1704,7 @@ static struct task_struct *pick_next_task_fair(struct rq *rq)
 	struct cfs_rq *cfs_rq = &rq->cfs;
 	struct sched_entity *se;
 
-	if (unlikely(!cfs_rq->nr_running))
+	if (!cfs_rq->nr_running)
 		return NULL;
 
 	do {

[tip:sched/core] sched: Optimize branch hint in context_switch()

[tip-bot for Tim Blechmann]

Commit-ID:  710390d90f143a9ebb87a475215140f426792efd
Gitweb:     http://git.kernel.org/tip/710390d90f143a9ebb87a475215140f426792efd
Author:     Tim Blechmann <tim@klingt.org>
AuthorDate: Tue, 24 Nov 2009 11:55:27 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 24 Nov 2009 12:18:42 +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>
---
 kernel/sched.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

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;
 	}

[tip:sched/core] sched, x86: Optimize branch hint in __switch_to()

[tip-bot for Tim Blechmann]

Commit-ID:  a3a1de0c34de6f5f8332cd6151c46af7813c0fcb
Gitweb:     http://git.kernel.org/tip/a3a1de0c34de6f5f8332cd6151c46af7813c0fcb
Author:     Tim Blechmann <tim@klingt.org>
AuthorDate: Tue, 24 Nov 2009 11:55:15 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 24 Nov 2009 12:20:04 +0100

sched, x86: Optimize branch hint in __switch_to()

Branch hint profiling on my nehalem machine showed 96%
incorrect branch hints:

  6548732 174664120  96 __switch_to                    process_64.c
    406
  6548745 174565593  96 __switch_to                    process_64.c
    410

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: <4B0BBB93.3080307@klingt.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/process_64.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index ad535b6..d9db104 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -406,11 +406,10 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
 	 * This won't pick up thread selector changes, but I guess that is ok.
 	 */
 	savesegment(es, prev->es);
-	if (unlikely(next->es | prev->es))
+	if (next->es | prev->es)
 		loadsegment(es, next->es);
-
 	savesegment(ds, prev->ds);
-	if (unlikely(next->ds | prev->ds))
+	if (next->ds | prev->ds)
 		loadsegment(ds, next->ds);
 
 

Re: [tip:sched/core] sched, x86: Optimize branch hint in __switch_to()

[Brian Gerst]

On Tue, Nov 24, 2009 at 11:57 AM, tip-bot for Tim Blechmann
<tim@klingt.org> wrote:
> Commit-ID:  a3a1de0c34de6f5f8332cd6151c46af7813c0fcb
> Gitweb:     http://git.kernel.org/tip/a3a1de0c34de6f5f8332cd6151c46af7813c0fcb
> Author:     Tim Blechmann <tim@klingt.org>
> AuthorDate: Tue, 24 Nov 2009 11:55:15 +0100
> Committer:  Ingo Molnar <mingo@elte.hu>
> CommitDate: Tue, 24 Nov 2009 12:20:04 +0100
>
> sched, x86: Optimize branch hint in __switch_to()
>
> Branch hint profiling on my nehalem machine showed 96%
> incorrect branch hints:
>
>  6548732 174664120  96 __switch_to                    process_64.c
>    406
>  6548745 174565593  96 __switch_to                    process_64.c
>    410
>
> 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: <4B0BBB93.3080307@klingt.org>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  arch/x86/kernel/process_64.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index ad535b6..d9db104 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -406,11 +406,10 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
>         * This won't pick up thread selector changes, but I guess that is ok.
>         */
>        savesegment(es, prev->es);
> -       if (unlikely(next->es | prev->es))
> +       if (next->es | prev->es)
>                loadsegment(es, next->es);
> -
>        savesegment(ds, prev->ds);
> -       if (unlikely(next->ds | prev->ds))
> +       if (next->ds | prev->ds)
>                loadsegment(ds, next->ds);
>
>

64-bit tasks should have %ds and %es set to null selectors.  The only
time they should be different is for 32-bit tasks.  It lookx like some
versions of GCC just aren't using the hint.  I've tested it with 4.4.2
and the generated assembly is the same with or without the hint.

--
Brian Gerst

Re: [tip:sched/core] sched, x86: Optimize branch hint in __switch_to()

[Tim Blechmann]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/24/2009 06:28 PM, Brian Gerst wrote:
> On Tue, Nov 24, 2009 at 11:57 AM, tip-bot for Tim Blechmann
> <tim@klingt.org> wrote:
>> Commit-ID:  a3a1de0c34de6f5f8332cd6151c46af7813c0fcb
>> Gitweb:     http://git.kernel.org/tip/a3a1de0c34de6f5f8332cd6151c46af7813c0fcb
>> Author:     Tim Blechmann <tim@klingt.org>
>> AuthorDate: Tue, 24 Nov 2009 11:55:15 +0100
>> Committer:  Ingo Molnar <mingo@elte.hu>
>> CommitDate: Tue, 24 Nov 2009 12:20:04 +0100
>>
>> sched, x86: Optimize branch hint in __switch_to()
>>
>> Branch hint profiling on my nehalem machine showed 96%
>> incorrect branch hints:
>>
>>  6548732 174664120  96 __switch_to                    process_64.c
>>    406
>>  6548745 174565593  96 __switch_to                    process_64.c
>>    410
>>
>> 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: <4B0BBB93.3080307@klingt.org>
>> Signed-off-by: Ingo Molnar <mingo@elte.hu>
>> ---
>>  arch/x86/kernel/process_64.c |    5 ++---
>>  1 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
>> index ad535b6..d9db104 100644
>> --- a/arch/x86/kernel/process_64.c
>> +++ b/arch/x86/kernel/process_64.c
>> @@ -406,11 +406,10 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
>>         * This won't pick up thread selector changes, but I guess that is ok.
>>         */
>>        savesegment(es, prev->es);
>> -       if (unlikely(next->es | prev->es))
>> +       if (next->es | prev->es)
>>                loadsegment(es, next->es);
>> -
>>        savesegment(ds, prev->ds);
>> -       if (unlikely(next->ds | prev->ds))
>> +       if (next->ds | prev->ds)
>>                loadsegment(ds, next->ds);
>>
>>
> 
> 64-bit tasks should have %ds and %es set to null selectors.  The only
> time they should be different is for 32-bit tasks.
	
this doesn't seem to be the case on my machine for 96% of the calls ...
i am just running 64bit binaries on this machine, no 32bit programs
(that i know of (ubuntu karmic amd64))

tim

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

Relying on the government to protect your privacy is like asking a
peeping tom to install your window blinds.
  John Perry Barlow
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)

iEYEARECAAYFAksM8s0ACgkQdL+4qsZfVsscjwCffcvQz7/VTtIfH1XXgJ2SxJxT
xWYAnjZ9aBD/OWmroVn68oEKqJZ7Pm3U
=C7kB
-----END PGP SIGNATURE-----

Re: [PATCH 3/5] slab.c: remove branch hint

[Pekka Enberg]

Ingo Molnar kirjoitti:
>> That sounds odd to me. Can you see where the incorrectly predicted
>> calls are coming from? Calling kmem_cache_alloc_node() with node set
>> to -1 most of the time could be a real bug somewhere.
> 
> I think it could occur in too limited tests - the branch prediction 
> looks 'wrong' in certain tests - while it's OK in general.
> 
> Is there some easy to run workload you consider more or less 
> representative of typical SLAB patterns?

I can think of three main classes: VFS, SCSI, or network intensive 
applications and benchmarks tend to bring out the worst in SLAB. There 
are probably some interesting NUMA related things that I'm not really 
aware of.

Re: [tip:sched/core] sched, x86: Optimize branch hint in __switch_to()

[Brian Gerst]

On Wed, Nov 25, 2009 at 4:03 AM, Tim Blechmann <tim@klingt.org> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 11/24/2009 06:28 PM, Brian Gerst wrote:
>> On Tue, Nov 24, 2009 at 11:57 AM, tip-bot for Tim Blechmann
>> <tim@klingt.org> wrote:
>>> Commit-ID:  a3a1de0c34de6f5f8332cd6151c46af7813c0fcb
>>> Gitweb:     http://git.kernel.org/tip/a3a1de0c34de6f5f8332cd6151c46af7813c0fcb
>>> Author:     Tim Blechmann <tim@klingt.org>
>>> AuthorDate: Tue, 24 Nov 2009 11:55:15 +0100
>>> Committer:  Ingo Molnar <mingo@elte.hu>
>>> CommitDate: Tue, 24 Nov 2009 12:20:04 +0100
>>>
>>> sched, x86: Optimize branch hint in __switch_to()
>>>
>>> Branch hint profiling on my nehalem machine showed 96%
>>> incorrect branch hints:
>>>
>>>  6548732 174664120  96 __switch_to                    process_64.c
>>>    406
>>>  6548745 174565593  96 __switch_to                    process_64.c
>>>    410
>>>
>>> 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: <4B0BBB93.3080307@klingt.org>
>>> Signed-off-by: Ingo Molnar <mingo@elte.hu>
>>> ---
>>>  arch/x86/kernel/process_64.c |    5 ++---
>>>  1 files changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
>>> index ad535b6..d9db104 100644
>>> --- a/arch/x86/kernel/process_64.c
>>> +++ b/arch/x86/kernel/process_64.c
>>> @@ -406,11 +406,10 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
>>>         * This won't pick up thread selector changes, but I guess that is ok.
>>>         */
>>>        savesegment(es, prev->es);
>>> -       if (unlikely(next->es | prev->es))
>>> +       if (next->es | prev->es)
>>>                loadsegment(es, next->es);
>>> -
>>>        savesegment(ds, prev->ds);
>>> -       if (unlikely(next->ds | prev->ds))
>>> +       if (next->ds | prev->ds)
>>>                loadsegment(ds, next->ds);
>>>
>>>
>>
>> 64-bit tasks should have %ds and %es set to null selectors.  The only
>> time they should be different is for 32-bit tasks.
>
> this doesn't seem to be the case on my machine for 96% of the calls ...
> i am just running 64bit binaries on this machine, no 32bit programs
> (that i know of (ubuntu karmic amd64))
>
> tim

Some early kernel threads inherited KERNEL_DS from the boot process.
Patch coming soon.

--
Brian Gerst

index 780cd92..22db86a 100644

[Brian Gerst]

-	/* set up data segments. actually 0 would do too */
-	movl $__KERNEL_DS,%eax
+	/* set up data segments */
+	xorl %eax,%eax
 	movl %eax,%ds
 	movl %eax,%ss
 	movl %eax,%es

[PATCH] x86, 64-bit: Set data segments to null after switching to 64-bit mode

[Brian Gerst]

This prevents kernel threads from inheriting non-null segment selectors,
and causing optimizations in __switch_to() to be ineffective.

Signed-off-by: Brian Gerst <brgerst@gmail.com>
---
 arch/x86/kernel/head_64.S |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 780cd92..22db86a 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -212,8 +212,8 @@ ENTRY(secondary_startup_64)
 	 */
 	lgdt	early_gdt_descr(%rip)
 
-	/* set up data segments. actually 0 would do too */
-	movl $__KERNEL_DS,%eax
+	/* set up data segments */
+	xorl %eax,%eax
 	movl %eax,%ds
 	movl %eax,%ss
 	movl %eax,%es
-- 
1.6.5.2

[tip:x86/asm] x86, 64-bit: Set data segments to null after switching to 64-bit mode

[tip-bot for Brian Gerst]

Commit-ID:  8ec6993d9f7d961014af970ded57542961fe9ad9
Gitweb:     http://git.kernel.org/tip/8ec6993d9f7d961014af970ded57542961fe9ad9
Author:     Brian Gerst <brgerst@gmail.com>
AuthorDate: Wed, 25 Nov 2009 11:17:36 -0500
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Thu, 26 Nov 2009 10:44:30 +0100

x86, 64-bit: Set data segments to null after switching to 64-bit mode

This prevents kernel threads from inheriting non-null segment
selectors, and causing optimizations in __switch_to() to be
ineffective.

Signed-off-by: Brian Gerst <brgerst@gmail.com>
Cc: Tim Blechmann <tim@klingt.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: Jan Beulich <JBeulich@novell.com>
LKML-Reference: <1259165856-3512-1-git-send-email-brgerst@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/head_64.S |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 2640660..17ba9ec 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -212,8 +212,8 @@ ENTRY(secondary_startup_64)
 	 */
 	lgdt	early_gdt_descr(%rip)
 
-	/* set up data segments. actually 0 would do too */
-	movl $__KERNEL_DS,%eax
+	/* set up data segments */
+	xorl %eax,%eax
 	movl %eax,%ds
 	movl %eax,%ss
 	movl %eax,%es

Re: [PATCH 3/5] slab.c: remove branch hint

[Tim Blechmann]

[-- Attachment #1.1: Type: text/plain, Size: 2038 bytes --]

On 11/24/2009 12:28 PM, Pekka Enberg wrote:
> On Tue, Nov 24, 2009 at 1:20 PM, Ingo Molnar <mingo@elte.hu> wrote:
>> (Pekka Cc:-ed)
>>
>> * Tim Blechmann <tim@klingt.org> wrote:
>>
>>> branch profiling on my nehalem machine showed 99% incorrect branch hints:
>>>
>>>    28459  7678524  99 __cache_alloc_node             slab.c
>>>   3551
>>>
>>> Signed-off-by: Tim Blechmann <tim@klingt.org>
>>> ---
>>>  mm/slab.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/mm/slab.c b/mm/slab.c
>>> index f70b326..4125fcd 100644
>>> --- a/mm/slab.c
>>> +++ b/mm/slab.c
>>> @@ -3548,7 +3548,7 @@ __cache_alloc_node(struct kmem_cache *cachep,
>>> gfp_t flags, int nodeid,
>>>       slab_irq_save(save_flags, this_cpu);
>>>       this_node = cpu_to_node(this_cpu);
>>> -     if (unlikely(nodeid == -1))
>>> +     if (nodeid == -1)
>>>               nodeid = this_node;
>>>       if (unlikely(!cachep->nodelists[nodeid])) {
> 
> That sounds odd to me. Can you see where the incorrectly predicted
> calls are coming from? Calling kmem_cache_alloc_node() with node set
> to -1 most of the time could be a real bug somewhere.

when dumping the stack for the incorrectly hinted branches, i get the
attached stack traces...

hth, tim

--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3548,8 +3548,10 @@ __cache_alloc_node(struct kmem_cache *cachep,
gfp_t flags, int nodeid,
        slab_irq_save(save_flags, this_cpu);

        this_node = cpu_to_node(this_cpu);
-       if (nodeid == -1)
+       if (nodeid == -1) {
+               dump_stack();
                nodeid = this_node;
+       }

        if (unlikely(!cachep->nodelists[nodeid])) {
                /* Node not bootstrapped yet */



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

It is better to make a piece of music than to perform one, better to
perform one than to listen to one, better to listen to one than to
misuse it as a means of distraction, entertainment, or acquisition of
'culture'.
  John Cage

[-- Attachment #1.2: traces.bz2 --]
[-- Type: application/x-bzip, Size: 6636 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] x86, 64-bit: Set data segments to null after switching to 64-bit mode

[Tim Blechmann]

[-- Attachment #1: Type: text/plain, Size: 645 bytes --]

On 11/25/2009 05:17 PM, Brian Gerst wrote:
> This prevents kernel threads from inheriting non-null segment selectors,
> and causing optimizations in __switch_to() to be ineffective.
> 
> Signed-off-by: Brian Gerst <brgerst@gmail.com>

with this patch, the branch hint is actually correct, giving 100%
correct predictions. so please revert my earlier patch (commit
a3a1de0c34de6f5f8332cd6151c46af7813c0fcb)

thnx, tim

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

Desperation is the raw material of drastic change. Only those who can
leave behind everything they have ever believed in can hope to escape.
  William S. Burroughs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 3/5] slab.c: remove branch hint

[Pekka Enberg]

Tim Blechmann kirjoitti:
> On 11/24/2009 12:28 PM, Pekka Enberg wrote:
>> On Tue, Nov 24, 2009 at 1:20 PM, Ingo Molnar <mingo@elte.hu> wrote:
>>> (Pekka Cc:-ed)
>>>
>>> * Tim Blechmann <tim@klingt.org> wrote:
>>>
>>>> branch profiling on my nehalem machine showed 99% incorrect branch hints:
>>>>
>>>>    28459  7678524  99 __cache_alloc_node             slab.c
>>>>   3551
>>>>
>>>> Signed-off-by: Tim Blechmann <tim@klingt.org>
>>>> ---
>>>>  mm/slab.c |    2 +-
>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/mm/slab.c b/mm/slab.c
>>>> index f70b326..4125fcd 100644
>>>> --- a/mm/slab.c
>>>> +++ b/mm/slab.c
>>>> @@ -3548,7 +3548,7 @@ __cache_alloc_node(struct kmem_cache *cachep,
>>>> gfp_t flags, int nodeid,
>>>>       slab_irq_save(save_flags, this_cpu);
>>>>       this_node = cpu_to_node(this_cpu);
>>>> -     if (unlikely(nodeid == -1))
>>>> +     if (nodeid == -1)
>>>>               nodeid = this_node;
>>>>       if (unlikely(!cachep->nodelists[nodeid])) {
>> That sounds odd to me. Can you see where the incorrectly predicted
>> calls are coming from? Calling kmem_cache_alloc_node() with node set
>> to -1 most of the time could be a real bug somewhere.
> 
> when dumping the stack for the incorrectly hinted branches, i get the
> attached stack traces...
> 
> hth, tim
> 
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -3548,8 +3548,10 @@ __cache_alloc_node(struct kmem_cache *cachep,
> gfp_t flags, int nodeid,
>         slab_irq_save(save_flags, this_cpu);
> 
>         this_node = cpu_to_node(this_cpu);
> -       if (nodeid == -1)
> +       if (nodeid == -1) {
> +               dump_stack();
>                 nodeid = this_node;
> +       }
> 
>         if (unlikely(!cachep->nodelists[nodeid])) {
>                 /* Node not bootstrapped yet */
> 
> 
> 

OK, so it's the generic alloc_skb() function that keeps hitting 
kmem_cache_alloc_node() with "-1". Christoph, are you okay with removing 
the unlikely() annotation from __cache_alloc_node()?

			Pekka

Re: [PATCH 3/5] slab.c: remove branch hint

[Christoph Lameter]

On Mon, 30 Nov 2009, Pekka Enberg wrote:

> OK, so it's the generic alloc_skb() function that keeps hitting
> kmem_cache_alloc_node() with "-1". Christoph, are you okay with removing the
> unlikely() annotation from __cache_alloc_node()?

Yes. Lets look for other cases in the allocators too.
kmem_cache_alloc_node used to be mainly used for off node allocations but
the network alloc_skb() case shows that this is changing now.

I hope the users of kmem_cache_alloc_node() realize that using -1 is not
equivalent to kmem_cache_alloc(). kmem_cache_alloc follows numa policies
for memory allocations. kmem_cache_alloc_node() does not.

Re: [PATCH 3/5] slab.c: remove branch hint

[Pekka Enberg]

On Mon, Nov 30, 2009 at 6:09 PM, Christoph Lameter
<cl@linux-foundation.org> wrote:
> On Mon, 30 Nov 2009, Pekka Enberg wrote:
>
>> OK, so it's the generic alloc_skb() function that keeps hitting
>> kmem_cache_alloc_node() with "-1". Christoph, are you okay with removing the
>> unlikely() annotation from __cache_alloc_node()?
>
> Yes. Lets look for other cases in the allocators too.
> kmem_cache_alloc_node used to be mainly used for off node allocations but
> the network alloc_skb() case shows that this is changing now.

Tim, can you please re-send the patch to me so I can apply it?

Re: [PATCH 3/5] slab.c: remove branch hint

[Christoph Lameter]

On Mon, 30 Nov 2009, Pekka Enberg wrote:

> Tim, can you please re-send the patch to me so I can apply it?

SLUB has no issue since NUMA decisions are deferred to the page allocator.

[PATCH] branch profiling on my nehalem machine showed 99% incorrect branch hints:

[Tim Blechmann]

[-- Attachment #1.1: Type: text/plain, Size: 378 bytes --]

i have regenerated the patch for tip/tip ...

--
   28459  7678524  99 __cache_alloc_node             slab.c               3551

Discussion on lkml [1] led to the solution to remove this hint.

[1] http://patchwork.kernel.org/patch/63517/

Signed-off-by: Tim Blechmann <tim@klingt.org>
---
 mm/slab.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-branch-profiling-on-my-nehalem-machine-showed-99-inc.patch --]
[-- Type: text/x-patch; name="0001-branch-profiling-on-my-nehalem-machine-showed-99-inc.patch", Size: 411 bytes --]

diff --git a/mm/slab.c b/mm/slab.c
index 7dfa481..0d4b731 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3306,7 +3306,7 @@ __cache_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
 	cache_alloc_debugcheck_before(cachep, flags);
 	local_irq_save(save_flags);
 
-	if (unlikely(nodeid == -1))
+	if (nodeid == -1)
 		nodeid = numa_node_id();
 
 	if (unlikely(!cachep->nodelists[nodeid])) {


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH] Revert "sched, x86: Optimize branch hint in __switch_to()"

[Tim Blechmann]

[-- Attachment #1: Type: text/plain, Size: 393 bytes --]


This reverts commit a3a1de0c34de6f5f8332cd6151c46af7813c0fcb.

Commit 8ec6993d9f7d961014af970ded57542961fe9ad9 cleared the es and ds
selectors, so the original branch hints are correct now. Therefore
the branch hint doesn't need to be removed.

Signed-off-by: Tim Blechmann <tim@klingt.org>
---
  arch/x86/kernel/process_64.c |    5 +++--
  1 files changed, 3 insertions(+), 2 deletions(-)



[-- Attachment #2: 0001-Revert-sched-x86-Optimize-branch-hint-in-__switch_to.patch --]
[-- Type: text/x-patch, Size: 595 bytes --]

diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 93c501d..eb62cbc 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -406,10 +406,11 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
 	 * This won't pick up thread selector changes, but I guess that is ok.
 	 */
 	savesegment(es, prev->es);
-	if (next->es | prev->es)
+	if (unlikely(next->es | prev->es))
 		loadsegment(es, next->es);
+
 	savesegment(ds, prev->ds);
-	if (next->ds | prev->ds)
+	if (unlikely(next->ds | prev->ds))
 		loadsegment(ds, next->ds);
 
 

[tip:sched/core] Revert "sched, x86: Optimize branch hint in __switch_to()"

[tip-bot for Tim Blechmann]

Commit-ID:  be8147e68625a1adb111acfd6b98a492be4b74d0
Gitweb:     http://git.kernel.org/tip/be8147e68625a1adb111acfd6b98a492be4b74d0
Author:     Tim Blechmann <tim@klingt.org>
AuthorDate: Wed, 2 Dec 2009 12:32:10 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 2 Dec 2009 12:38:03 +0100

Revert "sched, x86: Optimize branch hint in __switch_to()"

This reverts commit a3a1de0c34de6f5f8332cd6151c46af7813c0fcb.

Commit 8ec6993d9f7d961014af970ded57542961fe9ad9 cleared the es
and ds selectors, so the original branch hints are correct now.
Therefore the branch hint doesn't need to be removed.

Signed-off-by: Tim Blechmann <tim@klingt.org>
LKML-Reference: <4B16503A.8030508@klingt.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/process_64.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 93c501d..eb62cbc 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -406,10 +406,11 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
 	 * This won't pick up thread selector changes, but I guess that is ok.
 	 */
 	savesegment(es, prev->es);
-	if (next->es | prev->es)
+	if (unlikely(next->es | prev->es))
 		loadsegment(es, next->es);
+
 	savesegment(ds, prev->ds);
-	if (next->ds | prev->ds)
+	if (unlikely(next->ds | prev->ds))
 		loadsegment(ds, next->ds);
 
 

Re: [PATCH] branch profiling on my nehalem machine showed 99% incorrect branch hints:

[Pekka Enberg]

Tim Blechmann wrote:
> i have regenerated the patch for tip/tip ...
> 
> --
>    28459  7678524  99 __cache_alloc_node             slab.c               3551
> 
> Discussion on lkml [1] led to the solution to remove this hint.
> 
> [1] http://patchwork.kernel.org/patch/63517/
> 
> Signed-off-by: Tim Blechmann <tim@klingt.org>
> ---
>  mm/slab.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

Applied, thanks.

Please use plain text email for sending patches in the future.