diff mbox series

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

Message ID 1510242077-8122-2-git-send-email-yamada.masahiro@socionext.com
State Superseded
Headers show
Series kbuild: optimize output directory creation | expand

Commit Message

Masahiro Yamada Nov. 9, 2017, 3:41 p.m. UTC
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(-)

-- 
2.7.4

Comments

Doug Anderson Nov. 9, 2017, 5:59 p.m. UTC | #1
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
Masahiro Yamada Nov. 10, 2017, 4:12 a.m. UTC | #2
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
Doug Anderson Nov. 10, 2017, 5:34 p.m. UTC | #3
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
diff mbox series

Patch

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