[PATCH 0/4] kbuild: optimize output directory creation

[PATCH 0/4] kbuild: optimize output directory creation

[Masahiro Yamada]

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

[PATCH 1/4] kbuild: create directory for make cache only when necessary

[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

[PATCH 2/4] kbuild: remove redundant $(wildcard ...) for cmd_files calculation

[Masahiro Yamada]

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 +--
 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))
 
 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

[PATCH 3/4] kbuild: create object directories simpler and faster

[Masahiro Yamada]

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

[PATCH 4/4] kbuild: optimize object directory creation for incremental build

[Masahiro Yamada]

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

Re: [PATCH 1/4] kbuild: create directory for make cache only when necessary

[Doug Anderson]

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

Re: [PATCH 1/4] kbuild: create directory for make cache only when necessary

[Masahiro Yamada]

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

Re: [PATCH 2/4] kbuild: remove redundant $(wildcard ...) for cmd_files calculation

[Doug Anderson]

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...

Re: [PATCH 2/4] kbuild: remove redundant $(wildcard ...) for cmd_files calculation

[Masahiro Yamada]

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

Re: [PATCH 3/4] kbuild: create object directories simpler and faster

[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

Re: [PATCH 3/4] kbuild: create object directories simpler and faster

[Cao jin]

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
> 

Re: [PATCH 3/4] kbuild: create object directories simpler and faster

[Cao jin]

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
> 

Re: [PATCH 3/4] kbuild: create object directories simpler and faster

[Masahiro Yamada]

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

Re: [PATCH 4/4] kbuild: optimize object directory creation for incremental build

[Cao jin]

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
>  

Re: [PATCH 4/4] kbuild: optimize object directory creation for incremental build

[Cao jin]

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
>  

Re: [PATCH 1/4] kbuild: create directory for make cache only when necessary

[Doug Anderson]

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

Re: [PATCH 3/4] kbuild: create object directories simpler and faster

[Doug Anderson]

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>

Re: [PATCH 4/4] kbuild: optimize object directory creation for incremental build

[Masahiro Yamada]

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

Re: [PATCH 3/4] kbuild: create object directories simpler and faster

[Masahiro Yamada]

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