* [PATCH 0/4] kbuild: optimize output directory creation
@ 2017-11-09 15:41 Masahiro Yamada
2017-11-09 15:41 ` [PATCH 1/4] kbuild: create directory for make cache only when necessary Masahiro Yamada
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: Masahiro Yamada @ 2017-11-09 15:41 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Michal Marek, Sam Ravnborg, Douglas Anderson,
Masahiro Yamada, Michal Marek, x86, H. Peter Anvin,
Thomas Gleixner, Andy Lutomirski, Ingo Molnar
I looked into the build scripts, focusing on "mkdir" optimization.
With this series, I succeeded in speeding up the incremental build
with O= option.
The following is the result of "time make O=foo",
where "foo" is the output directory that has already been built.
Before:
real 0m8.322s
user 0m4.324s
sys 0m1.220s
After:
real 0m6.989s
user 0m4.168s
sys 0m1.080s
3/4 gave the biggest impact.
Masahiro Yamada (4):
kbuild: create directory for make cache only when necessary
kbuild: remove redundant $(wildcard ...) for cmd_files calculation
kbuild: create object directories simpler and faster
kbuild: optimize object directory creation for incremental build
Makefile | 3 +--
arch/x86/entry/vdso/Makefile | 4 ----
scripts/Kbuild.include | 13 +++++++++----
scripts/Makefile.build | 23 ++++++++++++-----------
scripts/Makefile.headersinst | 3 +--
scripts/Makefile.host | 11 -----------
scripts/Makefile.lib | 5 -----
scripts/Makefile.modpost | 3 +--
8 files changed, 24 insertions(+), 41 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/4] kbuild: create directory for make cache only when necessary
2017-11-09 15:41 [PATCH 0/4] kbuild: optimize output directory creation Masahiro Yamada
@ 2017-11-09 15:41 ` Masahiro Yamada
2017-11-09 17:59 ` Doug Anderson
2017-11-09 15:41 ` [PATCH 2/4] kbuild: remove redundant $(wildcard ...) for cmd_files calculation Masahiro Yamada
` (2 subsequent siblings)
3 siblings, 1 reply; 17+ messages in thread
From: Masahiro Yamada @ 2017-11-09 15:41 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Michal Marek, Sam Ravnborg, Douglas Anderson,
Masahiro Yamada
Currently, the existence of $(dir $(make-cache)) is always checked,
and created if it is missing.
We can avoid unnecessary system calls by some tricks.
[1] If KBUILD_SRC is unset, we are building in the source tree.
The output directory checks can be entirely skipped.
[2] If at least one cache data is found, it means the cache file
was included. Obiously its directory exists. Skip "mkdir -p".
[3] If Makefile does not contain any call of __run-and-store, it will
not create a cache file. No need to create its directory.
[4] The "mkdir -p" should be only invoked by the first call of
__run-and-store
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
scripts/Kbuild.include | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index be1c9d6..4fb1be1 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -99,18 +99,19 @@ cc-cross-prefix = \
# Include values from last time
make-cache := $(if $(KBUILD_EXTMOD),$(KBUILD_EXTMOD)/,$(if $(obj),$(obj)/)).cache.mk
-ifeq ($(wildcard $(dir $(make-cache))),)
-$(shell mkdir -p '$(dir $(make-cache))')
-endif
$(make-cache): ;
-include $(make-cache)
+cached-data := $(filter __cached_%, $(.VARIABLES))
+
# If cache exceeds 1000 lines, shrink it down to 500.
-ifneq ($(word 1000,$(filter __cached_%, $(.VARIABLES))),)
+ifneq ($(word 1000,$(cached-data)),)
$(shell tail -n 500 $(make-cache) > $(make-cache).tmp; \
mv $(make-cache).tmp $(make-cache))
endif
+cache-dir := $(if $(KBUILD_SRC),$(if $(cache-data),,$(dir $(make-cache))))
+
# Usage: $(call __sanitize-opt,Hello=Hola$(comma)Goodbye Adios)
#
# Convert all '$', ')', '(', '\', '=', ' ', ',', ':' to '_'
@@ -136,6 +137,10 @@ __sanitize-opt = $(subst $$,_,$(subst $(right_paren),_,$(subst $(left_paren),_,$
define __run-and-store
ifeq ($(origin $(1)),undefined)
$$(eval $(1) := $$(shell $$(2)))
+ifneq ($(cache-dir),)
+ $$(shell mkdir -p $(cache-dir))
+ $$(eval cache-dir :=)
+endif
$$(shell echo '$(1) := $$($(1))' >> $(make-cache))
endif
endef
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/4] kbuild: remove redundant $(wildcard ...) for cmd_files calculation
2017-11-09 15:41 [PATCH 0/4] kbuild: optimize output directory creation Masahiro Yamada
2017-11-09 15:41 ` [PATCH 1/4] kbuild: create directory for make cache only when necessary Masahiro Yamada
@ 2017-11-09 15:41 ` Masahiro Yamada
2017-11-10 4:53 ` Doug Anderson
2017-11-09 15:41 ` [PATCH 3/4] kbuild: create object directories simpler and faster Masahiro Yamada
2017-11-09 15:41 ` [PATCH 4/4] kbuild: optimize object directory creation for incremental build Masahiro Yamada
3 siblings, 1 reply; 17+ messages in thread
From: Masahiro Yamada @ 2017-11-09 15:41 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Michal Marek, Sam Ravnborg, Douglas Anderson,
Masahiro Yamada, Michal Marek
I do not why $(wildcard ...) needs to be called twice for computing
cmd_files. Remove the first one.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Makefile | 3 +--
scripts/Makefile.build | 3 +--
| 3 +--
scripts/Makefile.modpost | 3 +--
4 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/Makefile b/Makefile
index a7476e6..58dd245 100644
--- a/Makefile
+++ b/Makefile
@@ -1693,8 +1693,7 @@ cmd_crmodverdir = $(Q)mkdir -p $(MODVERDIR) \
# read all saved command lines
-targets := $(wildcard $(sort $(targets)))
-cmd_files := $(wildcard .*.cmd $(foreach f,$(targets),$(dir $(f)).$(notdir $(f)).cmd))
+cmd_files := $(wildcard .*.cmd $(foreach f,$(sort $(targets)),$(dir $(f)).$(notdir $(f)).cmd))
ifneq ($(cmd_files),)
$(cmd_files): ; # Do not try to update included dependency files
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 061d0c3..62d5314 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -583,8 +583,7 @@ FORCE:
# optimization, we don't need to read them if the target does not
# exist, we will rebuild anyway in that case.
-targets := $(wildcard $(sort $(targets)))
-cmd_files := $(wildcard $(foreach f,$(targets),$(dir $(f)).$(notdir $(f)).cmd))
+cmd_files := $(wildcard $(foreach f,$(sort $(targets)),$(dir $(f)).$(notdir $(f)).cmd))
ifneq ($(cmd_files),)
include $(cmd_files)
--git a/scripts/Makefile.headersinst b/scripts/Makefile.headersinst
index 5692d7a..2aa9181 100644
--- a/scripts/Makefile.headersinst
+++ b/scripts/Makefile.headersinst
@@ -114,9 +114,8 @@ $(check-file): scripts/headers_check.pl $(output-files) FORCE
endif
-targets := $(wildcard $(sort $(targets)))
cmd_files := $(wildcard \
- $(foreach f,$(targets),$(dir $(f)).$(notdir $(f)).cmd))
+ $(foreach f,$(sort $$(targets)),$(dir $(f)).$(notdir $(f)).cmd))
ifneq ($(cmd_files),)
include $(cmd_files)
diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index 16923ba..cf125c1 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -143,8 +143,7 @@ FORCE:
# optimization, we don't need to read them if the target does not
# exist, we will rebuild anyway in that case.
-targets := $(wildcard $(sort $(targets)))
-cmd_files := $(wildcard $(foreach f,$(targets),$(dir $(f)).$(notdir $(f)).cmd))
+cmd_files := $(wildcard $(foreach f,$(sort $(targets)),$(dir $(f)).$(notdir $(f)).cmd))
ifneq ($(cmd_files),)
include $(cmd_files)
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 3/4] kbuild: create object directories simpler and faster
2017-11-09 15:41 [PATCH 0/4] kbuild: optimize output directory creation Masahiro Yamada
2017-11-09 15:41 ` [PATCH 1/4] kbuild: create directory for make cache only when necessary Masahiro Yamada
2017-11-09 15:41 ` [PATCH 2/4] kbuild: remove redundant $(wildcard ...) for cmd_files calculation Masahiro Yamada
@ 2017-11-09 15:41 ` Masahiro Yamada
2017-11-10 6:26 ` Ingo Molnar
` (2 more replies)
2017-11-09 15:41 ` [PATCH 4/4] kbuild: optimize object directory creation for incremental build Masahiro Yamada
3 siblings, 3 replies; 17+ messages in thread
From: Masahiro Yamada @ 2017-11-09 15:41 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Michal Marek, Sam Ravnborg, Douglas Anderson,
Masahiro Yamada, Michal Marek, x86, H. Peter Anvin,
Thomas Gleixner, Andy Lutomirski, Ingo Molnar
For the out-of-tree build, scripts/Makefile.build creates output
directories, but this operation is not efficient.
scripts/Makefile.lib calculates obj-dirs as follows:
obj-dirs := $(dir $(multi-objs) $(obj-y))
Please notice $(sort ...) is not used here. Usually the resulted
obj-dirs is as many "./" as objects.
For those duplicated paths, the following command is invoked.
_dummy := $(foreach d,$(obj-dirs), $(shell [ -d $(d) ] || mkdir -p $(d)))
Then, the costly shell command is run over and over again.
I see many points for optimization:
[1] Use $(sort ...) to cut down duplicated paths before passing them
to system call
[2] Use single $(shell ...) instead of repeating it with $(foreach ...)
This will reduce forking.
[3] We can calculate obj-dirs more simply. Most of objects are already
accumulated in $(targets). So, $(dir $(targets)) is fine and more
comprehensive.
I also removed bad code in arch/x86/entry/vdso/Makefile. This is now
really unnecessary.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
arch/x86/entry/vdso/Makefile | 4 ----
scripts/Makefile.build | 15 ++++++---------
scripts/Makefile.host | 11 -----------
scripts/Makefile.lib | 5 -----
4 files changed, 6 insertions(+), 29 deletions(-)
diff --git a/arch/x86/entry/vdso/Makefile b/arch/x86/entry/vdso/Makefile
index d540966..f8e3d85 100644
--- a/arch/x86/entry/vdso/Makefile
+++ b/arch/x86/entry/vdso/Makefile
@@ -129,10 +129,6 @@ $(obj)/vdsox32.so.dbg: $(src)/vdsox32.lds $(vobjx32s) FORCE
CPPFLAGS_vdso32.lds = $(CPPFLAGS_vdso.lds)
VDSO_LDFLAGS_vdso32.lds = -m32 -Wl,-m,elf_i386 -Wl,-soname=linux-gate.so.1
-# This makes sure the $(obj) subdirectory exists even though vdso32/
-# is not a kbuild sub-make subdirectory.
-override obj-dirs = $(dir $(obj)) $(obj)/vdso32/
-
targets += vdso32/vdso32.lds
targets += vdso32/note.o vdso32/system_call.o vdso32/sigreturn.o
targets += vdso32/vclock_gettime.o
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 62d5314..89ac180 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -64,15 +64,6 @@ ifneq ($(hostprogs-y)$(hostprogs-m)$(hostlibs-y)$(hostlibs-m)$(hostcxxlibs-y)$(h
include scripts/Makefile.host
endif
-ifneq ($(KBUILD_SRC),)
-# Create output directory if not already present
-_dummy := $(shell [ -d $(obj) ] || mkdir -p $(obj))
-
-# Create directories for object files if directory does not exist
-# Needed when obj-y := dir/file.o syntax is used
-_dummy := $(foreach d,$(obj-dirs), $(shell [ -d $(d) ] || mkdir -p $(d)))
-endif
-
ifndef obj
$(warning kbuild: Makefile.build is included improperly)
endif
@@ -589,6 +580,12 @@ ifneq ($(cmd_files),)
include $(cmd_files)
endif
+ifneq ($(KBUILD_SRC),)
+# Create directories for object files if directory does not exist
+obj-dirs := $(sort $(obj) $(patsubst %/,%, $(dir $(targets))))
+$(shell mkdir -p $(obj-dirs))
+endif
+
# Declare the contents of the .PHONY variable as phony. We keep that
# information in a variable se we can use it in if_changed and friends.
diff --git a/scripts/Makefile.host b/scripts/Makefile.host
index 9cfd5c8..3a5460d 100644
--- a/scripts/Makefile.host
+++ b/scripts/Makefile.host
@@ -48,15 +48,6 @@ host-cxxobjs := $(sort $(foreach m,$(host-cxxmulti),$($(m)-cxxobjs)))
host-cshobjs := $(sort $(foreach m,$(host-cshlib),$($(m:.so=-objs))))
host-cxxshobjs := $(sort $(foreach m,$(host-cxxshlib),$($(m:.so=-objs))))
-# output directory for programs/.o files
-# hostprogs-y := tools/build may have been specified.
-# Retrieve also directory of .o files from prog-objs or prog-cxxobjs notation
-host-objdirs := $(dir $(__hostprogs) $(host-cobjs) $(host-cxxobjs))
-
-host-objdirs := $(strip $(sort $(filter-out ./,$(host-objdirs))))
-
-
-__hostprogs := $(addprefix $(obj)/,$(__hostprogs))
host-csingle := $(addprefix $(obj)/,$(host-csingle))
host-cmulti := $(addprefix $(obj)/,$(host-cmulti))
host-cobjs := $(addprefix $(obj)/,$(host-cobjs))
@@ -68,8 +59,6 @@ host-cshobjs := $(addprefix $(obj)/,$(host-cshobjs))
host-cxxshobjs := $(addprefix $(obj)/,$(host-cxxshobjs))
host-objdirs := $(addprefix $(obj)/,$(host-objdirs))
-obj-dirs += $(host-objdirs)
-
#####
# Handle options to gcc. Support building with separate output directory
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 4d88ad7..5fbc46d 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -50,15 +50,11 @@ single-used-m := $(sort $(filter-out $(multi-used-m),$(obj-m)))
# objects depend on those (obviously)
multi-objs-y := $(foreach m, $(multi-used-y), $($(m:.o=-objs)) $($(m:.o=-y)))
multi-objs-m := $(foreach m, $(multi-used-m), $($(m:.o=-objs)) $($(m:.o=-y)))
-multi-objs := $(multi-objs-y) $(multi-objs-m)
# $(subdir-obj-y) is the list of objects in $(obj-y) which uses dir/ to
# tell kbuild to descend
subdir-obj-y := $(filter %/built-in.o, $(obj-y))
-# $(obj-dirs) is a list of directories that contain object files
-obj-dirs := $(dir $(multi-objs) $(obj-y))
-
# Replace multi-part objects by their individual parts, look at local dir only
real-objs-y := $(foreach m, $(filter-out $(subdir-obj-y), $(obj-y)), $(if $(strip $($(m:.o=-objs)) $($(m:.o=-y))),$($(m:.o=-objs)) $($(m:.o=-y)),$(m))) $(extra-y)
real-objs-m := $(foreach m, $(obj-m), $(if $(strip $($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-m))),$($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-m)),$(m)))
@@ -81,7 +77,6 @@ multi-used-m := $(addprefix $(obj)/,$(multi-used-m))
multi-objs-y := $(addprefix $(obj)/,$(multi-objs-y))
multi-objs-m := $(addprefix $(obj)/,$(multi-objs-m))
subdir-ym := $(addprefix $(obj)/,$(subdir-ym))
-obj-dirs := $(addprefix $(obj)/,$(obj-dirs))
# These flags are needed for modversions and compiling, so we define them here
# $(modname_flags) defines KBUILD_MODNAME as the name of the module it will
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4/4] kbuild: optimize object directory creation for incremental build
2017-11-09 15:41 [PATCH 0/4] kbuild: optimize output directory creation Masahiro Yamada
` (2 preceding siblings ...)
2017-11-09 15:41 ` [PATCH 3/4] kbuild: create object directories simpler and faster Masahiro Yamada
@ 2017-11-09 15:41 ` Masahiro Yamada
2017-11-10 10:58 ` Cao jin
3 siblings, 1 reply; 17+ messages in thread
From: Masahiro Yamada @ 2017-11-09 15:41 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Michal Marek, Sam Ravnborg, Douglas Anderson,
Masahiro Yamada, Michal Marek
The previous commit largely optimized the object directory creation.
We can optimize it more for incremental build.
There are already *.cmd files in the output directory. The existing
*.cmd files have been picked up by $(wildcard ...). Obviously,
directories containing them exist too, so we can skip "mkdir -p".
With this, Kbuild runs almost zero "mkdir -p" in incremental building.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
scripts/Makefile.build | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 89ac180..90ea7a5 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -583,8 +583,13 @@ endif
ifneq ($(KBUILD_SRC),)
# Create directories for object files if directory does not exist
obj-dirs := $(sort $(obj) $(patsubst %/,%, $(dir $(targets))))
+# If cmd_files exist, their directories apparently exist. Skip mkdir.
+exist-dirs := $(sort $(patsubst %/,%, $(dir $(cmd_files))))
+obj-dirs := $(strip $(filter-out . $(exist-dirs), $(obj-dirs)))
+ifneq ($(obj-dirs),)
$(shell mkdir -p $(obj-dirs))
endif
+endif
# Declare the contents of the .PHONY variable as phony. We keep that
# information in a variable se we can use it in if_changed and friends.
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/4] kbuild: create directory for make cache only when necessary
2017-11-09 15:41 ` [PATCH 1/4] kbuild: create directory for make cache only when necessary Masahiro Yamada
@ 2017-11-09 17:59 ` Doug Anderson
2017-11-10 4:12 ` Masahiro Yamada
0 siblings, 1 reply; 17+ messages in thread
From: Doug Anderson @ 2017-11-09 17:59 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Linux Kbuild mailing list, LKML, Michal Marek, Sam Ravnborg
Hi,
On Thu, Nov 9, 2017 at 7:41 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> Currently, the existence of $(dir $(make-cache)) is always checked,
> and created if it is missing.
>
> We can avoid unnecessary system calls by some tricks.
>
> [1] If KBUILD_SRC is unset, we are building in the source tree.
> The output directory checks can be entirely skipped.
> [2] If at least one cache data is found, it means the cache file
> was included. Obiously its directory exists. Skip "mkdir -p".
> [3] If Makefile does not contain any call of __run-and-store, it will
> not create a cache file. No need to create its directory.
> [4] The "mkdir -p" should be only invoked by the first call of
> __run-and-store
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> scripts/Kbuild.include | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
> index be1c9d6..4fb1be1 100644
> --- a/scripts/Kbuild.include
> +++ b/scripts/Kbuild.include
> @@ -99,18 +99,19 @@ cc-cross-prefix = \
>
> # Include values from last time
> make-cache := $(if $(KBUILD_EXTMOD),$(KBUILD_EXTMOD)/,$(if $(obj),$(obj)/)).cache.mk
> -ifeq ($(wildcard $(dir $(make-cache))),)
> -$(shell mkdir -p '$(dir $(make-cache))')
> -endif
> $(make-cache): ;
> -include $(make-cache)
>
> +cached-data := $(filter __cached_%, $(.VARIABLES))
> +
> # If cache exceeds 1000 lines, shrink it down to 500.
> -ifneq ($(word 1000,$(filter __cached_%, $(.VARIABLES))),)
> +ifneq ($(word 1000,$(cached-data)),)
> $(shell tail -n 500 $(make-cache) > $(make-cache).tmp; \
> mv $(make-cache).tmp $(make-cache))
> endif
>
> +cache-dir := $(if $(KBUILD_SRC),$(if $(cache-data),,$(dir $(make-cache))))
It wouldn't hurt to add a comment that cache-dir will be blank if we
don't need to make the cache dir and will contain a directory path
only if the dir doesn't exist. Without a comment it could take
someone quite a while to realize that...
> +
> # Usage: $(call __sanitize-opt,Hello=Hola$(comma)Goodbye Adios)
> #
> # Convert all '$', ')', '(', '\', '=', ' ', ',', ':' to '_'
> @@ -136,6 +137,10 @@ __sanitize-opt = $(subst $$,_,$(subst $(right_paren),_,$(subst $(left_paren),_,$
> define __run-and-store
> ifeq ($(origin $(1)),undefined)
> $$(eval $(1) := $$(shell $$(2)))
> +ifneq ($(cache-dir),)
> + $$(shell mkdir -p $(cache-dir))
I _think_ you want some single quotes in there. AKA:
$$(shell mkdir -p '$(cache-dir)')
That at least matches what the "old" code used to do. Specifically if
'cache-dir' happens to have a space in it then it won't work right
without the single quotes. There may be other symbols that your shell
might interpret in interesting ways, too.
NOTE: I have no idea if the kernel Makefiles work if paths like
KBUILD_SRC have spaces in them to begin with, but it seems wise to add
the quotes here anyway.
ALSO NOTE: I think you could still confuse the kernel Makefiles if
somehow you had a single quote in your path somehow. I assume we
don't care?
> + $$(eval cache-dir :=)
> +endif
> $$(shell echo '$(1) := $$($(1))' >> $(make-cache))
> endif
> endef
Other than the single quote problem and the suggested comment, this
seems like a sane optimization to me. Feel free to add my Reviewed-by
once those fixes are in place.
-Doug
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/4] kbuild: create directory for make cache only when necessary
2017-11-09 17:59 ` Doug Anderson
@ 2017-11-10 4:12 ` Masahiro Yamada
2017-11-10 17:34 ` Doug Anderson
0 siblings, 1 reply; 17+ messages in thread
From: Masahiro Yamada @ 2017-11-10 4:12 UTC (permalink / raw)
To: Doug Anderson; +Cc: Linux Kbuild mailing list, LKML, Michal Marek, Sam Ravnborg
Hi Douglas,
Thanks for your review.
2017-11-10 2:59 GMT+09:00 Doug Anderson <dianders@chromium.org>:
> Hi,
>
> On Thu, Nov 9, 2017 at 7:41 AM, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>> Currently, the existence of $(dir $(make-cache)) is always checked,
>> and created if it is missing.
>>
>> We can avoid unnecessary system calls by some tricks.
>>
>> [1] If KBUILD_SRC is unset, we are building in the source tree.
>> The output directory checks can be entirely skipped.
>> [2] If at least one cache data is found, it means the cache file
>> was included. Obiously its directory exists. Skip "mkdir -p".
>> [3] If Makefile does not contain any call of __run-and-store, it will
>> not create a cache file. No need to create its directory.
>> [4] The "mkdir -p" should be only invoked by the first call of
>> __run-and-store
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>> scripts/Kbuild.include | 13 +++++++++----
>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
>> index be1c9d6..4fb1be1 100644
>> --- a/scripts/Kbuild.include
>> +++ b/scripts/Kbuild.include
>> @@ -99,18 +99,19 @@ cc-cross-prefix = \
>>
>> # Include values from last time
>> make-cache := $(if $(KBUILD_EXTMOD),$(KBUILD_EXTMOD)/,$(if $(obj),$(obj)/)).cache.mk
>> -ifeq ($(wildcard $(dir $(make-cache))),)
>> -$(shell mkdir -p '$(dir $(make-cache))')
>> -endif
>> $(make-cache): ;
>> -include $(make-cache)
>>
>> +cached-data := $(filter __cached_%, $(.VARIABLES))
>> +
>> # If cache exceeds 1000 lines, shrink it down to 500.
>> -ifneq ($(word 1000,$(filter __cached_%, $(.VARIABLES))),)
>> +ifneq ($(word 1000,$(cached-data)),)
>> $(shell tail -n 500 $(make-cache) > $(make-cache).tmp; \
>> mv $(make-cache).tmp $(make-cache))
>> endif
>>
>> +cache-dir := $(if $(KBUILD_SRC),$(if $(cache-data),,$(dir $(make-cache))))
>
> It wouldn't hurt to add a comment that cache-dir will be blank if we
> don't need to make the cache dir and will contain a directory path
> only if the dir doesn't exist. Without a comment it could take
> someone quite a while to realize that...
You are right. This is confusing.
Another idea is use a boolean flag.
For example, like follows:
create-cache-dir := $(if $(KBUILD_SRC),$(if $(cache-data),,1)))
define __run-and-store
ifeq ($(origin $(1)),undefined)
$$(eval $(1) := $$(shell $$(2)))
ifeq ($(create-cache-dir),1)
$$(shell mkdir -p $(dir $(make-cache)))
$$(eval create-cache-dir :=)
endif
$$(shell echo '$(1) := $$($(1))' >> $(make-cache))
endif
endef
Perhaps, this is clearer and self-documenting.
>> +
>> # Usage: $(call __sanitize-opt,Hello=Hola$(comma)Goodbye Adios)
>> #
>> # Convert all '$', ')', '(', '\', '=', ' ', ',', ':' to '_'
>> @@ -136,6 +137,10 @@ __sanitize-opt = $(subst $$,_,$(subst $(right_paren),_,$(subst $(left_paren),_,$
>> define __run-and-store
>> ifeq ($(origin $(1)),undefined)
>> $$(eval $(1) := $$(shell $$(2)))
>> +ifneq ($(cache-dir),)
>> + $$(shell mkdir -p $(cache-dir))
>
> I _think_ you want some single quotes in there. AKA:
>
> $$(shell mkdir -p '$(cache-dir)')
>
> That at least matches what the "old" code used to do. Specifically if
> 'cache-dir' happens to have a space in it then it won't work right
> without the single quotes. There may be other symbols that your shell
> might interpret in interesting ways, too.
Kbuild always runs in the output directory.
So, 'cache-dir' is always a relative path from the top of kernel directory
whether O= option is given or not.
For kernel source, I do not see any file path containing spaces.
Just in case, I renamed a directory and tested, but
something strange happened in silentoldconfig, it would not work.
Insane people may want to use a file path with spaces
for external modules.
I tested,
obj-m := fo o/
but, this would not work either.
It will be difficult to make it work
because $(sort ...) is used in several places
in core makefiles.
So, my conclusion is, it does not work.
> NOTE: I have no idea if the kernel Makefiles work if paths like
> KBUILD_SRC have spaces in them to begin with, but it seems wise to add
> the quotes here anyway.
I have not tested this case.
Probably, this will be less difficult
if we want to allow spaces in KBUILD_SRC.
> ALSO NOTE: I think you could still confuse the kernel Makefiles if
> somehow you had a single quote in your path somehow. I assume we
> don't care?
Hmm, I do not think this is worth efforts.
Probably, the most reasonable solution is
please do not use special characters in file paths.
>
>> + $$(eval cache-dir :=)
>> +endif
>> $$(shell echo '$(1) := $$($(1))' >> $(make-cache))
>> endif
>> endef
>
> Other than the single quote problem and the suggested comment, this
> seems like a sane optimization to me. Feel free to add my Reviewed-by
> once those fixes are in place.
>
> -Doug
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/4] kbuild: remove redundant $(wildcard ...) for cmd_files calculation
2017-11-09 15:41 ` [PATCH 2/4] kbuild: remove redundant $(wildcard ...) for cmd_files calculation Masahiro Yamada
@ 2017-11-10 4:53 ` Doug Anderson
2017-11-10 4:55 ` Masahiro Yamada
0 siblings, 1 reply; 17+ messages in thread
From: Doug Anderson @ 2017-11-10 4:53 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Linux Kbuild mailing list, LKML, Michal Marek, Sam Ravnborg,
Michal Marek
Hi,
On Thu, Nov 9, 2017 at 7:41 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> I do not why $(wildcard ...) needs to be called twice for computing
> cmd_files. Remove the first one.
I tried and I can't find any reason for the two calls $(wildcard ...)
either, so this seems fine to me.
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> Makefile | 3 +--
> scripts/Makefile.build | 3 +--
> scripts/Makefile.headersinst | 3 +--
> scripts/Makefile.modpost | 3 +--
> 4 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index a7476e6..58dd245 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1693,8 +1693,7 @@ cmd_crmodverdir = $(Q)mkdir -p $(MODVERDIR) \
>
> # read all saved command lines
>
> -targets := $(wildcard $(sort $(targets)))
> -cmd_files := $(wildcard .*.cmd $(foreach f,$(targets),$(dir $(f)).$(notdir $(f)).cmd))
> +cmd_files := $(wildcard .*.cmd $(foreach f,$(sort $(targets)),$(dir $(f)).$(notdir $(f)).cmd))
>
> ifneq ($(cmd_files),)
> $(cmd_files): ; # Do not try to update included dependency files
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 061d0c3..62d5314 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -583,8 +583,7 @@ FORCE:
> # optimization, we don't need to read them if the target does not
> # exist, we will rebuild anyway in that case.
>
> -targets := $(wildcard $(sort $(targets)))
> -cmd_files := $(wildcard $(foreach f,$(targets),$(dir $(f)).$(notdir $(f)).cmd))
> +cmd_files := $(wildcard $(foreach f,$(sort $(targets)),$(dir $(f)).$(notdir $(f)).cmd))
>
> ifneq ($(cmd_files),)
> include $(cmd_files)
> diff --git a/scripts/Makefile.headersinst b/scripts/Makefile.headersinst
> index 5692d7a..2aa9181 100644
> --- a/scripts/Makefile.headersinst
> +++ b/scripts/Makefile.headersinst
> @@ -114,9 +114,8 @@ $(check-file): scripts/headers_check.pl $(output-files) FORCE
>
> endif
>
> -targets := $(wildcard $(sort $(targets)))
> cmd_files := $(wildcard \
> - $(foreach f,$(targets),$(dir $(f)).$(notdir $(f)).cmd))
> + $(foreach f,$(sort $$(targets)),$(dir $(f)).$(notdir $(f)).cmd))
Did you mean the "$$" here before (targets)? At first glance it seems wrong...
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/4] kbuild: remove redundant $(wildcard ...) for cmd_files calculation
2017-11-10 4:53 ` Doug Anderson
@ 2017-11-10 4:55 ` Masahiro Yamada
0 siblings, 0 replies; 17+ messages in thread
From: Masahiro Yamada @ 2017-11-10 4:55 UTC (permalink / raw)
To: Doug Anderson
Cc: Linux Kbuild mailing list, LKML, Michal Marek, Sam Ravnborg,
Michal Marek
2017-11-10 13:53 GMT+09:00 Doug Anderson <dianders@chromium.org>:
> Hi,
>
> On Thu, Nov 9, 2017 at 7:41 AM, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>> I do not why $(wildcard ...) needs to be called twice for computing
>> cmd_files. Remove the first one.
>
> I tried and I can't find any reason for the two calls $(wildcard ...)
> either, so this seems fine to me.
>
>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>> Makefile | 3 +--
>> scripts/Makefile.build | 3 +--
>> scripts/Makefile.headersinst | 3 +--
>> scripts/Makefile.modpost | 3 +--
>> 4 files changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index a7476e6..58dd245 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1693,8 +1693,7 @@ cmd_crmodverdir = $(Q)mkdir -p $(MODVERDIR) \
>>
>> # read all saved command lines
>>
>> -targets := $(wildcard $(sort $(targets)))
>> -cmd_files := $(wildcard .*.cmd $(foreach f,$(targets),$(dir $(f)).$(notdir $(f)).cmd))
>> +cmd_files := $(wildcard .*.cmd $(foreach f,$(sort $(targets)),$(dir $(f)).$(notdir $(f)).cmd))
>>
>> ifneq ($(cmd_files),)
>> $(cmd_files): ; # Do not try to update included dependency files
>> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
>> index 061d0c3..62d5314 100644
>> --- a/scripts/Makefile.build
>> +++ b/scripts/Makefile.build
>> @@ -583,8 +583,7 @@ FORCE:
>> # optimization, we don't need to read them if the target does not
>> # exist, we will rebuild anyway in that case.
>>
>> -targets := $(wildcard $(sort $(targets)))
>> -cmd_files := $(wildcard $(foreach f,$(targets),$(dir $(f)).$(notdir $(f)).cmd))
>> +cmd_files := $(wildcard $(foreach f,$(sort $(targets)),$(dir $(f)).$(notdir $(f)).cmd))
>>
>> ifneq ($(cmd_files),)
>> include $(cmd_files)
>> diff --git a/scripts/Makefile.headersinst b/scripts/Makefile.headersinst
>> index 5692d7a..2aa9181 100644
>> --- a/scripts/Makefile.headersinst
>> +++ b/scripts/Makefile.headersinst
>> @@ -114,9 +114,8 @@ $(check-file): scripts/headers_check.pl $(output-files) FORCE
>>
>> endif
>>
>> -targets := $(wildcard $(sort $(targets)))
>> cmd_files := $(wildcard \
>> - $(foreach f,$(targets),$(dir $(f)).$(notdir $(f)).cmd))
>> + $(foreach f,$(sort $$(targets)),$(dir $(f)).$(notdir $(f)).cmd))
>
> Did you mean the "$$" here before (targets)? At first glance it seems wrong...
Good catch!
I will fix this.
Thanks!
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/4] kbuild: create object directories simpler and faster
2017-11-09 15:41 ` [PATCH 3/4] kbuild: create object directories simpler and faster Masahiro Yamada
@ 2017-11-10 6:26 ` Ingo Molnar
2017-11-10 8:45 ` Cao jin
2017-11-10 18:46 ` Doug Anderson
2 siblings, 0 replies; 17+ messages in thread
From: Ingo Molnar @ 2017-11-10 6:26 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, linux-kernel, Michal Marek, Sam Ravnborg,
Douglas Anderson, Michal Marek, x86, H. Peter Anvin,
Thomas Gleixner, Andy Lutomirski, Ingo Molnar
* Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> For the out-of-tree build, scripts/Makefile.build creates output
> directories, but this operation is not efficient.
>
> scripts/Makefile.lib calculates obj-dirs as follows:
>
> obj-dirs := $(dir $(multi-objs) $(obj-y))
>
> Please notice $(sort ...) is not used here. Usually the resulted
> obj-dirs is as many "./" as objects.
>
> For those duplicated paths, the following command is invoked.
>
> _dummy := $(foreach d,$(obj-dirs), $(shell [ -d $(d) ] || mkdir -p $(d)))
>
> Then, the costly shell command is run over and over again.
>
> I see many points for optimization:
>
> [1] Use $(sort ...) to cut down duplicated paths before passing them
> to system call
> [2] Use single $(shell ...) instead of repeating it with $(foreach ...)
> This will reduce forking.
> [3] We can calculate obj-dirs more simply. Most of objects are already
> accumulated in $(targets). So, $(dir $(targets)) is fine and more
> comprehensive.
>
> I also removed bad code in arch/x86/entry/vdso/Makefile. This is now
> really unnecessary.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> arch/x86/entry/vdso/Makefile | 4 ----
> scripts/Makefile.build | 15 ++++++---------
> scripts/Makefile.host | 11 -----------
> scripts/Makefile.lib | 5 -----
> 4 files changed, 6 insertions(+), 29 deletions(-)
I love not just the speedup, but the diffstat as well ;-)
Acked-by: Ingo Molnar <mingo@kernel.org>
Thanks,
Ingo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/4] kbuild: create object directories simpler and faster
2017-11-09 15:41 ` [PATCH 3/4] kbuild: create object directories simpler and faster Masahiro Yamada
2017-11-10 6:26 ` Ingo Molnar
@ 2017-11-10 8:45 ` Cao jin
2017-11-10 9:21 ` Masahiro Yamada
2017-11-10 18:46 ` Doug Anderson
2 siblings, 1 reply; 17+ messages in thread
From: Cao jin @ 2017-11-10 8:45 UTC (permalink / raw)
To: Masahiro Yamada, linux-kbuild
Cc: linux-kernel, Michal Marek, Sam Ravnborg, Douglas Anderson,
Michal Marek, x86, H. Peter Anvin, Thomas Gleixner,
Andy Lutomirski, Ingo Molnar
Hi Masahiro-san,
On 11/09/2017 11:41 PM, Masahiro Yamada wrote:
> For the out-of-tree build, scripts/Makefile.build creates output
> directories, but this operation is not efficient.
>
> scripts/Makefile.lib calculates obj-dirs as follows:
>
> obj-dirs := $(dir $(multi-objs) $(obj-y))
>
> Please notice $(sort ...) is not used here. Usually the resulted
> obj-dirs is as many "./" as objects.
>
> For those duplicated paths, the following command is invoked.
>
> _dummy := $(foreach d,$(obj-dirs), $(shell [ -d $(d) ] || mkdir -p $(d)))
>
> Then, the costly shell command is run over and over again.
>
> I see many points for optimization:
>
> [1] Use $(sort ...) to cut down duplicated paths before passing them
> to system call
> [2] Use single $(shell ...) instead of repeating it with $(foreach ...)
> This will reduce forking.
> [3] We can calculate obj-dirs more simply. Most of objects are already
> accumulated in $(targets). So, $(dir $(targets)) is fine and more
> comprehensive.
>
> I also removed bad code in arch/x86/entry/vdso/Makefile. This is now
> really unnecessary.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> arch/x86/entry/vdso/Makefile | 4 ----
> scripts/Makefile.build | 15 ++++++---------
> scripts/Makefile.host | 11 -----------
> scripts/Makefile.lib | 5 -----
> 4 files changed, 6 insertions(+), 29 deletions(-)
>
> diff --git a/arch/x86/entry/vdso/Makefile b/arch/x86/entry/vdso/Makefile
> index d540966..f8e3d85 100644
> --- a/arch/x86/entry/vdso/Makefile
> +++ b/arch/x86/entry/vdso/Makefile
> @@ -129,10 +129,6 @@ $(obj)/vdsox32.so.dbg: $(src)/vdsox32.lds $(vobjx32s) FORCE
> CPPFLAGS_vdso32.lds = $(CPPFLAGS_vdso.lds)
> VDSO_LDFLAGS_vdso32.lds = -m32 -Wl,-m,elf_i386 -Wl,-soname=linux-gate.so.1
>
> -# This makes sure the $(obj) subdirectory exists even though vdso32/
> -# is not a kbuild sub-make subdirectory.
> -override obj-dirs = $(dir $(obj)) $(obj)/vdso32/
> -
> targets += vdso32/vdso32.lds
> targets += vdso32/note.o vdso32/system_call.o vdso32/sigreturn.o
> targets += vdso32/vclock_gettime.o
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 62d5314..89ac180 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -64,15 +64,6 @@ ifneq ($(hostprogs-y)$(hostprogs-m)$(hostlibs-y)$(hostlibs-m)$(hostcxxlibs-y)$(h
> include scripts/Makefile.host
> endif
>
> -ifneq ($(KBUILD_SRC),)
> -# Create output directory if not already present
> -_dummy := $(shell [ -d $(obj) ] || mkdir -p $(obj))
> -
> -# Create directories for object files if directory does not exist
> -# Needed when obj-y := dir/file.o syntax is used
> -_dummy := $(foreach d,$(obj-dirs), $(shell [ -d $(d) ] || mkdir -p $(d)))
> -endif
> -
> ifndef obj
> $(warning kbuild: Makefile.build is included improperly)
> endif
> @@ -589,6 +580,12 @@ ifneq ($(cmd_files),)
> include $(cmd_files)
> endif
>
> +ifneq ($(KBUILD_SRC),)
> +# Create directories for object files if directory does not exist
> +obj-dirs := $(sort $(obj) $(patsubst %/,%, $(dir $(targets))))
> +$(shell mkdir -p $(obj-dirs))
> +endif
> +
I just take a quick glance: is "$(obj)" here necessary? I think all
$(targets) are under directory $(obj) when we descend into $(obj) to
recursive make, if I don't miss anything.
--
Sincerely,
Cao jin
> # Declare the contents of the .PHONY variable as phony. We keep that
> # information in a variable se we can use it in if_changed and friends.
>
> diff --git a/scripts/Makefile.host b/scripts/Makefile.host
> index 9cfd5c8..3a5460d 100644
> --- a/scripts/Makefile.host
> +++ b/scripts/Makefile.host
> @@ -48,15 +48,6 @@ host-cxxobjs := $(sort $(foreach m,$(host-cxxmulti),$($(m)-cxxobjs)))
> host-cshobjs := $(sort $(foreach m,$(host-cshlib),$($(m:.so=-objs))))
> host-cxxshobjs := $(sort $(foreach m,$(host-cxxshlib),$($(m:.so=-objs))))
>
> -# output directory for programs/.o files
> -# hostprogs-y := tools/build may have been specified.
> -# Retrieve also directory of .o files from prog-objs or prog-cxxobjs notation
> -host-objdirs := $(dir $(__hostprogs) $(host-cobjs) $(host-cxxobjs))
> -
> -host-objdirs := $(strip $(sort $(filter-out ./,$(host-objdirs))))
> -
> -
> -__hostprogs := $(addprefix $(obj)/,$(__hostprogs))
> host-csingle := $(addprefix $(obj)/,$(host-csingle))
> host-cmulti := $(addprefix $(obj)/,$(host-cmulti))
> host-cobjs := $(addprefix $(obj)/,$(host-cobjs))
> @@ -68,8 +59,6 @@ host-cshobjs := $(addprefix $(obj)/,$(host-cshobjs))
> host-cxxshobjs := $(addprefix $(obj)/,$(host-cxxshobjs))
> host-objdirs := $(addprefix $(obj)/,$(host-objdirs))
>
> -obj-dirs += $(host-objdirs)
> -
> #####
> # Handle options to gcc. Support building with separate output directory
>
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 4d88ad7..5fbc46d 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -50,15 +50,11 @@ single-used-m := $(sort $(filter-out $(multi-used-m),$(obj-m)))
> # objects depend on those (obviously)
> multi-objs-y := $(foreach m, $(multi-used-y), $($(m:.o=-objs)) $($(m:.o=-y)))
> multi-objs-m := $(foreach m, $(multi-used-m), $($(m:.o=-objs)) $($(m:.o=-y)))
> -multi-objs := $(multi-objs-y) $(multi-objs-m)
>
> # $(subdir-obj-y) is the list of objects in $(obj-y) which uses dir/ to
> # tell kbuild to descend
> subdir-obj-y := $(filter %/built-in.o, $(obj-y))
>
> -# $(obj-dirs) is a list of directories that contain object files
> -obj-dirs := $(dir $(multi-objs) $(obj-y))
> -
> # Replace multi-part objects by their individual parts, look at local dir only
> real-objs-y := $(foreach m, $(filter-out $(subdir-obj-y), $(obj-y)), $(if $(strip $($(m:.o=-objs)) $($(m:.o=-y))),$($(m:.o=-objs)) $($(m:.o=-y)),$(m))) $(extra-y)
> real-objs-m := $(foreach m, $(obj-m), $(if $(strip $($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-m))),$($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-m)),$(m)))
> @@ -81,7 +77,6 @@ multi-used-m := $(addprefix $(obj)/,$(multi-used-m))
> multi-objs-y := $(addprefix $(obj)/,$(multi-objs-y))
> multi-objs-m := $(addprefix $(obj)/,$(multi-objs-m))
> subdir-ym := $(addprefix $(obj)/,$(subdir-ym))
> -obj-dirs := $(addprefix $(obj)/,$(obj-dirs))
>
> # These flags are needed for modversions and compiling, so we define them here
> # $(modname_flags) defines KBUILD_MODNAME as the name of the module it will
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/4] kbuild: create object directories simpler and faster
2017-11-10 8:45 ` Cao jin
@ 2017-11-10 9:21 ` Masahiro Yamada
2017-11-13 8:48 ` Masahiro Yamada
0 siblings, 1 reply; 17+ messages in thread
From: Masahiro Yamada @ 2017-11-10 9:21 UTC (permalink / raw)
To: Cao jin
Cc: Linux Kbuild mailing list, Linux Kernel Mailing List,
Michal Marek, Sam Ravnborg, Douglas Anderson, Michal Marek,
X86 ML, H. Peter Anvin, Thomas Gleixner, Andy Lutomirski,
Ingo Molnar
Hi Cao,
2017-11-10 17:45 GMT+09:00 Cao jin <caoj.fnst@cn.fujitsu.com>:
>> +ifneq ($(KBUILD_SRC),)
>> +# Create directories for object files if directory does not exist
>> +obj-dirs := $(sort $(obj) $(patsubst %/,%, $(dir $(targets))))
>> +$(shell mkdir -p $(obj-dirs))
>> +endif
>> +
>
> I just take a quick glance: is "$(obj)" here necessary? I think all
> $(targets) are under directory $(obj) when we descend into $(obj) to
> recursive make, if I don't miss anything.
Good catch!
I agree that $(obj) is unnecessary.
I will remove it if I see no problem in testing.
Thanks!
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] kbuild: optimize object directory creation for incremental build
2017-11-09 15:41 ` [PATCH 4/4] kbuild: optimize object directory creation for incremental build Masahiro Yamada
@ 2017-11-10 10:58 ` Cao jin
2017-11-13 7:24 ` Masahiro Yamada
0 siblings, 1 reply; 17+ messages in thread
From: Cao jin @ 2017-11-10 10:58 UTC (permalink / raw)
To: Masahiro Yamada, linux-kbuild
Cc: linux-kernel, Michal Marek, Sam Ravnborg, Douglas Anderson, Michal Marek
Masahiro-san
On 11/09/2017 11:41 PM, Masahiro Yamada wrote:
> The previous commit largely optimized the object directory creation.
> We can optimize it more for incremental build.
>
> There are already *.cmd files in the output directory. The existing
> *.cmd files have been picked up by $(wildcard ...). Obviously,
> directories containing them exist too, so we can skip "mkdir -p".
>
> With this, Kbuild runs almost zero "mkdir -p" in incremental building.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> scripts/Makefile.build | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 89ac180..90ea7a5 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -583,8 +583,13 @@ endif
> ifneq ($(KBUILD_SRC),)
> # Create directories for object files if directory does not exist
> obj-dirs := $(sort $(obj) $(patsubst %/,%, $(dir $(targets))))
> +# If cmd_files exist, their directories apparently exist. Skip mkdir.
> +exist-dirs := $(sort $(patsubst %/,%, $(dir $(cmd_files))))
> +obj-dirs := $(strip $(filter-out . $(exist-dirs), $(obj-dirs)))
First I am not sure if the dot "." here is necessary, because I guess
kbuild always descend into subdir do recursive make, so, very
$(cmd_files) should have at least 1 level dir.
Second, Assuming that "." probably exists, would it be "./"? because it
is what "dir" function returns.
--
Sincerely,
Cao jin
> +ifneq ($(obj-dirs),)
> $(shell mkdir -p $(obj-dirs))
> endif
> +endif
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/4] kbuild: create directory for make cache only when necessary
2017-11-10 4:12 ` Masahiro Yamada
@ 2017-11-10 17:34 ` Doug Anderson
0 siblings, 0 replies; 17+ messages in thread
From: Doug Anderson @ 2017-11-10 17:34 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Linux Kbuild mailing list, LKML, Michal Marek, Sam Ravnborg
Hi,
On Thu, Nov 9, 2017 at 8:12 PM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>>> +cache-dir := $(if $(KBUILD_SRC),$(if $(cache-data),,$(dir $(make-cache))))
>>
>> It wouldn't hurt to add a comment that cache-dir will be blank if we
>> don't need to make the cache dir and will contain a directory path
>> only if the dir doesn't exist. Without a comment it could take
>> someone quite a while to realize that...
>
>
> You are right. This is confusing.
>
>
> Another idea is use a boolean flag.
>
>
> For example, like follows:
>
>
> create-cache-dir := $(if $(KBUILD_SRC),$(if $(cache-data),,1)))
>
>
> define __run-and-store
> ifeq ($(origin $(1)),undefined)
> $$(eval $(1) := $$(shell $$(2)))
> ifeq ($(create-cache-dir),1)
> $$(shell mkdir -p $(dir $(make-cache)))
> $$(eval create-cache-dir :=)
> endif
> $$(shell echo '$(1) := $$($(1))' >> $(make-cache))
> endif
> endef
>
>
>
> Perhaps, this is clearer and self-documenting.
Yes, that would be self-documenting enough for me.
>>> +
>>> # Usage: $(call __sanitize-opt,Hello=Hola$(comma)Goodbye Adios)
>>> #
>>> # Convert all '$', ')', '(', '\', '=', ' ', ',', ':' to '_'
>>> @@ -136,6 +137,10 @@ __sanitize-opt = $(subst $$,_,$(subst $(right_paren),_,$(subst $(left_paren),_,$
>>> define __run-and-store
>>> ifeq ($(origin $(1)),undefined)
>>> $$(eval $(1) := $$(shell $$(2)))
>>> +ifneq ($(cache-dir),)
>>> + $$(shell mkdir -p $(cache-dir))
>>
>> I _think_ you want some single quotes in there. AKA:
>>
>> $$(shell mkdir -p '$(cache-dir)')
>>
>> That at least matches what the "old" code used to do. Specifically if
>> 'cache-dir' happens to have a space in it then it won't work right
>> without the single quotes. There may be other symbols that your shell
>> might interpret in interesting ways, too.
>
>
> Kbuild always runs in the output directory.
>
> So, 'cache-dir' is always a relative path from the top of kernel directory
> whether O= option is given or not.
>
>
> For kernel source, I do not see any file path containing spaces.
>
> Just in case, I renamed a directory and tested, but
> something strange happened in silentoldconfig, it would not work.
>
>
> Insane people may want to use a file path with spaces
> for external modules.
>
> I tested,
>
> obj-m := fo o/
>
> but, this would not work either.
>
>
> It will be difficult to make it work
> because $(sort ...) is used in several places
> in core makefiles.
>
>
> So, my conclusion is, it does not work.
OK. This doesn't surprise me, but I'd never though through all the
cases. Thanks for checking!
Even so, if it's all the same to you I'd get a warm fuzzy if the
single quotes were there. It shouldn't hurt to have them and it seems
like it lessens the chances of problems in the future. ...but I won't
make a big stink about it and I'll leave it to your discretion.
-Doug
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/4] kbuild: create object directories simpler and faster
2017-11-09 15:41 ` [PATCH 3/4] kbuild: create object directories simpler and faster Masahiro Yamada
2017-11-10 6:26 ` Ingo Molnar
2017-11-10 8:45 ` Cao jin
@ 2017-11-10 18:46 ` Doug Anderson
2 siblings, 0 replies; 17+ messages in thread
From: Doug Anderson @ 2017-11-10 18:46 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Linux Kbuild mailing list, LKML, Michal Marek, Sam Ravnborg, x86,
H. Peter Anvin, Thomas Gleixner, Andy Lutomirski, Ingo Molnar
Hi,
On Thu, Nov 9, 2017 at 7:41 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> For the out-of-tree build, scripts/Makefile.build creates output
> directories, but this operation is not efficient.
>
> scripts/Makefile.lib calculates obj-dirs as follows:
>
> obj-dirs := $(dir $(multi-objs) $(obj-y))
>
> Please notice $(sort ...) is not used here. Usually the resulted
> obj-dirs is as many "./" as objects.
>
> For those duplicated paths, the following command is invoked.
>
> _dummy := $(foreach d,$(obj-dirs), $(shell [ -d $(d) ] || mkdir -p $(d)))
>
> Then, the costly shell command is run over and over again.
>
> I see many points for optimization:
>
> [1] Use $(sort ...) to cut down duplicated paths before passing them
> to system call
> [2] Use single $(shell ...) instead of repeating it with $(foreach ...)
> This will reduce forking.
> [3] We can calculate obj-dirs more simply. Most of objects are already
> accumulated in $(targets). So, $(dir $(targets)) is fine and more
> comprehensive.
>
> I also removed bad code in arch/x86/entry/vdso/Makefile. This is now
> really unnecessary.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> arch/x86/entry/vdso/Makefile | 4 ----
> scripts/Makefile.build | 15 ++++++---------
> scripts/Makefile.host | 11 -----------
> scripts/Makefile.lib | 5 -----
> 4 files changed, 6 insertions(+), 29 deletions(-)
I don't think I'm familiar enough with the full kbuild system to add a
reviewed-by tag, but I looked this over and it seems like a nice idea
to me.
I tested this on my own machine which does do out of tree build and it
did provide a speedup. I didn't run a ton of measurements, but it
appeared to save something like .5 seconds on a full "no-op" emerge
for me (bringing it down from ~28.5 seconds to 28 seconds). Things
also seem to be working for me with this patch, though I'm only
testing on my own workstation and not across any time of farm. In any
case:
Tested-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] kbuild: optimize object directory creation for incremental build
2017-11-10 10:58 ` Cao jin
@ 2017-11-13 7:24 ` Masahiro Yamada
0 siblings, 0 replies; 17+ messages in thread
From: Masahiro Yamada @ 2017-11-13 7:24 UTC (permalink / raw)
To: Cao jin
Cc: Linux Kbuild mailing list, Linux Kernel Mailing List,
Michal Marek, Sam Ravnborg, Douglas Anderson, Michal Marek
Hi Cao,
2017-11-10 19:58 GMT+09:00 Cao jin <caoj.fnst@cn.fujitsu.com>:
> Masahiro-san
>
> On 11/09/2017 11:41 PM, Masahiro Yamada wrote:
>> The previous commit largely optimized the object directory creation.
>> We can optimize it more for incremental build.
>>
>> There are already *.cmd files in the output directory. The existing
>> *.cmd files have been picked up by $(wildcard ...). Obviously,
>> directories containing them exist too, so we can skip "mkdir -p".
>>
>> With this, Kbuild runs almost zero "mkdir -p" in incremental building.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>> scripts/Makefile.build | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
>> index 89ac180..90ea7a5 100644
>> --- a/scripts/Makefile.build
>> +++ b/scripts/Makefile.build
>> @@ -583,8 +583,13 @@ endif
>> ifneq ($(KBUILD_SRC),)
>> # Create directories for object files if directory does not exist
>> obj-dirs := $(sort $(obj) $(patsubst %/,%, $(dir $(targets))))
>> +# If cmd_files exist, their directories apparently exist. Skip mkdir.
>> +exist-dirs := $(sort $(patsubst %/,%, $(dir $(cmd_files))))
>> +obj-dirs := $(strip $(filter-out . $(exist-dirs), $(obj-dirs)))
>
> First I am not sure if the dot "." here is necessary, because I guess
> kbuild always descend into subdir do recursive make, so, very
> $(cmd_files) should have at least 1 level dir.
The top level Makefile descends into ./Kbuild
prepare0: archprepare gcc-plugins
$(Q)$(MAKE) $(build)=.
So, it is possible to have 0 level dir.
> Second, Assuming that "." probably exists,
Right "." always exists. That's way I filtered it out.
> would it be "./"? because it
> is what "dir" function returns.
No.
You missed $(patsubst %/,%, ...)
Having said that, "." generally comes from phony targets
and I think I can fix it in a more correct way.
I will remove "." from v2.
> --
> Sincerely,
> Cao jin
>
>> +ifneq ($(obj-dirs),)
>> $(shell mkdir -p $(obj-dirs))
>> endif
>> +endif
>>
>
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/4] kbuild: create object directories simpler and faster
2017-11-10 9:21 ` Masahiro Yamada
@ 2017-11-13 8:48 ` Masahiro Yamada
0 siblings, 0 replies; 17+ messages in thread
From: Masahiro Yamada @ 2017-11-13 8:48 UTC (permalink / raw)
To: Cao jin
Cc: Linux Kbuild mailing list, Linux Kernel Mailing List,
Michal Marek, Sam Ravnborg, Douglas Anderson, Michal Marek,
X86 ML, H. Peter Anvin, Thomas Gleixner, Andy Lutomirski,
Ingo Molnar
2017-11-10 18:21 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> Hi Cao,
>
>
> 2017-11-10 17:45 GMT+09:00 Cao jin <caoj.fnst@cn.fujitsu.com>:
>
>>> +ifneq ($(KBUILD_SRC),)
>>> +# Create directories for object files if directory does not exist
>>> +obj-dirs := $(sort $(obj) $(patsubst %/,%, $(dir $(targets))))
>>> +$(shell mkdir -p $(obj-dirs))
>>> +endif
>>> +
>>
>> I just take a quick glance: is "$(obj)" here necessary? I think all
>> $(targets) are under directory $(obj) when we descend into $(obj) to
>> recursive make, if I don't miss anything.
>
> Good catch!
>
> I agree that $(obj) is unnecessary.
> I will remove it if I see no problem in testing.
>
> Thanks!
I take back this comment.
I was testing it, and in fact
Kbuild failed to create modules.order
due to missing $(obj) directory.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-11-13 8:49 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-09 15:41 [PATCH 0/4] kbuild: optimize output directory creation Masahiro Yamada
2017-11-09 15:41 ` [PATCH 1/4] kbuild: create directory for make cache only when necessary Masahiro Yamada
2017-11-09 17:59 ` Doug Anderson
2017-11-10 4:12 ` Masahiro Yamada
2017-11-10 17:34 ` Doug Anderson
2017-11-09 15:41 ` [PATCH 2/4] kbuild: remove redundant $(wildcard ...) for cmd_files calculation Masahiro Yamada
2017-11-10 4:53 ` Doug Anderson
2017-11-10 4:55 ` Masahiro Yamada
2017-11-09 15:41 ` [PATCH 3/4] kbuild: create object directories simpler and faster Masahiro Yamada
2017-11-10 6:26 ` Ingo Molnar
2017-11-10 8:45 ` Cao jin
2017-11-10 9:21 ` Masahiro Yamada
2017-11-13 8:48 ` Masahiro Yamada
2017-11-10 18:46 ` Doug Anderson
2017-11-09 15:41 ` [PATCH 4/4] kbuild: optimize object directory creation for incremental build Masahiro Yamada
2017-11-10 10:58 ` Cao jin
2017-11-13 7:24 ` Masahiro Yamada
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).