On Tue, 2006-05-30 at 13:38 -0400, Steven Rostedt wrote:
> On Mon, 2006-05-29 at 18:33 -0700, Andrew Morton wrote:
> > On Mon, 29 May 2006 23:23:28 +0200
> > Ingo Molnar <[email protected]> wrote:
> >
> > > add WARN_ON_ONCE(cond) to print once-per-bootup messages.
> > >
> > > Signed-off-by: Ingo Molnar <[email protected]>
> > > Signed-off-by: Arjan van de Ven <[email protected]>
> > > ---
> > > include/asm-generic/bug.h | 13 +++++++++++++
> > > 1 file changed, 13 insertions(+)
> > >
> > > Index: linux/include/asm-generic/bug.h
> > > ===================================================================
> > > --- linux.orig/include/asm-generic/bug.h
> > > +++ linux/include/asm-generic/bug.h
> > > @@ -44,4 +44,17 @@
> > > # define WARN_ON_SMP(x) do { } while (0)
> > > #endif
> > >
> > > +#define WARN_ON_ONCE(condition) \
> > > +({ \
> > > + static int __warn_once = 1; \
> > > + int __ret = 0; \
> > > + \
> > > + if (unlikely(__warn_once && (condition))) { \
>
> Since __warn_once is likely to be true, and the condition is likely to
> be false, wouldn't it be better to switch this around to:
>
> if (unlikely((condition) && __warn_once)) {
>
> So the && will fall out before having to check a global variable.
>
> Only after the unlikely condition would the __warn_once be false.
Hi Ingo,
Not sure if you missed this request or didn't think it mattered. But I
just tried out the difference between the two to see what gcc would do
to a simple function compiling with -O2.
Here's my code:
----- with the current WARN_ON_ONCE ----
#define unlikely(x) __builtin_expect(!!(x), 0)
#define WARN_ON_ONCE(condition) \
({ \
static int __warn_once = 1; \
int __ret = 0; \
\
if (__warn_once && unlikely((condition))) { \
__warn_once = 0; \
WARN_ON(1); \
__ret = 1; \
} \
__ret; \
})
int warn (int x)
{
WARN_ON_ONCE(x==1);
return x+1;
}
----- with the version I suggest. ----
#define unlikely(x) __builtin_expect(!!(x), 0)
#define WARN_ON_ONCE(condition) \
({ \
static int __warn_once = 1; \
int __ret = 0; \
\
if (unlikely((condition)) && __warn_once) { \
__warn_once = 0; \
WARN_ON(1); \
__ret = 1; \
} \
__ret; \
})
int warn(int x)
{
WARN_ON_ONCE(x==1);
return x+1;
}
-------
Compiling these two I get this:
current warn.o:
00000000 <warn>:
0: 55 push %ebp
1: 89 e5 mov %esp,%ebp
3: 53 push %ebx
4: 83 ec 04 sub $0x4,%esp
7: a1 00 00 00 00 mov 0x0,%eax
c: 8b 5d 08 mov 0x8(%ebp),%ebx
# here we test the __warn_once first and if it is not zero
# it jumps to warn+0x20 to do the condition test
f: 85 c0 test %eax,%eax
11: 75 0d jne 20 <warn+0x20>
13: 5a pop %edx
14: 8d 43 01 lea 0x1(%ebx),%eax
17: 5b pop %ebx
18: 5d pop %ebp
19: c3 ret
1a: 8d b6 00 00 00 00 lea 0x0(%esi),%esi
20: 83 fb 01 cmp $0x1,%ebx
23: 75 ee jne 13 <warn+0x13>
25: 31 c9 xor %ecx,%ecx
27: 89 0d 00 00 00 00 mov %ecx,0x0
2d: c7 04 24 01 00 00 00 movl $0x1,(%esp)
34: e8 fc ff ff ff call 35 <warn+0x35>
39: eb d8 jmp 13 <warn+0x13>
Disassembly of section .data:
My suggested change of doing the condition first:
00000000 <warn>:
0: 55 push %ebp
1: 89 e5 mov %esp,%ebp
3: 53 push %ebx
4: 83 ec 04 sub $0x4,%esp
7: 8b 5d 08 mov 0x8(%ebp),%ebx
# here we test the condition first, and if it the
# unlikely condition is true, then we jump to test
# the __warn_once.
a: 83 fb 01 cmp $0x1,%ebx
d: 74 07 je 16 <warn+0x16>
f: 5a pop %edx
10: 8d 43 01 lea 0x1(%ebx),%eax
13: 5b pop %ebx
14: 5d pop %ebp
15: c3 ret
16: a1 00 00 00 00 mov 0x0,%eax
1b: 85 c0 test %eax,%eax
1d: 74 f0 je f <warn+0xf>
1f: 31 c9 xor %ecx,%ecx
21: 89 0d 00 00 00 00 mov %ecx,0x0
27: c7 04 24 01 00 00 00 movl $0x1,(%esp)
2e: e8 fc ff ff ff call 2f <warn+0x2f>
33: eb da jmp f <warn+0xf>
Disassembly of section .data:
As you can see, because the whole thing is unlikely, the first condition
is expected to fail. With the current WARN_ON logic, that means that
the __warn_once is expected to fail, but that's not the case. So on a
normal system where the WARN_ON_ONCE condition would never happen, you
are always branching. So simply reversing the order to test the
condition before testing the __warn_once variable should improve cache
performance.
Below is my recommended patch.
-- Steve
Index: linux-2.6.17-rc5-mm2/include/asm-generic/bug.h
===================================================================
--- linux-2.6.17-rc5-mm2.orig/include/asm-generic/bug.h 2006-06-03 14:01:22.000000000 -0400
+++ linux-2.6.17-rc5-mm2/include/asm-generic/bug.h 2006-06-03 14:01:50.000000000 -0400
@@ -43,7 +43,7 @@
static int __warn_once = 1; \
int __ret = 0; \
\
- if (unlikely(__warn_once && (condition))) { \
+ if (unlikely((condition) && __warn_once)) { \
__warn_once = 0; \
WARN_ON(1); \
__ret = 1; \
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
