[RFC PATCH] Don't reset timestamps in include/generated if not needed

[RFC PATCH] Don't reset timestamps in include/generated if not needed

[Valdis Kletnieks]

Kbuild regenerates bounds.h and asm-offsets.h, resetting the timestamps
and forcing rebuilds even if the contents haven't changed.  Add a bit of
shell magic to only replace the file if the contents have in fact changed,
which should speed up git bisects and similar.

Signed-off-by: Valdis Kletnieks <valdis.kletnieks@vt.edu>

---
So I was doing a git bisect, and getting towards the end, and wondering why
it's taking a long time for each build, and I notice that netfilter modules
are being rebuilt, even though 'git bisect visualize' doesn't show anything
that should have touched netfilter.  Turns out the offender is bounds.h

Fully 3/4 of my modules have a dependency on bounds.h - and we touch it
all the time, even if the contents haven't changed.

RFC because I can't wrap my head around why this wasn't done ages ago.
If I'm missing something something obvious, please apply a cluestick. :)

Lightly tested - if I rm one of those two files, it gets rebuilt. If I
insert some whitespace, it gets replaced. If I don't touch it, the datestamp
doesn't change.

--- linux-next/Kbuild.dist	2015-03-03 19:50:45.175673346 -0500
+++ linux-next/Kbuild	2015-03-03 20:03:20.107820199 -0500
@@ -15,7 +15,7 @@
 
 quiet_cmd_offsets = GEN     $@
 define cmd_offsets
-	(set -e; \
+	( (set -e; \
 	 echo "#ifndef $2"; \
 	 echo "#define $2"; \
 	 echo "/*"; \
@@ -26,7 +26,10 @@
 	 echo ""; \
 	 sed -ne $(sed-y) $<; \
 	 echo ""; \
-	 echo "#endif" ) > $@
+	 echo "#endif" ) > $@.tmp; \
+	 if [ ! -f $@ ]; then mv $@.tmp $@; \
+	 elif cmp -s $@ $@.tmp ; then rm $@.tmp; \
+	 else mv -f $@.tmp $@; fi )
 endef
 
 #####

Re: [RFC PATCH] Don't reset timestamps in include/generated if not needed

[Linus Torvalds]

On Tue, Mar 3, 2015 at 5:07 PM, Valdis Kletnieks
<Valdis.Kletnieks@vt.edu> wrote:
>
> Kbuild regenerates bounds.h and asm-offsets.h, resetting the timestamps
> and forcing rebuilds even if the contents haven't changed.  Add a bit of
> shell magic to only replace the file if the contents have in fact changed,
> which should speed up git bisects and similar.
> ...
> RFC because I can't wrap my head around why this wasn't done ages ago.
> If I'm missing something something obvious, please apply a cluestick. :)
>
> Lightly tested - if I rm one of those two files, it gets rebuilt. If I
> insert some whitespace, it gets replaced. If I don't touch it, the datestamp
> doesn't change.

I don't think this is wrong, but I'd really prefer to do the whole
move-if-changed thing as a separate rule, and a separate command.

IOW, could we please have something that generates the "build.h" file
by separately creating a new temporary file, and then having the
traditional kind of move-if-changed definition

   define move-if-changed
        if ! cmp -s $(1) $(2); then mv -f $(1) $(2); else rm -f $(1); fi
   endef

kind of thing that just does

   $(call move-if-changed,build.h.tmp,build.h)

Hmm? That move-if-changed can be useful for other cases too, I've seen
something like it in a lot of makefiles. I'm surprised we don't have
one already.

That said, we already have that "filechk" define that does pretty much
all of this.

So I don't like the exact patch, but I do like the effect it gives. Michal?

                    Linus

Re: [RFC PATCH] Don't reset timestamps in include/generated if not needed

[Michal Marek]

On Sun, Mar 08, 2015 at 04:08:06PM -0700, Linus Torvalds wrote:
> On Tue, Mar 3, 2015 at 5:07 PM, Valdis Kletnieks
> <Valdis.Kletnieks@vt.edu> wrote:
> >
> > Kbuild regenerates bounds.h and asm-offsets.h, resetting the timestamps
> > and forcing rebuilds even if the contents haven't changed.  Add a bit of
> > shell magic to only replace the file if the contents have in fact changed,
> > which should speed up git bisects and similar.
> > ...
> > RFC because I can't wrap my head around why this wasn't done ages ago.
> > If I'm missing something something obvious, please apply a cluestick. :)

We do not regenerate the files *always*, but only if bounds.c /
asm-offsets.c or one of the headers they include change. So the rule is
not any worse than the rule for compiling a regular .c file into an .o
file. You can use ccache to avoid repeated rebuilds.

However, I can see that especially bounds.h ends up being included in
many places, so cutting its dependencies helps in some cases.


> > Lightly tested - if I rm one of those two files, it gets rebuilt. If I
> > insert some whitespace, it gets replaced. If I don't touch it, the datestamp
> > doesn't change.
> 
> I don't think this is wrong, but I'd really prefer to do the whole
> move-if-changed thing as a separate rule, and a separate command.
> 
> IOW, could we please have something that generates the "build.h" file
> by separately creating a new temporary file, and then having the
> traditional kind of move-if-changed definition
> 
>    define move-if-changed
>         if ! cmp -s $(1) $(2); then mv -f $(1) $(2); else rm -f $(1); fi
>    endef

We already have it and it is called "filechk." Valdis, can you check if
the below patch works equally well for you?

diff --git a/Kbuild b/Kbuild
index ab8ded9..47d0630 100644
--- a/Kbuild
+++ b/Kbuild
@@ -13,8 +13,9 @@ define sed-y
 	s:->::; p;}"
 endef
 
-quiet_cmd_offsets = GEN     $@
-define cmd_offsets
+# Use filechk to avoid rebuilds when a header changes, but the resulting file
+# does not
+define filechk_offsets
 	(set -e; \
 	 echo "#ifndef $2"; \
 	 echo "#define $2"; \
@@ -24,9 +25,9 @@ define cmd_offsets
 	 echo " * This file was generated by Kbuild"; \
 	 echo " */"; \
 	 echo ""; \
-	 sed -ne $(sed-y) $<; \
+	 sed -ne $(sed-y); \
 	 echo ""; \
-	 echo "#endif" ) > $@
+	 echo "#endif" )
 endef
 
 #####
@@ -44,7 +45,7 @@ kernel/bounds.s: kernel/bounds.c FORCE
 
 $(obj)/$(bounds-file): kernel/bounds.s Kbuild
 	$(Q)mkdir -p $(dir $@)
-	$(call cmd,offsets,__LINUX_BOUNDS_H__)
+	$(call filechk,offsets,__LINUX_BOUNDS_H__)
 
 #####
 # 2) Generate asm-offsets.h
@@ -63,7 +64,7 @@ arch/$(SRCARCH)/kernel/asm-offsets.s: arch/$(SRCARCH)/kernel/asm-offsets.c \
 	$(call if_changed_dep,cc_s_c)
 
 $(obj)/$(offsets-file): arch/$(SRCARCH)/kernel/asm-offsets.s Kbuild
-	$(call cmd,offsets,__ASM_OFFSETS_H__)
+	$(call filechk,offsets,__ASM_OFFSETS_H__)
 
 #####
 # 3) Check for missing system calls

Re: [RFC PATCH] Don't reset timestamps in include/generated if not needed

[Masahiro Yamada]

Hi.


2015-03-11 19:01 GMT+09:00 Michal Marek <mmarek@suse.cz>:
> On Sun, Mar 08, 2015 at 04:08:06PM -0700, Linus Torvalds wrote:
>> On Tue, Mar 3, 2015 at 5:07 PM, Valdis Kletnieks
>> <Valdis.Kletnieks@vt.edu> wrote:
>> >
>> > Kbuild regenerates bounds.h and asm-offsets.h, resetting the timestamps
>> > and forcing rebuilds even if the contents haven't changed.  Add a bit of
>> > shell magic to only replace the file if the contents have in fact changed,
>> > which should speed up git bisects and similar.
>> > ...
>> > RFC because I can't wrap my head around why this wasn't done ages ago.
>> > If I'm missing something something obvious, please apply a cluestick. :)
>
> We do not regenerate the files *always*, but only if bounds.c /
> asm-offsets.c or one of the headers they include change.  So the rule is
> not any worse than the rule for compiling a regular .c file into an .o
> file. You can use ccache to avoid repeated rebuilds.

Right.

> However, I can see that especially bounds.h ends up being included in
> many places, so cutting its dependencies helps in some cases.

Agreed.

>
>> > Lightly tested - if I rm one of those two files, it gets rebuilt. If I
>> > insert some whitespace, it gets replaced. If I don't touch it, the datestamp
>> > doesn't change.
>>
>> I don't think this is wrong, but I'd really prefer to do the whole
>> move-if-changed thing as a separate rule, and a separate command.
>>
>> IOW, could we please have something that generates the "build.h" file
>> by separately creating a new temporary file, and then having the
>> traditional kind of move-if-changed definition
>>
>>    define move-if-changed
>>         if ! cmp -s $(1) $(2); then mv -f $(1) $(2); else rm -f $(1); fi
>>    endef
>
> We already have it and it is called "filechk." Valdis, can you check if
> the below patch works equally well for you?


This looks almost nice, but a few comments below.



> diff --git a/Kbuild b/Kbuild
> index ab8ded9..47d0630 100644
> --- a/Kbuild
> +++ b/Kbuild
> @@ -13,8 +13,9 @@ define sed-y
>         s:->::; p;}"
>  endef
>
> -quiet_cmd_offsets = GEN     $@
> -define cmd_offsets
> +# Use filechk to avoid rebuilds when a header changes, but the resulting file
> +# does not
> +define filechk_offsets
>         (set -e; \
>          echo "#ifndef $2"; \
>          echo "#define $2"; \
> @@ -24,9 +25,9 @@ define cmd_offsets
>          echo " * This file was generated by Kbuild"; \
>          echo " */"; \
>          echo ""; \
> -        sed -ne $(sed-y) $<; \
> +        sed -ne $(sed-y); \
>          echo ""; \
> -        echo "#endif" ) > $@
> +        echo "#endif" )
>  endef
>
>  #####
> @@ -44,7 +45,7 @@ kernel/bounds.s: kernel/bounds.c FORCE
>
>  $(obj)/$(bounds-file): kernel/bounds.s Kbuild

You are checking the resulting file content,
so the dependency on "Kbuild" is not necessary.

Instead, you need to add "FORCE" so that this rule is always invoked.



>         $(Q)mkdir -p $(dir $@)

You can drop this line because filechk automatically creates the
output directory.



> -       $(call cmd,offsets,__LINUX_BOUNDS_H__)
> +       $(call filechk,offsets,__LINUX_BOUNDS_H__)




To sum up, I want this part to look like this:

$(obj)/$(offsets-file): arch/$(SRCARCH)/kernel/asm-offsets.s FORCE
        $(call cmd,filechk,__ASM_OFFSETS_H__)





>  #####
>  # 2) Generate asm-offsets.h
> @@ -63,7 +64,7 @@ arch/$(SRCARCH)/kernel/asm-offsets.s: arch/$(SRCARCH)/kernel/asm-offsets.c \
>         $(call if_changed_dep,cc_s_c)
>
>  $(obj)/$(offsets-file): arch/$(SRCARCH)/kernel/asm-offsets.s Kbuild
> -       $(call cmd,offsets,__ASM_OFFSETS_H__)
> +       $(call filechk,offsets,__ASM_OFFSETS_H__)


Ditto.




-- 
Best Regards
Masahiro Yamada

Re: [RFC PATCH] Don't reset timestamps in include/generated if not needed

[Michal Marek]

Dne 13.3.2015 v 05:59 Masahiro Yamada napsal(a):
> 2015-03-11 19:01 GMT+09:00 Michal Marek <mmarek@suse.cz>:
>> We already have it and it is called "filechk." Valdis, can you check if
>> the below patch works equally well for you?
> 
> This looks almost nice, but a few comments below.

Thanks for the review!


>>  $(obj)/$(bounds-file): kernel/bounds.s Kbuild
> 
> You are checking the resulting file content,
> so the dependency on "Kbuild" is not necessary.
> 
> Instead, you need to add "FORCE" so that this rule is always invoked.

Good point.


>>         $(Q)mkdir -p $(dir $@)
> 
> You can drop this line because filechk automatically creates the
> output directory.

Likewise.

Michal

[PATCH] kbuild: Don't reset timestamps in include/generated if not needed

[Michal Marek]

Use filechk to generate asm-offsets.h and bounds.h.

Reported-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
Reviewed-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Signed-off-by: Michal Marek <mmarek@suse.cz>
---
 Kbuild | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/Kbuild b/Kbuild
index ab8ded9..96d0629 100644
--- a/Kbuild
+++ b/Kbuild
@@ -13,8 +13,9 @@ define sed-y
 	s:->::; p;}"
 endef
 
-quiet_cmd_offsets = GEN     $@
-define cmd_offsets
+# Use filechk to avoid rebuilds when a header changes, but the resulting file
+# does not
+define filechk_offsets
 	(set -e; \
 	 echo "#ifndef $2"; \
 	 echo "#define $2"; \
@@ -24,9 +25,9 @@ define cmd_offsets
 	 echo " * This file was generated by Kbuild"; \
 	 echo " */"; \
 	 echo ""; \
-	 sed -ne $(sed-y) $<; \
+	 sed -ne $(sed-y); \
 	 echo ""; \
-	 echo "#endif" ) > $@
+	 echo "#endif" )
 endef
 
 #####
@@ -42,9 +43,8 @@ kernel/bounds.s: kernel/bounds.c FORCE
 	$(Q)mkdir -p $(dir $@)
 	$(call if_changed_dep,cc_s_c)
 
-$(obj)/$(bounds-file): kernel/bounds.s Kbuild
-	$(Q)mkdir -p $(dir $@)
-	$(call cmd,offsets,__LINUX_BOUNDS_H__)
+$(obj)/$(bounds-file): kernel/bounds.s FORCE
+	$(call filechk,offsets,__LINUX_BOUNDS_H__)
 
 #####
 # 2) Generate asm-offsets.h
@@ -62,8 +62,8 @@ arch/$(SRCARCH)/kernel/asm-offsets.s: arch/$(SRCARCH)/kernel/asm-offsets.c \
 	$(Q)mkdir -p $(dir $@)
 	$(call if_changed_dep,cc_s_c)
 
-$(obj)/$(offsets-file): arch/$(SRCARCH)/kernel/asm-offsets.s Kbuild
-	$(call cmd,offsets,__ASM_OFFSETS_H__)
+$(obj)/$(offsets-file): arch/$(SRCARCH)/kernel/asm-offsets.s FORCE
+	$(call filechk,offsets,__ASM_OFFSETS_H__)
 
 #####
 # 3) Check for missing system calls
-- 
2.1.4

Re: [PATCH] kbuild: Don't reset timestamps in include/generated if not needed

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 458 bytes --]

On Tue, 24 Mar 2015 16:36:04 +0100, Michal Marek said:
> Use filechk to generate asm-offsets.h and bounds.h.
>
> Reported-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
> Reviewed-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Signed-off-by: Michal Marek <mmarek@suse.cz>
> ---
>  Kbuild | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)

Different from my patch, but that's OK.  Feel free to stick an Acked-By:
on there. :)


[-- Attachment #2: Type: application/pgp-signature, Size: 848 bytes --]