* [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 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
* 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
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