* [RFC PATCH] Don't reset timestamps in include/generated if not needed
@ 2015-03-04 1:07 Valdis Kletnieks
2015-03-08 23:08 ` Linus Torvalds
0 siblings, 1 reply; 7+ messages in thread
From: Valdis Kletnieks @ 2015-03-04 1:07 UTC (permalink / raw)
To: akpm, linus; +Cc: linux-kernel
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
#####
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] Don't reset timestamps in include/generated if not needed
2015-03-04 1:07 [RFC PATCH] Don't reset timestamps in include/generated if not needed Valdis Kletnieks
@ 2015-03-08 23:08 ` Linus Torvalds
2015-03-11 10:01 ` Michal Marek
0 siblings, 1 reply; 7+ messages in thread
From: Linus Torvalds @ 2015-03-08 23:08 UTC (permalink / raw)
To: Valdis Kletnieks, Michal Marek
Cc: Andrew Morton, Linux Kernel Mailing List, Linux Kbuild mailing list
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] Don't reset timestamps in include/generated if not needed
2015-03-08 23:08 ` Linus Torvalds
@ 2015-03-11 10:01 ` Michal Marek
2015-03-13 4:59 ` Masahiro Yamada
0 siblings, 1 reply; 7+ messages in thread
From: Michal Marek @ 2015-03-11 10:01 UTC (permalink / raw)
To: Linus Torvalds
Cc: Valdis Kletnieks, Andrew Morton, Linux Kernel Mailing List,
Linux Kbuild mailing list
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] Don't reset timestamps in include/generated if not needed
2015-03-11 10:01 ` Michal Marek
@ 2015-03-13 4:59 ` Masahiro Yamada
2015-03-13 10:00 ` Michal Marek
0 siblings, 1 reply; 7+ messages in thread
From: Masahiro Yamada @ 2015-03-13 4:59 UTC (permalink / raw)
To: Michal Marek
Cc: Linus Torvalds, Valdis Kletnieks, Andrew Morton,
Linux Kernel Mailing List, Linux Kbuild mailing list
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] Don't reset timestamps in include/generated if not needed
2015-03-13 4:59 ` Masahiro Yamada
@ 2015-03-13 10:00 ` Michal Marek
2015-03-24 15:36 ` [PATCH] kbuild: " Michal Marek
0 siblings, 1 reply; 7+ messages in thread
From: Michal Marek @ 2015-03-13 10:00 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Linus Torvalds, Valdis Kletnieks, Andrew Morton,
Linux Kernel Mailing List, Linux Kbuild mailing list
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] kbuild: Don't reset timestamps in include/generated if not needed
2015-03-13 10:00 ` Michal Marek
@ 2015-03-24 15:36 ` Michal Marek
2015-03-24 16:08 ` Valdis.Kletnieks
0 siblings, 1 reply; 7+ messages in thread
From: Michal Marek @ 2015-03-24 15:36 UTC (permalink / raw)
To: linux-kbuild; +Cc: Valdis Kletnieks, Masahiro Yamada
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] kbuild: Don't reset timestamps in include/generated if not needed
2015-03-24 15:36 ` [PATCH] kbuild: " Michal Marek
@ 2015-03-24 16:08 ` Valdis.Kletnieks
0 siblings, 0 replies; 7+ messages in thread
From: Valdis.Kletnieks @ 2015-03-24 16:08 UTC (permalink / raw)
To: Michal Marek; +Cc: linux-kbuild, Masahiro Yamada
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-03-24 16:08 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-04 1:07 [RFC PATCH] Don't reset timestamps in include/generated if not needed Valdis Kletnieks
2015-03-08 23:08 ` Linus Torvalds
2015-03-11 10:01 ` Michal Marek
2015-03-13 4:59 ` Masahiro Yamada
2015-03-13 10:00 ` Michal Marek
2015-03-24 15:36 ` [PATCH] kbuild: " Michal Marek
2015-03-24 16:08 ` Valdis.Kletnieks
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.