* [PATCH] WARN_ONCE(): use bool for boolean flag
@ 2009-09-27 13:53 Cesar Eduardo Barros
2009-09-27 14:03 ` Daniel Walker
0 siblings, 1 reply; 18+ messages in thread
From: Cesar Eduardo Barros @ 2009-09-27 13:53 UTC (permalink / raw)
To: linux-kernel; +Cc: Andrew Morton, Roland Dreier, Cesar Eduardo Barros
Commit 70867453092297be9afb2249e712a1f960ec0a09 changed printk_once()
to use bool instead of int for its guard variable. Do the same change
to WARN_ONCE() and WARN_ON_ONCE(), for the same reasons.
Signed-off-by: Cesar Eduardo Barros <cesarb@cesarb.net>
---
include/asm-generic/bug.h | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 4b67559..18c435d 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -113,22 +113,22 @@ extern void warn_slowpath_null(const char *file, const int line);
#endif
#define WARN_ON_ONCE(condition) ({ \
- static int __warned; \
+ static bool __warned; \
int __ret_warn_once = !!(condition); \
\
if (unlikely(__ret_warn_once)) \
if (WARN_ON(!__warned)) \
- __warned = 1; \
+ __warned = true; \
unlikely(__ret_warn_once); \
})
#define WARN_ONCE(condition, format...) ({ \
- static int __warned; \
+ static bool __warned; \
int __ret_warn_once = !!(condition); \
\
if (unlikely(__ret_warn_once)) \
if (WARN(!__warned, format)) \
- __warned = 1; \
+ __warned = true; \
unlikely(__ret_warn_once); \
})
--
1.6.4.4
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] WARN_ONCE(): use bool for boolean flag
2009-09-27 13:53 [PATCH] WARN_ONCE(): use bool for boolean flag Cesar Eduardo Barros
@ 2009-09-27 14:03 ` Daniel Walker
2009-09-27 15:56 ` Cesar Eduardo Barros
0 siblings, 1 reply; 18+ messages in thread
From: Daniel Walker @ 2009-09-27 14:03 UTC (permalink / raw)
To: Cesar Eduardo Barros; +Cc: linux-kernel, Andrew Morton, Roland Dreier
On Sun, 2009-09-27 at 10:53 -0300, Cesar Eduardo Barros wrote:
> #define
> WARN_ON_ONCE(condition) ({ \
> - static int __warned; \
> + static bool __warned; \
> int __ret_warn_once = !!(condition); \
Could __ret_warn_once be bool also ? It looks like just another
conditional variable..
Daniel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] WARN_ONCE(): use bool for boolean flag
2009-09-27 14:03 ` Daniel Walker
@ 2009-09-27 15:56 ` Cesar Eduardo Barros
2009-09-27 16:52 ` Daniel Walker
0 siblings, 1 reply; 18+ messages in thread
From: Cesar Eduardo Barros @ 2009-09-27 15:56 UTC (permalink / raw)
To: Daniel Walker; +Cc: linux-kernel, Andrew Morton, Roland Dreier
Daniel Walker escreveu:
> On Sun, 2009-09-27 at 10:53 -0300, Cesar Eduardo Barros wrote:
>> #define
>> WARN_ON_ONCE(condition) ({ \
>> - static int __warned; \
>> + static bool __warned; \
>> int __ret_warn_once = !!(condition); \
>
> Could __ret_warn_once be bool also ? It looks like just another
> conditional variable..
Yes, it could (as long as either it is converted back to int in the
return of the macro, or all users do not care about the macro's return
type). However, the justification used for the printk_once patch (and
this WARN_ONCE patch) does not apply directly anymore, since the code is
different (to start with, it is not a static variable).
--
Cesar Eduardo Barros
cesarb@cesarb.net
cesar.barros@gmail.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] WARN_ONCE(): use bool for boolean flag
2009-09-27 15:56 ` Cesar Eduardo Barros
@ 2009-09-27 16:52 ` Daniel Walker
2009-09-27 17:24 ` Cesar Eduardo Barros
0 siblings, 1 reply; 18+ messages in thread
From: Daniel Walker @ 2009-09-27 16:52 UTC (permalink / raw)
To: Cesar Eduardo Barros; +Cc: linux-kernel, Andrew Morton, Roland Dreier
On Sun, 2009-09-27 at 12:56 -0300, Cesar Eduardo Barros wrote:
> Daniel Walker escreveu:
> > On Sun, 2009-09-27 at 10:53 -0300, Cesar Eduardo Barros wrote:
> >> #define
> >> WARN_ON_ONCE(condition) ({ \
> >> - static int __warned; \
> >> + static bool __warned; \
> >> int __ret_warn_once = !!(condition); \
> >
> > Could __ret_warn_once be bool also ? It looks like just another
> > conditional variable..
>
> Yes, it could (as long as either it is converted back to int in the
> return of the macro, or all users do not care about the macro's return
> type). However, the justification used for the printk_once patch (and
> this WARN_ONCE patch) does not apply directly anymore, since the code is
> different (to start with, it is not a static variable).
I did a couple kernel builds to test this on a small normal config,
vmlinux.base-line
text data bss dec hex filename
6718958 497200 1082460 8298618 7ea07a vmlinux.base-line
vmlinux.one-bool <-- Your patch
text data bss dec hex filename
6718590 497232 1082292 8298114 7e9e82 vmlinux.one-bool
vmlinux.all-bool-converted
text data bss dec hex filename
6718506 497232 1082292 8298030 7e9e2e vmlinux.all-converted
your changes drops the size 368 bytes, and if you convert the other
conditionals it drops it by another 84 bytes. Not much more, but it's
something.
So I think Rolands original reasoning still holds.. As far as people
needing an int output from WARN_ON() , I'm not sure that's happening
anyplace .. I can't imagine a sane usage for that..
Daniel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] WARN_ONCE(): use bool for boolean flag
2009-09-27 16:52 ` Daniel Walker
@ 2009-09-27 17:24 ` Cesar Eduardo Barros
2009-09-27 17:32 ` Daniel Walker
0 siblings, 1 reply; 18+ messages in thread
From: Cesar Eduardo Barros @ 2009-09-27 17:24 UTC (permalink / raw)
To: Daniel Walker; +Cc: linux-kernel, Andrew Morton, Roland Dreier
Daniel Walker escreveu:
> On Sun, 2009-09-27 at 12:56 -0300, Cesar Eduardo Barros wrote:
>> Daniel Walker escreveu:
>>> On Sun, 2009-09-27 at 10:53 -0300, Cesar Eduardo Barros wrote:
>>>> #define
>>>> WARN_ON_ONCE(condition) ({ \
>>>> - static int __warned; \
>>>> + static bool __warned; \
>>>> int __ret_warn_once = !!(condition); \
>>> Could __ret_warn_once be bool also ? It looks like just another
>>> conditional variable..
>> Yes, it could (as long as either it is converted back to int in the
>> return of the macro, or all users do not care about the macro's return
>> type). However, the justification used for the printk_once patch (and
>> this WARN_ONCE patch) does not apply directly anymore, since the code is
>> different (to start with, it is not a static variable).
>
> I did a couple kernel builds to test this on a small normal config,
>
> vmlinux.base-line
> text data bss dec hex filename
> 6718958 497200 1082460 8298618 7ea07a vmlinux.base-line
>
> vmlinux.one-bool <-- Your patch
> text data bss dec hex filename
> 6718590 497232 1082292 8298114 7e9e82 vmlinux.one-bool
I am still trying to understand why data increases (but not enough to
offset the gains on text and bss). My own testing had the same
qualitative result (x86-64 defconfig):
text data bss dec hex filename
8101271 1207116 992764 10301151 9d2edf vmlinux.warn.before
8100553 1207148 991988 10299689 9d2929 vmlinux.warn.after
> vmlinux.all-bool-converted
> text data bss dec hex filename
> 6718506 497232 1082292 8298030 7e9e2e vmlinux.all-converted
>
> your changes drops the size 368 bytes, and if you convert the other
> conditionals it drops it by another 84 bytes. Not much more, but it's
> something.
>
> So I think Rolands original reasoning still holds.. As far as people
> needing an int output from WARN_ON() , I'm not sure that's happening
> anyplace .. I can't imagine a sane usage for that..
I took a quick look, and all uses seem to be directly in a boolean
context (within an if()), so there would be no problem. Besides, the
unlikely() all these macros end with does a double negation, meaning
even if it is an int, it will be either 0 or 1 (but I am not sure I am
reading these macros right - it seems CONFIG_TRACE_BRANCH_PROFILING
turns all unlikely() into likely()).
In fact, I was expecting no change at all, since gcc should be able to
see it is being treated as a boolean (perhaps I am trusting gcc too
much). And to make matters even more confusing, my own test changing all
__ret_warn_once to bool and dropping the !! caused an _increase_ of 598
bytes (x86-64 defconfig).
text data bss dec hex filename
8100553 1207148 991988 10299689 9d2929 vmlinux.warnret.before
8101119 1207180 991988 10300287 9d2b7f vmlinux.warnret.after
(And yes, data increased again.)
--
Cesar Eduardo Barros
cesarb@cesarb.net
cesar.barros@gmail.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] WARN_ONCE(): use bool for boolean flag
2009-09-27 17:24 ` Cesar Eduardo Barros
@ 2009-09-27 17:32 ` Daniel Walker
2009-09-27 17:48 ` Cesar Eduardo Barros
0 siblings, 1 reply; 18+ messages in thread
From: Daniel Walker @ 2009-09-27 17:32 UTC (permalink / raw)
To: Cesar Eduardo Barros; +Cc: linux-kernel, Andrew Morton, Roland Dreier
On Sun, 2009-09-27 at 14:24 -0300, Cesar Eduardo Barros wrote:
> I took a quick look, and all uses seem to be directly in a boolean
> context (within an if()), so there would be no problem. Besides, the
> unlikely() all these macros end with does a double negation, meaning
> even if it is an int, it will be either 0 or 1 (but I am not sure I am
> reading these macros right - it seems CONFIG_TRACE_BRANCH_PROFILING
> turns all unlikely() into likely()).
>
> In fact, I was expecting no change at all, since gcc should be able to
> see it is being treated as a boolean (perhaps I am trusting gcc too
> much). And to make matters even more confusing, my own test changing all
> __ret_warn_once to bool and dropping the !! caused an _increase_ of 598
> bytes (x86-64 defconfig).
>
> text data bss dec hex filename
> 8100553 1207148 991988 10299689 9d2929 vmlinux.warnret.before
> 8101119 1207180 991988 10300287 9d2b7f vmlinux.warnret.after
>
> (And yes, data increased again.)
Did you have the CONFIG_TRACE_BRANCH_PROFILING option enabled for the
test above?
If this was just your regular base line config , then that is odd .. I
also would think worse case would be no size reduction .. I did my
compile test on x86-32 btw..
Daniel
Daniel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] WARN_ONCE(): use bool for boolean flag
2009-09-27 17:32 ` Daniel Walker
@ 2009-09-27 17:48 ` Cesar Eduardo Barros
2009-09-27 18:12 ` Cesar Eduardo Barros
0 siblings, 1 reply; 18+ messages in thread
From: Cesar Eduardo Barros @ 2009-09-27 17:48 UTC (permalink / raw)
To: Daniel Walker; +Cc: linux-kernel, Andrew Morton, Roland Dreier
Daniel Walker escreveu:
> On Sun, 2009-09-27 at 14:24 -0300, Cesar Eduardo Barros wrote:
>
>> I took a quick look, and all uses seem to be directly in a boolean
>> context (within an if()), so there would be no problem. Besides, the
>> unlikely() all these macros end with does a double negation, meaning
>> even if it is an int, it will be either 0 or 1 (but I am not sure I am
>> reading these macros right - it seems CONFIG_TRACE_BRANCH_PROFILING
>> turns all unlikely() into likely()).
>>
>> In fact, I was expecting no change at all, since gcc should be able to
>> see it is being treated as a boolean (perhaps I am trusting gcc too
>> much). And to make matters even more confusing, my own test changing all
>> __ret_warn_once to bool and dropping the !! caused an _increase_ of 598
>> bytes (x86-64 defconfig).
>>
>> text data bss dec hex filename
>> 8100553 1207148 991988 10299689 9d2929 vmlinux.warnret.before
>> 8101119 1207180 991988 10300287 9d2b7f vmlinux.warnret.after
>>
>> (And yes, data increased again.)
>
> Did you have the CONFIG_TRACE_BRANCH_PROFILING option enabled for the
> test above?
CONFIG_BRANCH_PROFILE_NONE=y
CONFIG_TRACE_BRANCH_PROFILING does not even appear in the .config.
> If this was just your regular base line config , then that is odd .. I
> also would think worse case would be no size reduction .. I did my
> compile test on x86-32 btw..
Yes, it is very odd. And I tried compiling a small test module to see if
I could see the changes in the assembly output:
#include <linux/module.h>
#include <linux/kernel.h>
void test(int value)
{
WARN_ON_ONCE(value);
}
EXPORT_SYMBOL_GPL(test);
But the assembly output is identical.
I will try looking at the first function which shows a difference in
size (which appears to be handle_irq) and see what I can find.
--
Cesar Eduardo Barros
cesarb@cesarb.net
cesar.barros@gmail.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] WARN_ONCE(): use bool for boolean flag
2009-09-27 17:48 ` Cesar Eduardo Barros
@ 2009-09-27 18:12 ` Cesar Eduardo Barros
2009-09-27 18:25 ` [PATCH] WARN_ONCE(): use bool for condition Cesar Eduardo Barros
0 siblings, 1 reply; 18+ messages in thread
From: Cesar Eduardo Barros @ 2009-09-27 18:12 UTC (permalink / raw)
To: Cesar Eduardo Barros
Cc: Daniel Walker, linux-kernel, Andrew Morton, Roland Dreier
Cesar Eduardo Barros escreveu:
> Daniel Walker escreveu:
>> On Sun, 2009-09-27 at 14:24 -0300, Cesar Eduardo Barros wrote:
>>
>>> In fact, I was expecting no change at all, since gcc should be able
>>> to see it is being treated as a boolean (perhaps I am trusting gcc
>>> too much). And to make matters even more confusing, my own test
>>> changing all __ret_warn_once to bool and dropping the !! caused an
>>> _increase_ of 598 bytes (x86-64 defconfig).
>>>
>>> text data bss dec hex filename
>>> 8100553 1207148 991988 10299689 9d2929 vmlinux.warnret.before
>>> 8101119 1207180 991988 10300287 9d2b7f vmlinux.warnret.after
>>>
>>> (And yes, data increased again.)
>>
>> If this was just your regular base line config , then that is odd .. I
>> also would think worse case would be no size reduction .. I did my
>> compile test on x86-32 btw..
>
> I will try looking at the first function which shows a difference in
> size (which appears to be handle_irq) and see what I can find.
I just took a quick look, and it does seem to be bad code generation
(the gcc on this machine is a bit old). The question is, is the gain in
less buggy gcc versions enough to offset the loss in older and buggier
gcc versions?
The function in question (stack_overflow_check() in
arch/x86/kernel/irq_64.c) has a somewhat complex expression in the call
to WARN_ON, which gcc seems to be pessimizing in this case (it is
storing the boolean in a register just to test it again).
I will send the patch I am using in the next email.
gcc (Ubuntu 4.3.2-1ubuntu12) 4.3.2
--- /dev/fd/63 2009-09-27 14:59:26.124947107 -0300
+++ /dev/fd/62 2009-09-27 14:59:26.144947152 -0300
@@ -246,14 +246,14 @@
pushq %rbp
#APP
# 14
"/scratch/build/cesarb/linux/linux-2.6/arch/x86/include/asm/current.h" 1
- movq %gs:per_cpu__current_task,%rcx
+ movq %gs:per_cpu__current_task,%rax
# 0 "" 2
#NO_APP
movq %rsp, %rbp
pushq %rbx
movl %edi, %ebx
subq $8, %rsp
- movq 8(%rcx), %r8
+ movq 8(%rax), %r8
movq 152(%rsi), %rdx
cmpq %r8, %rdx
jb .L24
@@ -262,28 +262,40 @@
ja .L24
leaq 400(%r8), %rax
cmpq %rax, %rdx
- jae .L24
+ setb %al
+ movzbl %al, %eax
+ jmp .L25
+.L24:
+ xorl %eax, %eax
+.L25:
+ testl %eax, %eax
+ je .L26
cmpb $0, __warned.21424(%rip)
- jne .L24
+ jne .L26
movq %rdx, %r9
- addq $1112, %rcx
- movq $.LC3, %rdx
movl $47, %esi
+ movq $.LC3, %rdx
+#APP
+# 14
"/scratch/build/cesarb/linux/linux-2.6/arch/x86/include/asm/current.h" 1
+ movq %gs:per_cpu__current_task,%rcx
+# 0 "" 2
+#NO_APP
movq $.LC0, %rdi
+ addq $1112, %rcx
xorl %eax, %eax
call warn_slowpath_fmt
movb $1, __warned.21424(%rip)
-.L24:
+.L26:
movl %ebx, %edi
call irq_to_desc
xorl %edx, %edx
testq %rax, %rax
- je .L26
+ je .L28
movq %rax, %rsi
movl %ebx, %edi
call *24(%rax)
movb $1, %dl
-.L26:
+.L28:
movb %dl, %al
popq %rdx
popq %rbx
--
Cesar Eduardo Barros
cesarb@cesarb.net
cesar.barros@gmail.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] WARN_ONCE(): use bool for condition
2009-09-27 18:12 ` Cesar Eduardo Barros
@ 2009-09-27 18:25 ` Cesar Eduardo Barros
2009-09-27 18:28 ` Daniel Walker
2009-09-29 20:59 ` Andrew Morton
0 siblings, 2 replies; 18+ messages in thread
From: Cesar Eduardo Barros @ 2009-09-27 18:25 UTC (permalink / raw)
To: linux-kernel
Cc: Andrew Morton, Roland Dreier, Cesar Eduardo Barros, Daniel Walker
Use the type bool for __ret_warn_once and __ret_warn_on, instead of int
with a double negation. This matches the intent of the code better and
should allow the compiler to generate better code, like in commit
70867453092297be9afb2249e712a1f960ec0a09. However, some versions of gcc
seems to pessimize the code instead when the condition is not trivial.
Cc: Daniel Walker <dwalker@fifo99.com>
Signed-off-by: Cesar Eduardo Barros <cesarb@cesarb.net>
---
arch/avr32/include/asm/bug.h | 2 +-
arch/blackfin/include/asm/bug.h | 2 +-
arch/parisc/include/asm/bug.h | 2 +-
arch/powerpc/include/asm/bug.h | 2 +-
arch/s390/include/asm/bug.h | 2 +-
arch/sh/include/asm/bug.h | 4 ++--
include/asm-generic/bug.h | 12 ++++++------
7 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/arch/avr32/include/asm/bug.h b/arch/avr32/include/asm/bug.h
index 331d45b..c636fec 100644
--- a/arch/avr32/include/asm/bug.h
+++ b/arch/avr32/include/asm/bug.h
@@ -57,7 +57,7 @@
#define WARN_ON(condition) \
({ \
- int __ret_warn_on = !!(condition); \
+ bool __ret_warn_on = (condition); \
if (unlikely(__ret_warn_on)) \
_BUG_OR_WARN(BUGFLAG_WARNING); \
unlikely(__ret_warn_on); \
diff --git a/arch/blackfin/include/asm/bug.h b/arch/blackfin/include/asm/bug.h
index 655e495..3584d52 100644
--- a/arch/blackfin/include/asm/bug.h
+++ b/arch/blackfin/include/asm/bug.h
@@ -46,7 +46,7 @@
#define WARN_ON(condition) \
({ \
- int __ret_warn_on = !!(condition); \
+ bool __ret_warn_on = (condition); \
if (unlikely(__ret_warn_on)) \
_BUG_OR_WARN(BUGFLAG_WARNING); \
unlikely(__ret_warn_on); \
diff --git a/arch/parisc/include/asm/bug.h b/arch/parisc/include/asm/bug.h
index 8cfc553..373e16a 100644
--- a/arch/parisc/include/asm/bug.h
+++ b/arch/parisc/include/asm/bug.h
@@ -74,7 +74,7 @@
#define WARN_ON(x) ({ \
- int __ret_warn_on = !!(x); \
+ bool __ret_warn_on = (x); \
if (__builtin_constant_p(__ret_warn_on)) { \
if (__ret_warn_on) \
__WARN(); \
diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
index 64e1fdc..eda46ac 100644
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h
@@ -95,7 +95,7 @@
} while (0)
#define WARN_ON(x) ({ \
- int __ret_warn_on = !!(x); \
+ bool __ret_warn_on = (x); \
if (__builtin_constant_p(__ret_warn_on)) { \
if (__ret_warn_on) \
__WARN(); \
diff --git a/arch/s390/include/asm/bug.h b/arch/s390/include/asm/bug.h
index 7efd0ab..577b6d1 100644
--- a/arch/s390/include/asm/bug.h
+++ b/arch/s390/include/asm/bug.h
@@ -53,7 +53,7 @@
} while (0)
#define WARN_ON(x) ({ \
- int __ret_warn_on = !!(x); \
+ bool __ret_warn_on = (x); \
if (__builtin_constant_p(__ret_warn_on)) { \
if (__ret_warn_on) \
__EMIT_BUG(BUGFLAG_WARNING); \
diff --git a/arch/sh/include/asm/bug.h b/arch/sh/include/asm/bug.h
index d02c01b..7a7872c 100644
--- a/arch/sh/include/asm/bug.h
+++ b/arch/sh/include/asm/bug.h
@@ -62,7 +62,7 @@ do { \
} while (0)
#define WARN_ON(x) ({ \
- int __ret_warn_on = !!(x); \
+ bool __ret_warn_on = (x); \
if (__builtin_constant_p(__ret_warn_on)) { \
if (__ret_warn_on) \
__WARN(); \
@@ -87,7 +87,7 @@ do { \
} while (0)
#define UNWINDER_BUG_ON(x) ({ \
- int __ret_unwinder_on = !!(x); \
+ bool __ret_unwinder_on = (x); \
if (__builtin_constant_p(__ret_unwinder_on)) { \
if (__ret_unwinder_on) \
UNWINDER_BUG(); \
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 18c435d..9eb001e 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -71,7 +71,7 @@ extern void warn_slowpath_null(const char *file, const int line);
#ifndef WARN_ON
#define WARN_ON(condition) ({ \
- int __ret_warn_on = !!(condition); \
+ bool __ret_warn_on = (condition); \
if (unlikely(__ret_warn_on)) \
__WARN(); \
unlikely(__ret_warn_on); \
@@ -80,7 +80,7 @@ extern void warn_slowpath_null(const char *file, const int line);
#ifndef WARN
#define WARN(condition, format...) ({ \
- int __ret_warn_on = !!(condition); \
+ bool __ret_warn_on = (condition); \
if (unlikely(__ret_warn_on)) \
__WARN_printf(format); \
unlikely(__ret_warn_on); \
@@ -98,14 +98,14 @@ extern void warn_slowpath_null(const char *file, const int line);
#ifndef HAVE_ARCH_WARN_ON
#define WARN_ON(condition) ({ \
- int __ret_warn_on = !!(condition); \
+ bool __ret_warn_on = (condition); \
unlikely(__ret_warn_on); \
})
#endif
#ifndef WARN
#define WARN(condition, format...) ({ \
- int __ret_warn_on = !!(condition); \
+ bool __ret_warn_on = (condition); \
unlikely(__ret_warn_on); \
})
#endif
@@ -114,7 +114,7 @@ extern void warn_slowpath_null(const char *file, const int line);
#define WARN_ON_ONCE(condition) ({ \
static bool __warned; \
- int __ret_warn_once = !!(condition); \
+ bool __ret_warn_once = (condition); \
\
if (unlikely(__ret_warn_once)) \
if (WARN_ON(!__warned)) \
@@ -124,7 +124,7 @@ extern void warn_slowpath_null(const char *file, const int line);
#define WARN_ONCE(condition, format...) ({ \
static bool __warned; \
- int __ret_warn_once = !!(condition); \
+ bool __ret_warn_once = (condition); \
\
if (unlikely(__ret_warn_once)) \
if (WARN(!__warned, format)) \
--
1.6.4.4
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] WARN_ONCE(): use bool for condition
2009-09-27 18:25 ` [PATCH] WARN_ONCE(): use bool for condition Cesar Eduardo Barros
@ 2009-09-27 18:28 ` Daniel Walker
2009-09-27 18:55 ` Cesar Eduardo Barros
2009-09-29 20:59 ` Andrew Morton
1 sibling, 1 reply; 18+ messages in thread
From: Daniel Walker @ 2009-09-27 18:28 UTC (permalink / raw)
To: Cesar Eduardo Barros; +Cc: linux-kernel, Andrew Morton, Roland Dreier
On Sun, 2009-09-27 at 15:25 -0300, Cesar Eduardo Barros wrote:
> - int __ret_warn_on = !!(condition); \
> + bool __ret_warn_on = (condition); \
Did you try it without removing the "!!" ? In my original email I tested
the path of least resistance, and I left the "!!" in there..
Daniel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] WARN_ONCE(): use bool for condition
2009-09-27 18:28 ` Daniel Walker
@ 2009-09-27 18:55 ` Cesar Eduardo Barros
2009-09-27 19:03 ` Daniel Walker
0 siblings, 1 reply; 18+ messages in thread
From: Cesar Eduardo Barros @ 2009-09-27 18:55 UTC (permalink / raw)
To: Daniel Walker; +Cc: linux-kernel, Andrew Morton, Roland Dreier
Daniel Walker escreveu:
> On Sun, 2009-09-27 at 15:25 -0300, Cesar Eduardo Barros wrote:
>> - int __ret_warn_on = !!(condition); \
>> + bool __ret_warn_on = (condition); \
>
> Did you try it without removing the "!!" ? In my original email I tested
> the path of least resistance, and I left the "!!" in there..
Just tried, same result (it seems gcc can easily see past the double
negation).
--
Cesar Eduardo Barros
cesarb@cesarb.net
cesar.barros@gmail.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] WARN_ONCE(): use bool for condition
2009-09-27 18:55 ` Cesar Eduardo Barros
@ 2009-09-27 19:03 ` Daniel Walker
0 siblings, 0 replies; 18+ messages in thread
From: Daniel Walker @ 2009-09-27 19:03 UTC (permalink / raw)
To: Cesar Eduardo Barros; +Cc: linux-kernel, Andrew Morton, Roland Dreier
On Sun, 2009-09-27 at 15:55 -0300, Cesar Eduardo Barros wrote:
> Daniel Walker escreveu:
> > On Sun, 2009-09-27 at 15:25 -0300, Cesar Eduardo Barros wrote:
> >> - int __ret_warn_on = !!(condition); \
> >> + bool __ret_warn_on = (condition); \
> >
> > Did you try it without removing the "!!" ? In my original email I tested
> > the path of least resistance, and I left the "!!" in there..
>
> Just tried, same result (it seems gcc can easily see past the double
> negation).
Ok, I'm not sure it's worth it then.. I'd prefer to move forward and
assume newer compilers, but most kernel hackers use older compilers ..
It's a pretty big size increase too..
Daniel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] WARN_ONCE(): use bool for condition
2009-09-27 18:25 ` [PATCH] WARN_ONCE(): use bool for condition Cesar Eduardo Barros
2009-09-27 18:28 ` Daniel Walker
@ 2009-09-29 20:59 ` Andrew Morton
2009-09-29 23:11 ` Cesar Eduardo Barros
1 sibling, 1 reply; 18+ messages in thread
From: Andrew Morton @ 2009-09-29 20:59 UTC (permalink / raw)
To: Cesar Eduardo Barros; +Cc: linux-kernel, rolandd, cesarb, dwalker
On Sun, 27 Sep 2009 15:25:12 -0300
Cesar Eduardo Barros <cesarb@cesarb.net> wrote:
> Use the type bool for __ret_warn_once and __ret_warn_on, instead of int
> with a double negation. This matches the intent of the code better and
> should allow the compiler to generate better code, like in commit
> 70867453092297be9afb2249e712a1f960ec0a09. However, some versions of gcc
> seems to pessimize the code instead when the condition is not trivial.
>
> Cc: Daniel Walker <dwalker@fifo99.com>
> Signed-off-by: Cesar Eduardo Barros <cesarb@cesarb.net>
> ---
> arch/avr32/include/asm/bug.h | 2 +-
> arch/blackfin/include/asm/bug.h | 2 +-
> arch/parisc/include/asm/bug.h | 2 +-
> arch/powerpc/include/asm/bug.h | 2 +-
> arch/s390/include/asm/bug.h | 2 +-
> arch/sh/include/asm/bug.h | 4 ++--
> include/asm-generic/bug.h | 12 ++++++------
There's a small reject in include/asm-generic/bug.h against current
mainline, easily fixed.
It would be nice if we had some accurate numbers on the kernel size
reductions from this, please. I assume that the patch is still of
benefit in 2.6.32-rc1(2?), but it's always good to confirm.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] WARN_ONCE(): use bool for condition
2009-09-29 20:59 ` Andrew Morton
@ 2009-09-29 23:11 ` Cesar Eduardo Barros
2009-09-29 23:12 ` [PATCH] WARN_ONCE(): use bool for boolean flag Cesar Eduardo Barros
2009-09-29 23:18 ` [PATCH] WARN_ONCE(): use bool for condition Cesar Eduardo Barros
0 siblings, 2 replies; 18+ messages in thread
From: Cesar Eduardo Barros @ 2009-09-29 23:11 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, rolandd, dwalker
Andrew Morton escreveu:
> On Sun, 27 Sep 2009 15:25:12 -0300
> Cesar Eduardo Barros <cesarb@cesarb.net> wrote:
>
>> Use the type bool for __ret_warn_once and __ret_warn_on, instead of int
>> with a double negation. This matches the intent of the code better and
>> should allow the compiler to generate better code, like in commit
>> 70867453092297be9afb2249e712a1f960ec0a09. However, some versions of gcc
>> seems to pessimize the code instead when the condition is not trivial.
>
> There's a small reject in include/asm-generic/bug.h against current
> mainline, easily fixed.
>
> It would be nice if we had some accurate numbers on the kernel size
> reductions from this, please. I assume that the patch is still of
> benefit in 2.6.32-rc1(2?), but it's always good to confirm.
This one was the one where some compilers saw the size reduction and
other compilers saw the size _increase_ due to bad code generation.
The good one was the first post in the original thread, "[PATCH]
WARN_ONCE(): use bool for boolean flag" (the small reject you saw was
probably because that one was not applied before this one, since this
one was generated on top of that one).
In the first patch, Daniel Walker saw a decrease of 504 bytes in IA-32,
and I saw a decrease of 1462 bytes in x86-64 defconfig. I will resend it
as a reply to this email; I think it should be included, as there seems
to be no obvious drawbacks.
For this one, on the other hand, I am not sure whether it should be
included or dropped. While Daniel Walker saw a decrease of 84 bytes in
IA-32, I saw an *increase* of 598 bytes in x86-64 defconfig. It seems
the older compiler I am using (4.3.2-1ubuntu12) generates laughably bad
code for it (setting the variable just to test it again in the next
instruction).
Sorry for the confusion, I should have made more clear that both patches
were separate and meant to be applied in sequence (and that the second
one was under discussion).
--
Cesar Eduardo Barros
cesarb@cesarb.net
cesar.barros@gmail.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] WARN_ONCE(): use bool for boolean flag
2009-09-29 23:11 ` Cesar Eduardo Barros
@ 2009-09-29 23:12 ` Cesar Eduardo Barros
2009-09-30 0:17 ` Andrew Morton
2009-09-29 23:18 ` [PATCH] WARN_ONCE(): use bool for condition Cesar Eduardo Barros
1 sibling, 1 reply; 18+ messages in thread
From: Cesar Eduardo Barros @ 2009-09-29 23:12 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, Roland Dreier, Daniel Walker, Cesar Eduardo Barros
Commit 70867453092297be9afb2249e712a1f960ec0a09 changed printk_once()
to use bool instead of int for its guard variable. Do the same change
to WARN_ONCE() and WARN_ON_ONCE(), for the same reasons.
Signed-off-by: Cesar Eduardo Barros <cesarb@cesarb.net>
---
include/asm-generic/bug.h | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 4b67559..18c435d 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -113,22 +113,22 @@ extern void warn_slowpath_null(const char *file, const int line);
#endif
#define WARN_ON_ONCE(condition) ({ \
- static int __warned; \
+ static bool __warned; \
int __ret_warn_once = !!(condition); \
\
if (unlikely(__ret_warn_once)) \
if (WARN_ON(!__warned)) \
- __warned = 1; \
+ __warned = true; \
unlikely(__ret_warn_once); \
})
#define WARN_ONCE(condition, format...) ({ \
- static int __warned; \
+ static bool __warned; \
int __ret_warn_once = !!(condition); \
\
if (unlikely(__ret_warn_once)) \
if (WARN(!__warned, format)) \
- __warned = 1; \
+ __warned = true; \
unlikely(__ret_warn_once); \
})
--
1.6.4.4
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] WARN_ONCE(): use bool for condition
2009-09-29 23:11 ` Cesar Eduardo Barros
2009-09-29 23:12 ` [PATCH] WARN_ONCE(): use bool for boolean flag Cesar Eduardo Barros
@ 2009-09-29 23:18 ` Cesar Eduardo Barros
1 sibling, 0 replies; 18+ messages in thread
From: Cesar Eduardo Barros @ 2009-09-29 23:18 UTC (permalink / raw)
To: Cesar Eduardo Barros; +Cc: Andrew Morton, linux-kernel, rolandd, dwalker
Cesar Eduardo Barros escreveu:
> Andrew Morton escreveu:
>> On Sun, 27 Sep 2009 15:25:12 -0300
>> Cesar Eduardo Barros <cesarb@cesarb.net> wrote:
>>
>>> Use the type bool for __ret_warn_once and __ret_warn_on, instead of int
>>> with a double negation. This matches the intent of the code better and
>>> should allow the compiler to generate better code, like in commit
>>> 70867453092297be9afb2249e712a1f960ec0a09. However, some versions of gcc
>>> seems to pessimize the code instead when the condition is not trivial.
>>
>> It would be nice if we had some accurate numbers on the kernel size
>> reductions from this, please. I assume that the patch is still of
>> benefit in 2.6.32-rc1(2?), but it's always good to confirm.
>
> In the first patch, Daniel Walker saw a decrease of 504 bytes in IA-32,
> and I saw a decrease of 1462 bytes in x86-64 defconfig. I will resend it
>
> For this one, on the other hand, I am not sure whether it should be
> included or dropped. While Daniel Walker saw a decrease of 84 bytes in
> IA-32, I saw an *increase* of 598 bytes in x86-64 defconfig. It seems
Forgot to mention, all my testing was against v2.6.31-9194-g0d9df25
--
Cesar Eduardo Barros
cesarb@cesarb.net
cesar.barros@gmail.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] WARN_ONCE(): use bool for boolean flag
2009-09-29 23:12 ` [PATCH] WARN_ONCE(): use bool for boolean flag Cesar Eduardo Barros
@ 2009-09-30 0:17 ` Andrew Morton
2009-09-30 0:37 ` Cesar Eduardo Barros
0 siblings, 1 reply; 18+ messages in thread
From: Andrew Morton @ 2009-09-30 0:17 UTC (permalink / raw)
To: Cesar Eduardo Barros; +Cc: linux-kernel, rolandd, dwalker, cesarb
On Tue, 29 Sep 2009 20:12:28 -0300
Cesar Eduardo Barros <cesarb@cesarb.net> wrote:
> Commit 70867453092297be9afb2249e712a1f960ec0a09 changed printk_once()
> to use bool instead of int for its guard variable. Do the same change
> to WARN_ONCE() and WARN_ON_ONCE(), for the same reasons.
>
Again, it would be useful were this changelog to describe the impact
upon kernel code size, please.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] WARN_ONCE(): use bool for boolean flag
2009-09-30 0:17 ` Andrew Morton
@ 2009-09-30 0:37 ` Cesar Eduardo Barros
0 siblings, 0 replies; 18+ messages in thread
From: Cesar Eduardo Barros @ 2009-09-30 0:37 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, Roland Dreier, Daniel Walker, Cesar Eduardo Barros
Commit 70867453092297be9afb2249e712a1f960ec0a09 changed printk_once()
to use bool instead of int for its guard variable. Do the same change
to WARN_ONCE() and WARN_ON_ONCE(), for the same reasons.
This resulted in a reduction of 1462 bytes on a x86-64 defconfig:
text data bss dec hex filename
8101271 1207116 992764 10301151 9d2edf vmlinux.before
8100553 1207148 991988 10299689 9d2929 vmlinux.after
Signed-off-by: Cesar Eduardo Barros <cesarb@cesarb.net>
---
Andrew Morton escreveu:
> Again, it would be useful were this changelog to describe the impact
> upon kernel code size, please.
Is the above good enough?
include/asm-generic/bug.h | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 4b67559..18c435d 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -113,22 +113,22 @@ extern void warn_slowpath_null(const char *file, const int line);
#endif
#define WARN_ON_ONCE(condition) ({ \
- static int __warned; \
+ static bool __warned; \
int __ret_warn_once = !!(condition); \
\
if (unlikely(__ret_warn_once)) \
if (WARN_ON(!__warned)) \
- __warned = 1; \
+ __warned = true; \
unlikely(__ret_warn_once); \
})
#define WARN_ONCE(condition, format...) ({ \
- static int __warned; \
+ static bool __warned; \
int __ret_warn_once = !!(condition); \
\
if (unlikely(__ret_warn_once)) \
if (WARN(!__warned, format)) \
- __warned = 1; \
+ __warned = true; \
unlikely(__ret_warn_once); \
})
--
1.6.4.4
^ permalink raw reply related [flat|nested] 18+ messages in thread
end of thread, other threads:[~2009-09-30 0:38 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-09-27 13:53 [PATCH] WARN_ONCE(): use bool for boolean flag Cesar Eduardo Barros
2009-09-27 14:03 ` Daniel Walker
2009-09-27 15:56 ` Cesar Eduardo Barros
2009-09-27 16:52 ` Daniel Walker
2009-09-27 17:24 ` Cesar Eduardo Barros
2009-09-27 17:32 ` Daniel Walker
2009-09-27 17:48 ` Cesar Eduardo Barros
2009-09-27 18:12 ` Cesar Eduardo Barros
2009-09-27 18:25 ` [PATCH] WARN_ONCE(): use bool for condition Cesar Eduardo Barros
2009-09-27 18:28 ` Daniel Walker
2009-09-27 18:55 ` Cesar Eduardo Barros
2009-09-27 19:03 ` Daniel Walker
2009-09-29 20:59 ` Andrew Morton
2009-09-29 23:11 ` Cesar Eduardo Barros
2009-09-29 23:12 ` [PATCH] WARN_ONCE(): use bool for boolean flag Cesar Eduardo Barros
2009-09-30 0:17 ` Andrew Morton
2009-09-30 0:37 ` Cesar Eduardo Barros
2009-09-29 23:18 ` [PATCH] WARN_ONCE(): use bool for condition Cesar Eduardo Barros
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox