[PATCH 1/4] compiler.h: introduce unused_expression() macro

[PATCH 1/4] compiler.h: introduce unused_expression() macro

[Konstantin Khlebnikov]

Sometimes we want to check some expressions correctness in compile-time without
generating extra code. "(void)(e)" does not work if expression has side-effects.
This patch introduces macro unused_expression() which helps in this situation.

Cast to "long" required because sizeof does not work for bit-fields.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
---
 include/linux/compiler.h |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 923d093..46fbda3 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -310,4 +310,6 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
  */
 #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
 
+#define unused_expression(e) ((void)(sizeof((__force long)(e))))
+
 #endif /* __LINUX_COMPILER_H */

[PATCH 2/4] bug: completely remove code of disabled VM_BUG_ON()

[Konstantin Khlebnikov]

Even if CONFIG_DEBUG_VM=n gcc genereates code for some VM_BUG_ON()

bloat-o-meter: add/remove: 0/1 grow/shrink: 10/120 up/down: 185/-1889 (-1704)

Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
---
 include/linux/mmdebug.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
index c04ecfe..9c9a2a0 100644
--- a/include/linux/mmdebug.h
+++ b/include/linux/mmdebug.h
@@ -4,7 +4,7 @@
 #ifdef CONFIG_DEBUG_VM
 #define VM_BUG_ON(cond) BUG_ON(cond)
 #else
-#define VM_BUG_ON(cond) do { (void)(cond); } while (0)
+#define VM_BUG_ON(cond) unused_expression(cond)
 #endif
 
 #ifdef CONFIG_DEBUG_VIRTUAL

Re: [PATCH 2/4] bug: completely remove code of disabled VM_BUG_ON()

[Geert Uytterhoeven]

On Wed, Apr 25, 2012 at 13:26, Konstantin Khlebnikov
<khlebnikov@openvz.org> wrote:
> Even if CONFIG_DEBUG_VM=n gcc genereates code for some VM_BUG_ON()

That's because of the side effects of the expression

https://lkml.org/lkml/2012/4/25/146

But IRIC, we do want them here?

> bloat-o-meter: add/remove: 0/1 grow/shrink: 10/120 up/down: 185/-1889 (-1704)
>
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
> ---
>  include/linux/mmdebug.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
> index c04ecfe..9c9a2a0 100644
> --- a/include/linux/mmdebug.h
> +++ b/include/linux/mmdebug.h
> @@ -4,7 +4,7 @@
>  #ifdef CONFIG_DEBUG_VM
>  #define VM_BUG_ON(cond) BUG_ON(cond)
>  #else
> -#define VM_BUG_ON(cond) do { (void)(cond); } while (0)
> +#define VM_BUG_ON(cond) unused_expression(cond)
>  #endif
>
>  #ifdef CONFIG_DEBUG_VIRTUAL

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/4] bug: completely remove code of disabled VM_BUG_ON()

[Andrew Morton]

On Wed, 25 Apr 2012 16:40:32 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> On Wed, Apr 25, 2012 at 13:26, Konstantin Khlebnikov
> <khlebnikov@openvz.org> wrote:
> > Even if CONFIG_DEBUG_VM=n gcc genereates code for some VM_BUG_ON()
> 
> That's because of the side effects of the expression
> 
> https://lkml.org/lkml/2012/4/25/146
> 
> But IRIC, we do want them here?
> 

AFAICT (lkml.org appears to be having a meltdown), you've gone and
linked to this very thread.

Please try again, this time avoiding hyperlinks ;)

Re: [PATCH 2/4] bug: completely remove code of disabled VM_BUG_ON()

[Geert Uytterhoeven]

On Fri, Apr 27, 2012 at 00:32, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed, 25 Apr 2012 16:40:32 +0200
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
>> On Wed, Apr 25, 2012 at 13:26, Konstantin Khlebnikov
>> <khlebnikov@openvz.org> wrote:
>> > Even if CONFIG_DEBUG_VM=n gcc genereates code for some VM_BUG_ON()
>>
>> That's because of the side effects of the expression
>>
>> https://lkml.org/lkml/2012/4/25/146
>>
>> But IRIC, we do want them here?
>>
>
> AFAICT (lkml.org appears to be having a meltdown), you've gone and
> linked to this very thread.
>
> Please try again, this time avoiding hyperlinks ;)

Yeah, I noticed after the fact. I wanted to look up the definition of
unused_expression(), which obviously wasn't in my tree yet ;-)

Still, I think people started relyong on the side effects, didn't they?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/4] bug: completely remove code of disabled VM_BUG_ON()

[Andrew Morton]

On Fri, 27 Apr 2012 07:17:50 +0200 Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> On Fri, Apr 27, 2012 at 00:32, Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Wed, 25 Apr 2012 16:40:32 +0200
> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >
> >> On Wed, Apr 25, 2012 at 13:26, Konstantin Khlebnikov
> >> <khlebnikov@openvz.org> wrote:
> >> > Even if CONFIG_DEBUG_VM=n gcc genereates code for some VM_BUG_ON()
> >>
> >> That's because of the side effects of the expression
> >>
> >> https://lkml.org/lkml/2012/4/25/146
> >>
> >> But IRIC, we do want them here?
> >>
> >
> > AFAICT (lkml.org appears to be having a meltdown), you've gone and
> > linked to this very thread.
> >
> > Please try again, this time avoiding hyperlinks ;)
> 
> Yeah, I noticed after the fact. I wanted to look up the definition of
> unused_expression(), which obviously wasn't in my tree yet ;-)
> 
> Still, I think people started relyong on the side effects, didn't they?

I hope not.  If they are, they snuck it past me cleverly!  A quick grep
of mm/*.c looks clean.

[PATCH 3/4] bug: completely remove code of disabled BUG_ON()

[Konstantin Khlebnikov]

Even if CONFIG_BUG=n gcc generates code for some BUG_ON()

bloat-o-meter: add/remove: 2/9 grow/shrink: 25/229 up/down: 1053/-5019 (-3966)

Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
---
 include/asm-generic/bug.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 2520a6e..aadb6fc 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -112,7 +112,7 @@ extern void warn_slowpath_null(const char *file, const int line);
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (condition) ; } while(0)
+#define BUG_ON(condition) unused_expression(condition)
 #endif
 
 #ifndef HAVE_ARCH_WARN_ON

[PATCH 4/4] bug: mark disabled BUG() as unreachable() code

[Konstantin Khlebnikov]

This patch suppress some compiler warnings (if CONFIG_BUG=n)
"warning: control reaches end of non-void function"

Plus now gcc can throw out some dead code.
bloat-o-meter: add/remove: 1/1 grow/shrink: 68/173 up/down: 1569/-5607 (-4038)

Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
---
 include/asm-generic/bug.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index aadb6fc..0b08199 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -108,7 +108,7 @@ extern void warn_slowpath_null(const char *file, const int line);
 
 #else /* !CONFIG_BUG */
 #ifndef HAVE_ARCH_BUG
-#define BUG() do {} while(0)
+#define BUG() unreachable()
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON

Re: [PATCH 4/4] bug: mark disabled BUG() as unreachable() code

[Konstantin Khlebnikov]

Konstantin Khlebnikov wrote:
> This patch suppress some compiler warnings (if CONFIG_BUG=n)
> "warning: control reaches end of non-void function"

With this patch gcc throw out loop at the end of do_exit():

	schedule();
	BUG();
	/* Avoid "noreturn function does return".  */
	for (;;)
		cpu_relax();	/* For when BUG is null */

Is this ok? Probably not, and we need here some BUG_NORETURN() which
really never returns even if CONFIG_BUG=n.

>
> Plus now gcc can throw out some dead code.
> bloat-o-meter: add/remove: 1/1 grow/shrink: 68/173 up/down: 1569/-5607 (-4038)
>
> Signed-off-by: Konstantin Khlebnikov<khlebnikov@openvz.org>
> ---
>   include/asm-generic/bug.h |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> index aadb6fc..0b08199 100644
> --- a/include/asm-generic/bug.h
> +++ b/include/asm-generic/bug.h
> @@ -108,7 +108,7 @@ extern void warn_slowpath_null(const char *file, const int line);
>
>   #else /* !CONFIG_BUG */
>   #ifndef HAVE_ARCH_BUG
> -#define BUG() do {} while(0)
> +#define BUG() unreachable()
>   #endif
>
>   #ifndef HAVE_ARCH_BUG_ON
>

Re: [PATCH 4/4] bug: mark disabled BUG() as unreachable() code

[Linus Torvalds]

On Fri, Apr 27, 2012 at 10:10 PM, Konstantin Khlebnikov
<khlebnikov@openvz.org> wrote:
>
> With this patch gcc throw out loop at the end of do_exit():

I think that's the least of our problems - that loop only exists to
get rid of a warning, and the return from that schedule() really is
supposed to be unreachable().

What makes me much more worried is that iirc, gcc will use
"unreachable()" to also get rid of function epilogues etc, which is
perfectly fine if it really is something entirely unreachable. But the
kernel use of BUG() is often as a kind of "assert()", and if gcc
generates actively bad code for it (and it does, with unreachable()),
it turns the turned-off BUG() into something *really* horrible that
makes for a debugging disaster.

So some pattern like

   if (badness)
      BUG();

would generate code that is actively insane, instead of (like now)
generating basically a no-op.

And THAT makes me go "Eww". "unreachable()" should only be used in
situations that really are very very unreachable() (eg after an "asm"
that goes off to la-la-land or a call to "exit()" in user land etc).

The kernel kinds of "assert" is *hopefully* not reachable, but if we
ever reach it, we don't want to make things worse. The BUG() was there
to give us a nicer debug message about what went wrong, and I think
your patch actively destroys that whole thing and makes for something
*worse*.

                    Linus

Re: [PATCH 4/4] bug: mark disabled BUG() as unreachable() code

[Andrew Morton]

On Sat, 28 Apr 2012 09:10:18 +0400 Konstantin Khlebnikov <khlebnikov@openvz.org> wrote:

> Konstantin Khlebnikov wrote:
> > This patch suppress some compiler warnings (if CONFIG_BUG=n)
> > "warning: control reaches end of non-void function"
> 
> With this patch gcc throw out loop at the end of do_exit():
> 
> 	schedule();
> 	BUG();
> 	/* Avoid "noreturn function does return".  */
> 	for (;;)
> 		cpu_relax();	/* For when BUG is null */
> 
> Is this ok? Probably not, and we need here some BUG_NORETURN() which
> really never returns even if CONFIG_BUG=n.

Probably we should do away with CONFIG_BUG.

CONFIG_BUG=n causes various compile-time warnings to come out when the
execution proceeds into places where we didn't intend and these aren't
worth fixing.

More seriously, I rather doubt that anyone ever sets it to n.  And it
would be a pretty dumb thing to do - if your kernel has seriously
malfunctioned and it *knows* that it malfunctioned, it will just
blunder on anyway not telling anyone about it.

It doesn't seem worth any effort to support this thing.

Re: [PATCH 1/4] compiler.h: introduce unused_expression() macro

[Cong Wang]

On 04/25/2012 07:26 PM, Konstantin Khlebnikov wrote:
> Sometimes we want to check some expressions correctness in compile-time without
> generating extra code. "(void)(e)" does not work if expression has side-effects.
> This patch introduces macro unused_expression() which helps in this situation.
>

Interesting, I am wondering why gcc doesn't eliminate the code as we 
pass either -O2 or -Os to it.

Re: [PATCH 1/4] compiler.h: introduce unused_expression() macro

[Konstantin Khlebnikov]

Cong Wang wrote:
> On 04/25/2012 07:26 PM, Konstantin Khlebnikov wrote:
>> Sometimes we want to check some expressions correctness in compile-time without
>> generating extra code. "(void)(e)" does not work if expression has side-effects.
>> This patch introduces macro unused_expression() which helps in this situation.
>>
>
> Interesting, I am wondering why gcc doesn't eliminate the code as we
> pass either -O2 or -Os to it.

It cannot do this if expression has some side-effects, for example if it contains BUG().

Re: [PATCH 1/4] compiler.h: introduce unused_expression() macro

[Andrew Morton]

On Wed, 25 Apr 2012 15:26:23 +0400
Konstantin Khlebnikov <khlebnikov@openvz.org> wrote:

> Sometimes we want to check some expressions correctness in compile-time without
> generating extra code. "(void)(e)" does not work if expression has side-effects.
> This patch introduces macro unused_expression() which helps in this situation.
> 
> Cast to "long" required because sizeof does not work for bit-fields.
> 
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
> ---
>  include/linux/compiler.h |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 923d093..46fbda3 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -310,4 +310,6 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>   */
>  #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
>  
> +#define unused_expression(e) ((void)(sizeof((__force long)(e))))
> +

hm, maybe.

Thing is, if anyone ever has an expression-with-side-effects within
conditionally-compiled code then they probably have a bug, don't they?
I mean, as an extreme example

	VM_BUG_ON(do_something_important());

is a nice little hand-grenade.  Your patch will cause that (bad) code
to newly fail at runtime, but our coverage testing is so awful that it
would take a long time for the bug to be discovered.

It would be nice if we could cause the build to warn or outright fail
if the unused_expression() argument would have caused any code
generation.  But I can't suggest how to do that.


Your changelogs assert that gcc is emitting code for these expressions,
but details are not presented.  Please give examples - where is this
code generation coming from, what is causing it?


Bottom line: are these patches a workaround for gcc inadequacies, or
are they a bandaid covering up poor kernel code?

Re: [PATCH 1/4] compiler.h: introduce unused_expression() macro

[Konstantin Khlebnikov]

Andrew Morton wrote:
> On Wed, 25 Apr 2012 15:26:23 +0400
> Konstantin Khlebnikov<khlebnikov@openvz.org>  wrote:
>
>> Sometimes we want to check some expressions correctness in compile-time without
>> generating extra code. "(void)(e)" does not work if expression has side-effects.
>> This patch introduces macro unused_expression() which helps in this situation.
>>
>> Cast to "long" required because sizeof does not work for bit-fields.
>>
>> Signed-off-by: Konstantin Khlebnikov<khlebnikov@openvz.org>
>> ---
>>   include/linux/compiler.h |    2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
>> index 923d093..46fbda3 100644
>> --- a/include/linux/compiler.h
>> +++ b/include/linux/compiler.h
>> @@ -310,4 +310,6 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>>    */
>>   #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
>>
>> +#define unused_expression(e) ((void)(sizeof((__force long)(e))))
>> +
>
> hm, maybe.
>
> Thing is, if anyone ever has an expression-with-side-effects within
> conditionally-compiled code then they probably have a bug, don't they?
> I mean, as an extreme example
>
> 	VM_BUG_ON(do_something_important());
>
> is a nice little hand-grenade.  Your patch will cause that (bad) code
> to newly fail at runtime, but our coverage testing is so awful that it
> would take a long time for the bug to be discovered.
>
> It would be nice if we could cause the build to warn or outright fail
> if the unused_expression() argument would have caused any code
> generation.  But I can't suggest how to do that.
>
>
> Your changelogs assert that gcc is emitting code for these expressions,
> but details are not presented.  Please give examples - where is this
> code generation coming from, what is causing it?

for example  VM_BUG_ON(!PageCompound(page) || !PageHead(page)); in
do_huge_pmd_wp_page() generates 114 bytes.

But they mostly disappears if I replace it with
-VM_BUG_ON(!PageCompound(page) || !PageHead(page));
+VM_BUG_ON(!PageCompound(page));
+VM_BUG_ON(!PageHead(page));
weird...

add/remove: 0/0 grow/shrink: 7/97 up/down: 135/-1784 (-1649)
function                                     old     new   delta
make_alloc_exact                             160     210     +50
deactivate_slab                             1302    1350     +48
get_page_from_freelist                      2043    2059     +16
split_free_page                              348     356      +8
add_to_swap                                  113     118      +5
__do_fault                                  1152    1157      +5
page_referenced_one                          415     418      +3
lru_cache_add_lru                             66      65      -1
static.get_partial_node                      593     591      -2
static.copy_user_highpage                     93      91      -2
unuse_mm                                    1559    1556      -3
unlock_page                                   49      46      -3
try_to_merge_with_ksm_page                  1540    1537      -3
static.update_isolated_counts                335     332      -3
special_mapping_fault                        130     127      -3
set_page_dirty_balance                        98      95      -3
mem_cgroup_uncharge_cache_page                21      18      -3
mem_cgroup_newpage_charge                     38      35      -3
add_page_to_unevictable_list                 189     186      -3
__get_user_pages                            1287    1284      -3
putback_lru_page                             216     210      -6
follow_page                                 1020    1014      -6
__activate_page                              375     369      -6
update_and_free_page                         121     114      -7
try_to_munlock                                72      65      -7
shmem_truncate_range                        1586    1579      -7
put_page                                      56      49      -7
page_evictable                               145     138      -7
copy_huge_pmd                                411     404      -7
add_to_page_cache_locked                     292     285      -7
__free_pages_bootmem                         122     115      -7
zap_huge_pmd                                 244     236      -8
try_get_mem_cgroup_from_page                 326     318      -8
static.__page_cache_release                  277     269      -8
shmem_find_get_pages_and_swap                353     345      -8
shmem_file_aio_read                          886     878      -8
shmem_add_to_page_cache                      340     332      -8
reuse_swap_page                              239     231      -8
new_page_node                                100      92      -8
mem_cgroup_move_account                      425     417      -8
ksm_migrate_page                              69      61      -8
invalidate_inode_page                        186     178      -8
hugetlb_cow                                 1145    1137      -8
get_page                                      50      42      -8
find_get_pages_tag                           449     441      -8
find_get_pages                               402     394      -8
enabled_show                                 184     176      -8
dequeue_huge_page_node                       149     141      -8
defrag_show                                  184     176      -8
__alloc_pages_nodemask                      2215    2207      -8
follow_trans_huge_pmd                        149     140      -9
__delete_from_swap_cache                      91      82      -9
swap_readpage                                 95      85     -10
static.get_mctgt_type_thp                    178     167     -11
free_pages                                    74      63     -11
__pagevec_lru_add_fn                         243     231     -12
new_node_page                                 57      43     -14
__put_anon_vma                               161     147     -14
rmap_walk_ksm                                321     305     -16
rmap_walk                                    575     559     -16
replace_page_cache_page                      310     294     -16
remove_migration_pte                         632     616     -16
release_pages                                484     468     -16
page_add_new_anon_rmap                       237     221     -16
move_active_pages_to_lru                     382     366     -16
migrate_pages                               1283    1267     -16
migrate_page_copy                            469     453     -16
mem_cgroup_charge_common                     169     153     -16
ksm_scan_thread                             3164    3148     -16
follow_hugetlb_page                          825     809     -16
find_get_pages_contig                        431     415     -16
do_wp_page                                  1829    1813     -16
clear_page_dirty_for_io                      269     253     -16
alloc_fresh_huge_page                        254     238     -16
__mem_cgroup_try_charge                     2425    2409     -16
__isolate_lru_page                           213     197     -16
__add_to_swap_cache                          203     187     -16
lru_add_page_tail                            392     373     -19
__mem_cgroup_begin_update_page_stat          161     140     -21
remove_mapping                                69      46     -23
__split_huge_page_pmd                        180     157     -23
alloc_buddy_huge_page                        333     309     -24
static.isolate_lru_pages                     399     373     -26
split_page                                   101      73     -28
__remove_mapping                             311     283     -28
__mem_cgroup_uncharge_common                 787     757     -30
putback_inactive_pages                       641     609     -32
do_page_add_anon_rmap                        247     215     -32
__get_page_tail                              274     242     -32
try_to_unmap                                 135     100     -35
mem_cgroup_prepare_migration                 443     408     -35
new_slab                                     763     725     -38
hugetlb_acct_memory                          827     786     -41
put_compound_page                            343     301     -42
__rmqueue                                   1093    1050     -43
page_move_anon_rmap                           71      26     -45
free_pcppages_bulk                           956     911     -45
static.migrate_page_move_mapping             563     513     -50
migrate_huge_page_move_mapping               372     322     -50
free_one_page                                819     769     -50
isolate_lru_page                             383     326     -57
shrink_page_list                            2313    2233     -80
khugepaged                                  5028    4947     -81
do_huge_pmd_wp_page                         1747    1628    -119


>
>
> Bottom line: are these patches a workaround for gcc inadequacies, or
> are they a bandaid covering up poor kernel code?
>

gcc do weird things, but sometimes it really cannot throw out code.
I hope we can do this safely for VM_BUG_ON(), anyway nobody disables real BUG_ON()

Re: [PATCH 1/4] compiler.h: introduce unused_expression() macro

[Andrew Morton]

On Fri, 27 Apr 2012 13:55:06 +0400
Konstantin Khlebnikov <khlebnikov@openvz.org> wrote:

> Andrew Morton wrote:
> > On Wed, 25 Apr 2012 15:26:23 +0400
> > Konstantin Khlebnikov<khlebnikov@openvz.org>  wrote:
> 
> for example  VM_BUG_ON(!PageCompound(page) || !PageHead(page)); in
> do_huge_pmd_wp_page() generates 114 bytes.
> 
> But they mostly disappears if I replace it with
> -VM_BUG_ON(!PageCompound(page) || !PageHead(page));
> +VM_BUG_ON(!PageCompound(page));
> +VM_BUG_ON(!PageHead(page));
> weird...
> 
> add/remove: 0/0 grow/shrink: 7/97 up/down: 135/-1784 (-1649)

OK, thanks.  I'm inclined to apply the patchset as-is.  If the
apparently mythical use of side-effects in VM_BUG_ON() really exist
then we deserve everything which happens to us as a result ;)

Please update the changelogs so they cover all the points which have been
discussed, add my little code comment then send out a v2, being sure to
cc everyone who has been involved in the discussion?

Re: [PATCH 1/4] compiler.h: introduce unused_expression() macro

[Andrew Morton]

On Wed, 25 Apr 2012 15:26:23 +0400
Konstantin Khlebnikov <khlebnikov@openvz.org> wrote:

> Sometimes we want to check some expressions correctness in compile-time without
> generating extra code. "(void)(e)" does not work if expression has side-effects.
> This patch introduces macro unused_expression() which helps in this situation.
> 
> Cast to "long" required because sizeof does not work for bit-fields.
> 
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
> ---
>  include/linux/compiler.h |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 923d093..46fbda3 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -310,4 +310,6 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>   */
>  #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
>  
> +#define unused_expression(e) ((void)(sizeof((__force long)(e))))
> +

btw, please let's not add undocumented stuff, particularly obscure
undocumented stuff.

I typed this in:

--- a/include/linux/compiler.h~compilerh-introduce-unused_expression-macro-fix
+++ a/include/linux/compiler.h
@@ -310,6 +310,11 @@ void ftrace_likely_update(struct ftrace_
  */
 #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
 
+/*
+ * unused_expression() permits the compiler to check the validity of the
+ * expression but avoids the generation of any code, even if that expression
+ * has side-effects.
+ */
 #define unused_expression(e) ((void)(sizeof((__force long)(e))))
 
 #endif /* __LINUX_COMPILER_H */

but then decided I didn't want to apply the patches (yet?).

Re: [PATCH 1/4] compiler.h: introduce unused_expression() macro

[Konstantin Khlebnikov]

Andrew Morton wrote:
> On Wed, 25 Apr 2012 15:26:23 +0400
> Konstantin Khlebnikov<khlebnikov@openvz.org>  wrote:
>
>> Sometimes we want to check some expressions correctness in compile-time without
>> generating extra code. "(void)(e)" does not work if expression has side-effects.
>> This patch introduces macro unused_expression() which helps in this situation.
>>
>> Cast to "long" required because sizeof does not work for bit-fields.
>>
>> Signed-off-by: Konstantin Khlebnikov<khlebnikov@openvz.org>
>> ---
>>   include/linux/compiler.h |    2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
>> index 923d093..46fbda3 100644
>> --- a/include/linux/compiler.h
>> +++ b/include/linux/compiler.h
>> @@ -310,4 +310,6 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>>    */
>>   #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
>>
>> +#define unused_expression(e) ((void)(sizeof((__force long)(e))))
>> +
>
> btw, please let's not add undocumented stuff, particularly obscure
> undocumented stuff.
>
> I typed this in:

Thanks!

>
> --- a/include/linux/compiler.h~compilerh-introduce-unused_expression-macro-fix
> +++ a/include/linux/compiler.h
> @@ -310,6 +310,11 @@ void ftrace_likely_update(struct ftrace_
>    */
>   #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
>
> +/*
> + * unused_expression() permits the compiler to check the validity of the
> + * expression but avoids the generation of any code, even if that expression
> + * has side-effects.
> + */
>   #define unused_expression(e) ((void)(sizeof((__force long)(e))))
>
>   #endif /* __LINUX_COMPILER_H */
>
> but then decided I didn't want to apply the patches (yet?).

I'm OK with that. But probably Hugh Dickins (now in CC) aggressively wants to
remove side-effects of VM_BUG_ON's in case CONFIG_DEBUG_VM=n

> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] compiler.h: introduce unused_expression() macro

[H. Peter Anvin]

On 04/25/2012 04:26 AM, Konstantin Khlebnikov wrote:
> Sometimes we want to check some expressions correctness in compile-time without
> generating extra code. "(void)(e)" does not work if expression has side-effects.
> This patch introduces macro unused_expression() which helps in this situation.
> 
> Cast to "long" required because sizeof does not work for bit-fields.
> 
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
> ---
>  include/linux/compiler.h |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 923d093..46fbda3 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -310,4 +310,6 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>   */
>  #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
>  
> +#define unused_expression(e) ((void)(sizeof((__force long)(e))))
> +

OK, bikeshedding a bit:

"unused_expression()" doesn't sound like something that inhibits side
effects, especially since "unused" applied to an argument or variable
means "don't make a fuss over this not being used."

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH 1/4] compiler.h: introduce unused_expression() macro

[Konstantin Khlebnikov]

H. Peter Anvin wrote:
> On 04/25/2012 04:26 AM, Konstantin Khlebnikov wrote:
>> Sometimes we want to check some expressions correctness in compile-time without
>> generating extra code. "(void)(e)" does not work if expression has side-effects.
>> This patch introduces macro unused_expression() which helps in this situation.
>>
>> Cast to "long" required because sizeof does not work for bit-fields.
>>
>> Signed-off-by: Konstantin Khlebnikov<khlebnikov@openvz.org>
>> ---
>>   include/linux/compiler.h |    2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
>> index 923d093..46fbda3 100644
>> --- a/include/linux/compiler.h
>> +++ b/include/linux/compiler.h
>> @@ -310,4 +310,6 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>>    */
>>   #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
>>
>> +#define unused_expression(e) ((void)(sizeof((__force long)(e))))
>> +
>
> OK, bikeshedding a bit:
>
> "unused_expression()" doesn't sound like something that inhibits side
> effects, especially since "unused" applied to an argument or variable
> means "don't make a fuss over this not being used."
>
> 	-hpa
>

Ok, in v2 I'll rename it into BUILD_BUG_ON_INVALID()

[PATCH v2 1/2] bug: introduce BUILD_BUG_ON_INVALID() macro

[Konstantin Khlebnikov]

Sometimes we want to check some expressions correctness in compile-time.
"(void)(e);" or "if (e);" can be dangerous if expression has side-effects, and
gcc sometimes generates a lot of code, even if the expression has no effect.

This patch introduces macro BUILD_BUG_ON_INVALID() for such checks,
it forces a compilation error if expression is invalid without any extra code.

[Cast to "long" required because sizeof does not work for bit-fields.]

Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
---
 include/linux/bug.h |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index 72961c3..aaac4bb 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -30,6 +30,13 @@ struct pt_regs;
 #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
 #define BUILD_BUG_ON_NULL(e) ((void *)sizeof(struct { int:-!!(e); }))
 
+/*
+ * BUILD_BUG_ON_INVALID() permits the compiler to check the validity of the
+ * expression but avoids the generation of any code, even if that expression
+ * has side-effects.
+ */
+#define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e))))
+
 /**
  * BUILD_BUG_ON - break compile if a condition is true.
  * @condition: the condition which the compiler should know is false.

[PATCH v2 1/2] bug: introduce BUILD_BUG_ON_INVALID() macro

[Konstantin Khlebnikov]

Sometimes we want to check some expressions correctness in compile-time.
"(void)(e);" or "if (e);" can be dangerous if expression has side-effects, and
gcc sometimes generates a lot of code, even if the expression has no effect.

This patch introduces macro BUILD_BUG_ON_INVALID() for such checks,
it forces a compilation error if expression is invalid without any extra code.

[Cast to "long" required because sizeof does not work for bit-fields.]

Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
---
 include/linux/bug.h |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index 72961c3..aaac4bb 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -30,6 +30,13 @@ struct pt_regs;
 #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
 #define BUILD_BUG_ON_NULL(e) ((void *)sizeof(struct { int:-!!(e); }))
 
+/*
+ * BUILD_BUG_ON_INVALID() permits the compiler to check the validity of the
+ * expression but avoids the generation of any code, even if that expression
+ * has side-effects.
+ */
+#define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e))))
+
 /**
  * BUILD_BUG_ON - break compile if a condition is true.
  * @condition: the condition which the compiler should know is false.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2 2/2] bug: completely remove code of disabled VM_BUG_ON()

[Konstantin Khlebnikov]

Even if CONFIG_DEBUG_VM=n gcc genereates code for some VM_BUG_ON()

for example VM_BUG_ON(!PageCompound(page) || !PageHead(page)); in
do_huge_pmd_wp_page() generates 114 bytes of code.
But they mostly disappears when I split this VM_BUG_ON into two:
-VM_BUG_ON(!PageCompound(page) || !PageHead(page));
+VM_BUG_ON(!PageCompound(page));
+VM_BUG_ON(!PageHead(page));
weird... but anyway after this patch code disappears completely.

add/remove: 0/0 grow/shrink: 7/97 up/down: 135/-1784 (-1649)

Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>

---

add/remove: 0/0 grow/shrink: 7/97 up/down: 135/-1784 (-1649)
function                                     old     new   delta
make_alloc_exact                             160     210     +50
deactivate_slab                             1302    1350     +48
get_page_from_freelist                      2043    2059     +16
split_free_page                              348     356      +8
add_to_swap                                  113     118      +5
__do_fault                                  1152    1157      +5
page_referenced_one                          415     418      +3
lru_cache_add_lru                             66      65      -1
static.get_partial_node                      593     591      -2
static.copy_user_highpage                     93      91      -2
unuse_mm                                    1559    1556      -3
unlock_page                                   49      46      -3
try_to_merge_with_ksm_page                  1540    1537      -3
static.update_isolated_counts                335     332      -3
special_mapping_fault                        130     127      -3
set_page_dirty_balance                        98      95      -3
mem_cgroup_uncharge_cache_page                21      18      -3
mem_cgroup_newpage_charge                     38      35      -3
add_page_to_unevictable_list                 189     186      -3
__get_user_pages                            1287    1284      -3
putback_lru_page                             216     210      -6
follow_page                                 1020    1014      -6
__activate_page                              375     369      -6
update_and_free_page                         121     114      -7
try_to_munlock                                72      65      -7
shmem_truncate_range                        1586    1579      -7
put_page                                      56      49      -7
page_evictable                               145     138      -7
copy_huge_pmd                                411     404      -7
add_to_page_cache_locked                     292     285      -7
__free_pages_bootmem                         122     115      -7
zap_huge_pmd                                 244     236      -8
try_get_mem_cgroup_from_page                 326     318      -8
static.__page_cache_release                  277     269      -8
shmem_find_get_pages_and_swap                353     345      -8
shmem_file_aio_read                          886     878      -8
shmem_add_to_page_cache                      340     332      -8
reuse_swap_page                              239     231      -8
new_page_node                                100      92      -8
mem_cgroup_move_account                      425     417      -8
ksm_migrate_page                              69      61      -8
invalidate_inode_page                        186     178      -8
hugetlb_cow                                 1145    1137      -8
get_page                                      50      42      -8
find_get_pages_tag                           449     441      -8
find_get_pages                               402     394      -8
enabled_show                                 184     176      -8
dequeue_huge_page_node                       149     141      -8
defrag_show                                  184     176      -8
__alloc_pages_nodemask                      2215    2207      -8
follow_trans_huge_pmd                        149     140      -9
__delete_from_swap_cache                      91      82      -9
swap_readpage                                 95      85     -10
static.get_mctgt_type_thp                    178     167     -11
free_pages                                    74      63     -11
__pagevec_lru_add_fn                         243     231     -12
new_node_page                                 57      43     -14
__put_anon_vma                               161     147     -14
rmap_walk_ksm                                321     305     -16
rmap_walk                                    575     559     -16
replace_page_cache_page                      310     294     -16
remove_migration_pte                         632     616     -16
release_pages                                484     468     -16
page_add_new_anon_rmap                       237     221     -16
move_active_pages_to_lru                     382     366     -16
migrate_pages                               1283    1267     -16
migrate_page_copy                            469     453     -16
mem_cgroup_charge_common                     169     153     -16
ksm_scan_thread                             3164    3148     -16
follow_hugetlb_page                          825     809     -16
find_get_pages_contig                        431     415     -16
do_wp_page                                  1829    1813     -16
clear_page_dirty_for_io                      269     253     -16
alloc_fresh_huge_page                        254     238     -16
__mem_cgroup_try_charge                     2425    2409     -16
__isolate_lru_page                           213     197     -16
__add_to_swap_cache                          203     187     -16
lru_add_page_tail                            392     373     -19
__mem_cgroup_begin_update_page_stat          161     140     -21
remove_mapping                                69      46     -23
__split_huge_page_pmd                        180     157     -23
alloc_buddy_huge_page                        333     309     -24
static.isolate_lru_pages                     399     373     -26
split_page                                   101      73     -28
__remove_mapping                             311     283     -28
__mem_cgroup_uncharge_common                 787     757     -30
putback_inactive_pages                       641     609     -32
do_page_add_anon_rmap                        247     215     -32
__get_page_tail                              274     242     -32
try_to_unmap                                 135     100     -35
mem_cgroup_prepare_migration                 443     408     -35
new_slab                                     763     725     -38
hugetlb_acct_memory                          827     786     -41
put_compound_page                            343     301     -42
__rmqueue                                   1093    1050     -43
page_move_anon_rmap                           71      26     -45
free_pcppages_bulk                           956     911     -45
static.migrate_page_move_mapping             563     513     -50
migrate_huge_page_move_mapping               372     322     -50
free_one_page                                819     769     -50
isolate_lru_page                             383     326     -57
shrink_page_list                            2313    2233     -80
khugepaged                                  5028    4947     -81
do_huge_pmd_wp_page                         1747    1628    -119
---
 include/linux/mmdebug.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
index c04ecfe..580bd58 100644
--- a/include/linux/mmdebug.h
+++ b/include/linux/mmdebug.h
@@ -4,7 +4,7 @@
 #ifdef CONFIG_DEBUG_VM
 #define VM_BUG_ON(cond) BUG_ON(cond)
 #else
-#define VM_BUG_ON(cond) do { (void)(cond); } while (0)
+#define VM_BUG_ON(cond) BUILD_BUG_ON_INVALID(cond)
 #endif
 
 #ifdef CONFIG_DEBUG_VIRTUAL

[PATCH v2 2/2] bug: completely remove code of disabled VM_BUG_ON()

[Konstantin Khlebnikov]

Even if CONFIG_DEBUG_VM=n gcc genereates code for some VM_BUG_ON()

for example VM_BUG_ON(!PageCompound(page) || !PageHead(page)); in
do_huge_pmd_wp_page() generates 114 bytes of code.
But they mostly disappears when I split this VM_BUG_ON into two:
-VM_BUG_ON(!PageCompound(page) || !PageHead(page));
+VM_BUG_ON(!PageCompound(page));
+VM_BUG_ON(!PageHead(page));
weird... but anyway after this patch code disappears completely.

add/remove: 0/0 grow/shrink: 7/97 up/down: 135/-1784 (-1649)

Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>

---

add/remove: 0/0 grow/shrink: 7/97 up/down: 135/-1784 (-1649)
function                                     old     new   delta
make_alloc_exact                             160     210     +50
deactivate_slab                             1302    1350     +48
get_page_from_freelist                      2043    2059     +16
split_free_page                              348     356      +8
add_to_swap                                  113     118      +5
__do_fault                                  1152    1157      +5
page_referenced_one                          415     418      +3
lru_cache_add_lru                             66      65      -1
static.get_partial_node                      593     591      -2
static.copy_user_highpage                     93      91      -2
unuse_mm                                    1559    1556      -3
unlock_page                                   49      46      -3
try_to_merge_with_ksm_page                  1540    1537      -3
static.update_isolated_counts                335     332      -3
special_mapping_fault                        130     127      -3
set_page_dirty_balance                        98      95      -3
mem_cgroup_uncharge_cache_page                21      18      -3
mem_cgroup_newpage_charge                     38      35      -3
add_page_to_unevictable_list                 189     186      -3
__get_user_pages                            1287    1284      -3
putback_lru_page                             216     210      -6
follow_page                                 1020    1014      -6
__activate_page                              375     369      -6
update_and_free_page                         121     114      -7
try_to_munlock                                72      65      -7
shmem_truncate_range                        1586    1579      -7
put_page                                      56      49      -7
page_evictable                               145     138      -7
copy_huge_pmd                                411     404      -7
add_to_page_cache_locked                     292     285      -7
__free_pages_bootmem                         122     115      -7
zap_huge_pmd                                 244     236      -8
try_get_mem_cgroup_from_page                 326     318      -8
static.__page_cache_release                  277     269      -8
shmem_find_get_pages_and_swap                353     345      -8
shmem_file_aio_read                          886     878      -8
shmem_add_to_page_cache                      340     332      -8
reuse_swap_page                              239     231      -8
new_page_node                                100      92      -8
mem_cgroup_move_account                      425     417      -8
ksm_migrate_page                              69      61      -8
invalidate_inode_page                        186     178      -8
hugetlb_cow                                 1145    1137      -8
get_page                                      50      42      -8
find_get_pages_tag                           449     441      -8
find_get_pages                               402     394      -8
enabled_show                                 184     176      -8
dequeue_huge_page_node                       149     141      -8
defrag_show                                  184     176      -8
__alloc_pages_nodemask                      2215    2207      -8
follow_trans_huge_pmd                        149     140      -9
__delete_from_swap_cache                      91      82      -9
swap_readpage                                 95      85     -10
static.get_mctgt_type_thp                    178     167     -11
free_pages                                    74      63     -11
__pagevec_lru_add_fn                         243     231     -12
new_node_page                                 57      43     -14
__put_anon_vma                               161     147     -14
rmap_walk_ksm                                321     305     -16
rmap_walk                                    575     559     -16
replace_page_cache_page                      310     294     -16
remove_migration_pte                         632     616     -16
release_pages                                484     468     -16
page_add_new_anon_rmap                       237     221     -16
move_active_pages_to_lru                     382     366     -16
migrate_pages                               1283    1267     -16
migrate_page_copy                            469     453     -16
mem_cgroup_charge_common                     169     153     -16
ksm_scan_thread                             3164    3148     -16
follow_hugetlb_page                          825     809     -16
find_get_pages_contig                        431     415     -16
do_wp_page                                  1829    1813     -16
clear_page_dirty_for_io                      269     253     -16
alloc_fresh_huge_page                        254     238     -16
__mem_cgroup_try_charge                     2425    2409     -16
__isolate_lru_page                           213     197     -16
__add_to_swap_cache                          203     187     -16
lru_add_page_tail                            392     373     -19
__mem_cgroup_begin_update_page_stat          161     140     -21
remove_mapping                                69      46     -23
__split_huge_page_pmd                        180     157     -23
alloc_buddy_huge_page                        333     309     -24
static.isolate_lru_pages                     399     373     -26
split_page                                   101      73     -28
__remove_mapping                             311     283     -28
__mem_cgroup_uncharge_common                 787     757     -30
putback_inactive_pages                       641     609     -32
do_page_add_anon_rmap                        247     215     -32
__get_page_tail                              274     242     -32
try_to_unmap                                 135     100     -35
mem_cgroup_prepare_migration                 443     408     -35
new_slab                                     763     725     -38
hugetlb_acct_memory                          827     786     -41
put_compound_page                            343     301     -42
__rmqueue                                   1093    1050     -43
page_move_anon_rmap                           71      26     -45
free_pcppages_bulk                           956     911     -45
static.migrate_page_move_mapping             563     513     -50
migrate_huge_page_move_mapping               372     322     -50
free_one_page                                819     769     -50
isolate_lru_page                             383     326     -57
shrink_page_list                            2313    2233     -80
khugepaged                                  5028    4947     -81
do_huge_pmd_wp_page                         1747    1628    -119
---
 include/linux/mmdebug.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
index c04ecfe..580bd58 100644
--- a/include/linux/mmdebug.h
+++ b/include/linux/mmdebug.h
@@ -4,7 +4,7 @@
 #ifdef CONFIG_DEBUG_VM
 #define VM_BUG_ON(cond) BUG_ON(cond)
 #else
-#define VM_BUG_ON(cond) do { (void)(cond); } while (0)
+#define VM_BUG_ON(cond) BUILD_BUG_ON_INVALID(cond)
 #endif
 
 #ifdef CONFIG_DEBUG_VIRTUAL

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>