| Message ID | 1510242077-8122-2-git-send-email-yamada.masahiro@socionext.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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
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 -- 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
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
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(-)