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

[Masahiro Yamada]

2017-11-10 18:21 GMT+09:00 Masahiro Yamada :
> Hi Cao,
>
>
> 2017-11-10 17:45 GMT+09:00 Cao jin :
>
>>> +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

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

[Masahiro Yamada]

2017-11-10 18:21 GMT+09:00 Masahiro Yamada :
> Hi Cao,
>
>
> 2017-11-10 17:45 GMT+09:00 Cao jin :
>
>>> +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

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

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

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 :

>> +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 3/4] kbuild: create object directories simpler and faster

[Masahiro Yamada]

Hi Cao,


2017-11-10 17:45 GMT+09:00 Cao jin :

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

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 
> ---
> 
>  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 := 

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

[Ingo Molnar]

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

Thanks,

Ingo

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

[Ingo Molnar]

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

Thanks,

Ingo