* [patch] i386: switch_to(): misplaced parentheses
@ 2006-07-25 20:15 Chuck Ebbert
2006-07-29 1:39 ` Steven Rostedt
0 siblings, 1 reply; 3+ messages in thread
From: Chuck Ebbert @ 2006-07-25 20:15 UTC (permalink / raw)
To: linux-kernel; +Cc: Andrew Morton, Linus Torvalds
Recent changes in i386 __switch_to() have a misplaced closing
parenthesis causing an unlikely() to terminate early.
Signed-off-by: Chuck Ebbert <76306.1226@compuserve.com>
--- 2.6.18-rc2-32.orig/arch/i386/kernel/process.c
+++ 2.6.18-rc2-32/arch/i386/kernel/process.c
@@ -690,8 +690,8 @@ struct task_struct fastcall * __switch_t
/*
* Now maybe handle debug registers and/or IO bitmaps
*/
- if (unlikely((task_thread_info(next_p)->flags & _TIF_WORK_CTXSW))
- || test_tsk_thread_flag(prev_p, TIF_IO_BITMAP))
+ if (unlikely((task_thread_info(next_p)->flags & _TIF_WORK_CTXSW)
+ || test_tsk_thread_flag(prev_p, TIF_IO_BITMAP)))
__switch_to_xtra(next_p, tss);
disable_tsc(prev_p, next_p);
--
Chuck
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [patch] i386: switch_to(): misplaced parentheses
2006-07-25 20:15 [patch] i386: switch_to(): misplaced parentheses Chuck Ebbert
@ 2006-07-29 1:39 ` Steven Rostedt
2006-07-29 2:08 ` Steven Rostedt
0 siblings, 1 reply; 3+ messages in thread
From: Steven Rostedt @ 2006-07-29 1:39 UTC (permalink / raw)
To: Chuck Ebbert; +Cc: linux-kernel, Andrew Morton, Linus Torvalds
On Tue, 2006-07-25 at 16:15 -0400, Chuck Ebbert wrote:
> Recent changes in i386 __switch_to() have a misplaced closing
> parenthesis causing an unlikely() to terminate early.
>
> Signed-off-by: Chuck Ebbert <76306.1226@compuserve.com>
>
> --- 2.6.18-rc2-32.orig/arch/i386/kernel/process.c
> +++ 2.6.18-rc2-32/arch/i386/kernel/process.c
> @@ -690,8 +690,8 @@ struct task_struct fastcall * __switch_t
> /*
> * Now maybe handle debug registers and/or IO bitmaps
> */
> - if (unlikely((task_thread_info(next_p)->flags & _TIF_WORK_CTXSW))
> - || test_tsk_thread_flag(prev_p, TIF_IO_BITMAP))
> + if (unlikely((task_thread_info(next_p)->flags & _TIF_WORK_CTXSW)
> + || test_tsk_thread_flag(prev_p, TIF_IO_BITMAP)))
> __switch_to_xtra(next_p, tss);
>
> disable_tsc(prev_p, next_p);
Unlikely's with or's are kind of ambiguous. An 'and' makes sense but
or's don't. Because a branch is going to happen anyway. Just to test
this out, I made a little function and tried out different types of
parenthesis placements.
Here: (the original placement)
#define unlikely(x) __builtin_expect(!!(x), 0)
int switch_to(int x, int y)
{
int a = 0;
if (unlikely(x==24) || (y==83))
a=10;
return a;
}
and now yours:
#define unlikely(x) __builtin_expect(!!(x), 0)
int switch_to(int x, int y)
{
int a = 0;
if (unlikely(x==24 || y==83))
a=10;
return a;
}
The original gave me this: gcc -O2 -c switch.c
objdump -Dr switch.o
00000000 <switch_to>:
0: 55 push %ebp
1: 89 e5 mov %esp,%ebp
3: 83 7d 08 18 cmpl $0x18,0x8(%ebp)
7: 74 0a je 13 <switch_to+0x13>
9: 31 c0 xor %eax,%eax
b: 83 7d 0c 53 cmpl $0x53,0xc(%ebp)
f: 74 02 je 13 <switch_to+0x13>
11: 5d pop %ebp
12: c3 ret
13: 5d pop %ebp
14: b8 0a 00 00 00 mov $0xa,%eax
19: c3 ret
and then yours: gcc -O2 -c switch_alt.c
objdump -Dr switch_alt.o
00000000 <switch_to>:
0: 55 push %ebp
1: 89 e5 mov %esp,%ebp
3: 83 7d 08 18 cmpl $0x18,0x8(%ebp)
7: 74 0a je 13 <switch_to+0x13>
9: 31 c0 xor %eax,%eax
b: 83 7d 0c 53 cmpl $0x53,0xc(%ebp)
f: 74 02 je 13 <switch_to+0x13>
11: 5d pop %ebp
12: c3 ret
13: 5d pop %ebp
14: b8 0a 00 00 00 mov $0xa,%eax
19: c3 ret
They are identical!!!
This is not surprising since you still need to test the other OR case if
it fails, which you are saying it will. So saying it is unlikely is
meaningless. Likely's are good for 'or' and unlikely's are good for
'and'. But not the other way around.
|| and && short circuit when they can.
So the statement of A || B is:
TEST A
JUMP_IF_TRUE true
TEST B
JUMP_IF_FALSE false
true:
do true statement
JUMP out:
false:
do else statement
out:
Saying TEST A is likely to fail really doesn't help the situation.
Saying it will likely pass will. Then we could do this:
TEST A
JUMP_IF_FALSE test2
true:
do true statement
JUMP out:
false:
do false statement
jump out:
test2:
TEST B
JUMP_IF_TRUE true
JUMP false:
out:
Where here we can move the true after the first compare and prevent the
conditional branch.
So what I'm saying is that unlikely(a || b) has really no good meaning.
Out of curiosity, I removed the unlikely altogether and here's what I
got.
gcc -O2 -c switch_none.c
00000000 <switch_to>:
0: 55 push %ebp
1: 89 e5 mov %esp,%ebp
3: 83 7d 08 18 cmpl $0x18,0x8(%ebp)
7: 74 0a je 13 <switch_to+0x13>
9: 31 c0 xor %eax,%eax
b: 83 7d 0c 53 cmpl $0x53,0xc(%ebp)
f: 74 02 je 13 <switch_to+0x13>
11: 5d pop %ebp
12: c3 ret
13: 5d pop %ebp
14: b8 0a 00 00 00 mov $0xa,%eax
19: c3 ret
Ha! still the same.
Is that unlikely there really do any good?
-- Steve
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [patch] i386: switch_to(): misplaced parentheses
2006-07-29 1:39 ` Steven Rostedt
@ 2006-07-29 2:08 ` Steven Rostedt
0 siblings, 0 replies; 3+ messages in thread
From: Steven Rostedt @ 2006-07-29 2:08 UTC (permalink / raw)
To: Chuck Ebbert; +Cc: linux-kernel, Andrew Morton, Linus Torvalds
On Fri, 2006-07-28 at 21:39 -0400, Steven Rostedt wrote:
>
> Unlikely's with or's are kind of ambiguous. An 'and' makes sense but
> or's don't. Because a branch is going to happen anyway. Just to test
> this out, I made a little function and tried out different types of
> parenthesis placements.
OK, I take this back. Thinking that it might make a difference to the
second compare, I added a little more to my test program:
---
#define unlikely(x) __builtin_expect(!!(x), 0)
int do_func(void);
int a;
int switch_to(int x, int y)
{
a = 2;
if (unlikely(x==24 || y==83))
a=10;
do_func();
return a;
}
---
This way I now have a global 'a' and a function that just might use that
'a'. So this does make a difference: Kind of funky though. It wastes
space to make it avoid branching when we don't have your total unlikely
(see below):
The above gave:
00000000 <switch_to>:
0: 55 push %ebp
1: ba 02 00 00 00 mov $0x2,%edx
6: 89 e5 mov %esp,%ebp
8: 83 ec 08 sub $0x8,%esp
b: 83 7d 08 18 cmpl $0x18,0x8(%ebp)
f: 89 15 00 00 00 00 mov %edx,0x0
11: R_386_32 a
15: 74 12 je 29 <switch_to+0x29>
17: 83 7d 0c 53 cmpl $0x53,0xc(%ebp)
1b: 74 0c je 29 <switch_to+0x29>
1d: e8 fc ff ff ff call 1e <switch_to+0x1e>
1e: R_386_PC32 do_func
22: a1 00 00 00 00 mov 0x0,%eax
23: R_386_32 a
27: c9 leave
28: c3 ret
29: b8 0a 00 00 00 mov $0xa,%eax
2e: a3 00 00 00 00 mov %eax,0x0
2f: R_386_32 a
33: eb e8 jmp 1d <switch_to+0x1d>
Which looks the best, so your patch may be good after all :-)
The other tests looked pretty much the same:
Between (unlikely(x==24) || (y==83)) and
((x==24) || unlikely(y==83)) and
(x==24 || y==83)
which was this: (done with the unlikely(y==83))
00000000 <switch_to>:
0: 55 push %ebp
1: ba 02 00 00 00 mov $0x2,%edx
6: 89 e5 mov %esp,%ebp
8: 83 ec 08 sub $0x8,%esp
b: 83 7d 08 18 cmpl $0x18,0x8(%ebp)
f: 89 15 00 00 00 00 mov %edx,0x0
11: R_386_32 a
15: 74 19 je 30 <switch_to+0x30>
17: 83 7d 0c 53 cmpl $0x53,0xc(%ebp)
1b: 74 13 je 30 <switch_to+0x30>
1d: e8 fc ff ff ff call 1e <switch_to+0x1e>
1e: R_386_PC32 do_func
22: a1 00 00 00 00 mov 0x0,%eax
23: R_386_32 a
27: c9 leave
28: c3 ret
29: 8d b4 26 00 00 00 00 lea 0x0(%esi),%esi
30: b8 0a 00 00 00 mov $0xa,%eax
35: a3 00 00 00 00 mov %eax,0x0
36: R_386_32 a
3a: e8 fc ff ff ff call 3b <switch_to+0x3b>
3b: R_386_PC32 do_func
3f: a1 00 00 00 00 mov 0x0,%eax
40: R_386_32 a
44: c9 leave
45: c3 ret
So the big savings isn't the branching, but the entire branch is
expected to fail, so it doesn't bother with the duplicate code to speed
things up (see the two do_func calls). So your patch really just saves
space, and not really speed (but you can argue that this space savings
increases speed by not wasting cache).
So I do give credence to your patch.
Acked-by: Steven Rostedt <rostedt@goodmis.org>
-- Steve
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2006-07-29 2:08 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-07-25 20:15 [patch] i386: switch_to(): misplaced parentheses Chuck Ebbert
2006-07-29 1:39 ` Steven Rostedt
2006-07-29 2:08 ` Steven Rostedt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.