diff mbox

[dpdk-dev] mk: parallelize make config

Message ID 20170122015034.19824-1-ferruh.yigit@intel.com (mailing list archive)
State Changes Requested, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel compilation success Compilation OK

Commit Message

Ferruh Yigit Jan. 22, 2017, 1:50 a.m. UTC 
  make config dependency resolving was always running serial,
parallelize it for better performance.

$ time make T=x86_64-native-linuxapp-gcc config
real    0m12.633s

$ time make -j8 T=x86_64-native-linuxapp-gcc config
real    0m1.826s

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 mk/internal/rte.depdirs-post.mk | 11 ++++++-----
 mk/rte.sdkdepdirs.mk            | 21 ++++++++++++---------
 mk/rte.subdir.mk                | 15 +++++++--------
 3 files changed, 25 insertions(+), 22 deletions(-)

Comments

Olivier Matz Jan. 23, 2017, 5:18 p.m. UTC | #1 
Hi Ferruh,

On Sun, 22 Jan 2017 01:50:34 +0000, Ferruh Yigit
<ferruh.yigit@intel.com> wrote:
> make config dependency resolving was always running serial,
> parallelize it for better performance.
> 
> $ time make T=x86_64-native-linuxapp-gcc config
> real    0m12.633s
> 
> $ time make -j8 T=x86_64-native-linuxapp-gcc config
> real    0m1.826s
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

I have a patch that fix the same issue (configuration takes to long),
but done differently. It is more intrusive, since it rework the way
DEPDIRS are used, but it does not require to use -j.

I'm sending it as a reply to this thread.

Regards,
Olivier
Wiles, Keith Jan. 23, 2017, 5:50 p.m. UTC | #2 
> On Jan 23, 2017, at 10:18 AM, Olivier Matz <olivier.matz@6wind.com> wrote:
> 
> Hi Ferruh,
> 
> On Sun, 22 Jan 2017 01:50:34 +0000, Ferruh Yigit
> <ferruh.yigit@intel.com> wrote:
>> make config dependency resolving was always running serial,
>> parallelize it for better performance.
>> 
>> $ time make T=x86_64-native-linuxapp-gcc config
>> real    0m12.633s
>> 
>> $ time make -j8 T=x86_64-native-linuxapp-gcc config
>> real    0m1.826s
>> 
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> I have a patch that fix the same issue (configuration takes to long),
> but done differently. It is more intrusive, since it rework the way
> DEPDIRS are used, but it does not require to use -j.

I tested the patch and the performance is very quick and seems to work very nicely. I have not looked at the other patch yet are they both compatible and could be combined or not?

> 
> I'm sending it as a reply to this thread.
> 
> Regards,
> Olivier

Regards,
Keith
Michał Mirosław Jan. 23, 2017, 7:03 p.m. UTC | #3 
2017-01-22 2:50 GMT+01:00 Ferruh Yigit <ferruh.yigit@intel.com>:
> make config dependency resolving was always running serial,
> parallelize it for better performance.
>
> $ time make T=x86_64-native-linuxapp-gcc config
> real    0m12.633s
>
> $ time make -j8 T=x86_64-native-linuxapp-gcc config
> real    0m1.826s
[...]
> +$(RTE_OUTPUT)/.depdirs: $(DEPDIRS)
> +       @rm -f $@
> +       @for f in $(DEPDIRS); do cat $$f >> $@; done
> +       @sort -u -o $@ $@

This could be just one 'sort -u -o $@ $(DEPDIRS)'

Best Regards,
Michał Mirosław
Olivier Matz Jan. 24, 2017, 8:42 a.m. UTC | #4 
Hi Keith,

On Mon, 23 Jan 2017 17:50:27 +0000, "Wiles, Keith"
<keith.wiles@intel.com> wrote:
> > On Jan 23, 2017, at 10:18 AM, Olivier Matz <olivier.matz@6wind.com>
> > wrote:
> > 
> > Hi Ferruh,
> > 
> > On Sun, 22 Jan 2017 01:50:34 +0000, Ferruh Yigit
> > <ferruh.yigit@intel.com> wrote:  
> >> make config dependency resolving was always running serial,
> >> parallelize it for better performance.
> >> 
> >> $ time make T=x86_64-native-linuxapp-gcc config
> >> real    0m12.633s
> >> 
> >> $ time make -j8 T=x86_64-native-linuxapp-gcc config
> >> real    0m1.826s
> >> 
> >> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>  
> > 
> > I have a patch that fix the same issue (configuration takes to
> > long), but done differently. It is more intrusive, since it rework
> > the way DEPDIRS are used, but it does not require to use -j.  
> 
> I tested the patch and the performance is very quick and seems to
> work very nicely. I have not looked at the other patch yet are they
> both compatible and could be combined or not?

No, I don't think they could be combined.

Ferruh's approach looks less risky, since it's a parallelization
of the config. My approach changes the way dependencies are processed,
and also how they are declared. I think it's faster, but more intrusive.

Regards,
Olivier
Bruce Richardson Jan. 24, 2017, 10:02 a.m. UTC | #5 
On Tue, Jan 24, 2017 at 09:42:35AM +0100, Olivier MATZ wrote:
> Hi Keith,
> 
> On Mon, 23 Jan 2017 17:50:27 +0000, "Wiles, Keith"
> <keith.wiles@intel.com> wrote:
> > > On Jan 23, 2017, at 10:18 AM, Olivier Matz <olivier.matz@6wind.com>
> > > wrote:
> > > 
> > > Hi Ferruh,
> > > 
> > > On Sun, 22 Jan 2017 01:50:34 +0000, Ferruh Yigit
> > > <ferruh.yigit@intel.com> wrote:  
> > >> make config dependency resolving was always running serial,
> > >> parallelize it for better performance.
> > >> 
> > >> $ time make T=x86_64-native-linuxapp-gcc config
> > >> real    0m12.633s
> > >> 
> > >> $ time make -j8 T=x86_64-native-linuxapp-gcc config
> > >> real    0m1.826s
> > >> 
> > >> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>  
> > > 
> > > I have a patch that fix the same issue (configuration takes to
> > > long), but done differently. It is more intrusive, since it rework
> > > the way DEPDIRS are used, but it does not require to use -j.  
> > 
> > I tested the patch and the performance is very quick and seems to
> > work very nicely. I have not looked at the other patch yet are they
> > both compatible and could be combined or not?
> 
> No, I don't think they could be combined.
> 
> Ferruh's approach looks less risky, since it's a parallelization
> of the config. My approach changes the way dependencies are processed,
> and also how they are declared. I think it's faster, but more intrusive.
>
I think we may hit a case of diminishing returns. On my system, the
config time goes from 12 seconds down to less than 1 second (large
amounts of parallelism). Even with limiting parallelism to just 4
make processes, the time for config drops to 3 seconds.
I don't see huge value in trying to shave off any more time than
that, given the amount of extra complexity involved.

/Bruce
Bruce Richardson Jan. 24, 2017, 10:52 a.m. UTC | #6 
On Sun, Jan 22, 2017 at 01:50:34AM +0000, Ferruh Yigit wrote:
> make config dependency resolving was always running serial,
> parallelize it for better performance.
> 
> $ time make T=x86_64-native-linuxapp-gcc config
> real    0m12.633s
> 
> $ time make -j8 T=x86_64-native-linuxapp-gcc config
> real    0m1.826s
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

Works great for me! Config time has always been a pain.

Tested-by: Bruce Richardson <bruce.richardson@intel.com>
Thomas Monjalon Jan. 29, 2017, 3:29 p.m. UTC | #7 
2017-01-22 01:50, Ferruh Yigit:
> make config dependency resolving was always running serial,
> parallelize it for better performance.

It could be interesting to explain why it was not parallelized,
and how you made it possible.

The test script should be updated as below:

--- a/devtools/test-build.sh
+++ b/devtools/test-build.sh
-               make T=$2 O=$1 config
+               make -j$J T=$2 O=$1 config


> --- a/mk/internal/rte.depdirs-post.mk
> +++ b/mk/internal/rte.depdirs-post.mk
> @@ -29,11 +29,12 @@
>  #   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>  #   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  
> -.PHONY: depdirs
> -depdirs:
> -	@for d in $(DEPDIRS-y); do \
> -		$(RTE_SDK)/buildtools/depdirs-rule.sh $(S) $$d ; \
> -	done
> +.PHONY: depdirs $(DEPDIRS-y)
> +depdirs: $(DEPDIRS-y)
> +	@echo ""

Why this echo "" ?

> +
> +$(DEPDIRS-y):
> +	@$(RTE_SDK)/buildtools/depdirs-rule.sh $(S) $@


> --- a/mk/rte.sdkdepdirs.mk
> +++ b/mk/rte.sdkdepdirs.mk
> @@ -36,19 +36,22 @@ ifeq (,$(wildcard $(RTE_OUTPUT)/Makefile))
>    $(error "need a make config first")
>  endif
>  
> +DEPDIRS = $(addsuffix /.depdirs, $(addprefix $(BUILDDIR)/,$(ROOTDIRS-y)))

These DEPDIRS are files, although DEPDIRS in other contexts are directories.
I think it should be renamed. DEPDIR_FILES?

>  # use a "for" in a shell to process dependencies: we don't want this
>  # task to be run in parallel.

You forgot to remove this obsolete comment.

>  .PHONY: depdirs
>  depdirs: $(RTE_OUTPUT)/.depdirs
> -$(RTE_OUTPUT)/.depdirs: $(RTE_OUTPUT)/.config
> -	@rm -f $(RTE_OUTPUT)/.depdirs ; \
> -	for d in $(ROOTDIRS-y); do \
> -		if [ -f $(RTE_SRCDIR)/$$d/Makefile ]; then \
> -			[ -d $(BUILDDIR)/$$d ] || mkdir -p $(BUILDDIR)/$$d ; \
> -			$(MAKE) S=$$d -f $(RTE_SRCDIR)/$$d/Makefile depdirs \
> -				>> $(RTE_OUTPUT)/.depdirs ; \
> -		fi ; \
> -	done
> +$(RTE_OUTPUT)/.depdirs: $(DEPDIRS)
> +	@rm -f $@
> +	@for f in $(DEPDIRS); do cat $$f >> $@; done
> +	@sort -u -o $@ $@
> +
> +$(DEPDIRS): $(RTE_OUTPUT)/.config
> +	@f=$(lastword $(subst /, ,$(dir $@))); \

Could you use $(notdir $(@D)) ?

> +	[ -d $(BUILDDIR)/$$f ] || mkdir -p $(BUILDDIR)/$$f; \
> +	rm -f $@; \

Why this removal?

> +	$(MAKE) S=$$f -f $(RTE_SRCDIR)/$$f/Makefile depdirs >> $@

This part is a bit complicated.
Could it be simplified by better naming $f?


> --- a/mk/rte.subdir.mk
> +++ b/mk/rte.subdir.mk
> @@ -76,7 +76,7 @@ clean: _postclean
>  # include .depdirs and define rules to order priorities between build
>  # of directories.
>  #
> -include $(RTE_OUTPUT)/.depdirs
> +-include $(RTE_OUTPUT)/.depdirs
>  
>  define depdirs_rule
>  $(1): $(sort $(patsubst $(S)/%,%,$(LOCAL_DEPDIRS-$(S)/$(1))))
> @@ -84,16 +84,15 @@ endef
>  
>  $(foreach d,$(DIRS-y),$(eval $(call depdirs_rule,$(d))))
>  
> +DEPDIRS = $(wildcard $(addprefix $(S)/,$(DIRS-y)))
>  
>  # use a "for" in a shell to process dependencies: we don't want this
>  # task to be run in parallel.

You forgot to remove this obsolete comment.

> -.PHONY: depdirs
> -depdirs:
> -	@for d in $(DIRS-y); do \
> -		if [ -f $(SRCDIR)/$$d/Makefile ]; then \
> -			$(MAKE) S=$S/$$d -f $(SRCDIR)/$$d/Makefile depdirs ; \
> -		fi ; \
> -	done
> +.PHONY: depdirs $(DEPDIRS)
> +depdirs: $(DEPDIRS)
> +
> +$(DEPDIRS):
> +	@$(MAKE) S=$@ -f $(RTE_SRCDIR)/$@/Makefile depdirs
Ferruh Yigit Jan. 30, 2017, 9:41 a.m. UTC | #8 
On 1/23/2017 7:03 PM, Michał Mirosław wrote:
> 2017-01-22 2:50 GMT+01:00 Ferruh Yigit <ferruh.yigit@intel.com>:
>> make config dependency resolving was always running serial,
>> parallelize it for better performance.
>>
>> $ time make T=x86_64-native-linuxapp-gcc config
>> real    0m12.633s
>>
>> $ time make -j8 T=x86_64-native-linuxapp-gcc config
>> real    0m1.826s
> [...]
>> +$(RTE_OUTPUT)/.depdirs: $(DEPDIRS)
>> +       @rm -f $@
>> +       @for f in $(DEPDIRS); do cat $$f >> $@; done
>> +       @sort -u -o $@ $@
> 
> This could be just one 'sort -u -o $@ $(DEPDIRS)'

Yes, I will update in next version, thanks.

> 
> Best Regards,
> Michał Mirosław
>
Ferruh Yigit Jan. 30, 2017, 9:46 a.m. UTC | #9 
On 1/29/2017 3:29 PM, Thomas Monjalon wrote:
> 2017-01-22 01:50, Ferruh Yigit:
>> make config dependency resolving was always running serial,
>> parallelize it for better performance.
> 
> It could be interesting to explain why it was not parallelized,
> and how you made it possible.

I will try to explain in next version.

> 
> The test script should be updated as below:
> 
> --- a/devtools/test-build.sh
> +++ b/devtools/test-build.sh
> -               make T=$2 O=$1 config
> +               make -j$J T=$2 O=$1 config

I will update.

> 
> 
>> --- a/mk/internal/rte.depdirs-post.mk
>> +++ b/mk/internal/rte.depdirs-post.mk
>> @@ -29,11 +29,12 @@
>>  #   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>>  #   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>>  
>> -.PHONY: depdirs
>> -depdirs:
>> -	@for d in $(DEPDIRS-y); do \
>> -		$(RTE_SDK)/buildtools/depdirs-rule.sh $(S) $$d ; \
>> -	done
>> +.PHONY: depdirs $(DEPDIRS-y)
>> +depdirs: $(DEPDIRS-y)
>> +	@echo ""
> 
> Why this echo "" ?

If there is no dependency (DEPDIRS-y) set and there is nothing to do
here, make prints a line like "nothing to do", since we redirect the
output to the files, that msg goes into .depdir files. To prevent that
msg, added a dummy action here.

> 
>> +
>> +$(DEPDIRS-y):
>> +	@$(RTE_SDK)/buildtools/depdirs-rule.sh $(S) $@
> 
> 
>> --- a/mk/rte.sdkdepdirs.mk
>> +++ b/mk/rte.sdkdepdirs.mk
>> @@ -36,19 +36,22 @@ ifeq (,$(wildcard $(RTE_OUTPUT)/Makefile))
>>    $(error "need a make config first")
>>  endif
>>  
>> +DEPDIRS = $(addsuffix /.depdirs, $(addprefix $(BUILDDIR)/,$(ROOTDIRS-y)))
> 
> These DEPDIRS are files, although DEPDIRS in other contexts are directories.
> I think it should be renamed. DEPDIR_FILES?

Make sense, I will update.

> 
>>  # use a "for" in a shell to process dependencies: we don't want this
>>  # task to be run in parallel.
> 
> You forgot to remove this obsolete comment.

Will update on next version.

> 
>>  .PHONY: depdirs
>>  depdirs: $(RTE_OUTPUT)/.depdirs
>> -$(RTE_OUTPUT)/.depdirs: $(RTE_OUTPUT)/.config
>> -	@rm -f $(RTE_OUTPUT)/.depdirs ; \
>> -	for d in $(ROOTDIRS-y); do \
>> -		if [ -f $(RTE_SRCDIR)/$$d/Makefile ]; then \
>> -			[ -d $(BUILDDIR)/$$d ] || mkdir -p $(BUILDDIR)/$$d ; \
>> -			$(MAKE) S=$$d -f $(RTE_SRCDIR)/$$d/Makefile depdirs \
>> -				>> $(RTE_OUTPUT)/.depdirs ; \
>> -		fi ; \
>> -	done
>> +$(RTE_OUTPUT)/.depdirs: $(DEPDIRS)
>> +	@rm -f $@
>> +	@for f in $(DEPDIRS); do cat $$f >> $@; done
>> +	@sort -u -o $@ $@
>> +
>> +$(DEPDIRS): $(RTE_OUTPUT)/.config
>> +	@f=$(lastword $(subst /, ,$(dir $@))); \
> 
> Could you use $(notdir $(@D)) ?

Can be, I will try.

> 
>> +	[ -d $(BUILDDIR)/$$f ] || mkdir -p $(BUILDDIR)/$$f; \
>> +	rm -f $@; \
> 
> Why this removal?

To be sure we are not using old files, but this can be done below line
with using ">" I guess.

> 
>> +	$(MAKE) S=$$f -f $(RTE_SRCDIR)/$$f/Makefile depdirs >> $@
> 
> This part is a bit complicated.
> Could it be simplified by better naming $f?

I will try.

> 
> 
>> --- a/mk/rte.subdir.mk
>> +++ b/mk/rte.subdir.mk
>> @@ -76,7 +76,7 @@ clean: _postclean
>>  # include .depdirs and define rules to order priorities between build
>>  # of directories.
>>  #
>> -include $(RTE_OUTPUT)/.depdirs
>> +-include $(RTE_OUTPUT)/.depdirs
>>  
>>  define depdirs_rule
>>  $(1): $(sort $(patsubst $(S)/%,%,$(LOCAL_DEPDIRS-$(S)/$(1))))
>> @@ -84,16 +84,15 @@ endef
>>  
>>  $(foreach d,$(DIRS-y),$(eval $(call depdirs_rule,$(d))))
>>  
>> +DEPDIRS = $(wildcard $(addprefix $(S)/,$(DIRS-y)))
>>  
>>  # use a "for" in a shell to process dependencies: we don't want this
>>  # task to be run in parallel.
> 
> You forgot to remove this obsolete comment.

Right, will update.

> 
>> -.PHONY: depdirs
>> -depdirs:
>> -	@for d in $(DIRS-y); do \
>> -		if [ -f $(SRCDIR)/$$d/Makefile ]; then \
>> -			$(MAKE) S=$S/$$d -f $(SRCDIR)/$$d/Makefile depdirs ; \
>> -		fi ; \
>> -	done
>> +.PHONY: depdirs $(DEPDIRS)
>> +depdirs: $(DEPDIRS)
>> +
>> +$(DEPDIRS):
>> +	@$(MAKE) S=$@ -f $(RTE_SRCDIR)/$@/Makefile depdirs
>
diff mbox

Patch

diff --git a/mk/internal/rte.depdirs-post.mk b/mk/internal/rte.depdirs-post.mk
index 102a369..eb73ad3 100644
--- a/mk/internal/rte.depdirs-post.mk
+++ b/mk/internal/rte.depdirs-post.mk
@@ -29,11 +29,12 @@ 
 #   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 #   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
-.PHONY: depdirs
-depdirs:
-	@for d in $(DEPDIRS-y); do \
-		$(RTE_SDK)/buildtools/depdirs-rule.sh $(S) $$d ; \
-	done
+.PHONY: depdirs $(DEPDIRS-y)
+depdirs: $(DEPDIRS-y)
+	@echo ""
+
+$(DEPDIRS-y):
+	@$(RTE_SDK)/buildtools/depdirs-rule.sh $(S) $@
 
 .PHONY: depgraph
 depgraph:
diff --git a/mk/rte.sdkdepdirs.mk b/mk/rte.sdkdepdirs.mk
index bebaf2a..daf42eb 100644
--- a/mk/rte.sdkdepdirs.mk
+++ b/mk/rte.sdkdepdirs.mk
@@ -36,19 +36,22 @@  ifeq (,$(wildcard $(RTE_OUTPUT)/Makefile))
   $(error "need a make config first")
 endif
 
+DEPDIRS = $(addsuffix /.depdirs, $(addprefix $(BUILDDIR)/,$(ROOTDIRS-y)))
+
 # use a "for" in a shell to process dependencies: we don't want this
 # task to be run in parallel.
 .PHONY: depdirs
 depdirs: $(RTE_OUTPUT)/.depdirs
-$(RTE_OUTPUT)/.depdirs: $(RTE_OUTPUT)/.config
-	@rm -f $(RTE_OUTPUT)/.depdirs ; \
-	for d in $(ROOTDIRS-y); do \
-		if [ -f $(RTE_SRCDIR)/$$d/Makefile ]; then \
-			[ -d $(BUILDDIR)/$$d ] || mkdir -p $(BUILDDIR)/$$d ; \
-			$(MAKE) S=$$d -f $(RTE_SRCDIR)/$$d/Makefile depdirs \
-				>> $(RTE_OUTPUT)/.depdirs ; \
-		fi ; \
-	done
+$(RTE_OUTPUT)/.depdirs: $(DEPDIRS)
+	@rm -f $@
+	@for f in $(DEPDIRS); do cat $$f >> $@; done
+	@sort -u -o $@ $@
+
+$(DEPDIRS): $(RTE_OUTPUT)/.config
+	@f=$(lastword $(subst /, ,$(dir $@))); \
+	[ -d $(BUILDDIR)/$$f ] || mkdir -p $(BUILDDIR)/$$f; \
+	rm -f $@; \
+	$(MAKE) S=$$f -f $(RTE_SRCDIR)/$$f/Makefile depdirs >> $@
 
 .PHONY: depgraph
 depgraph:
diff --git a/mk/rte.subdir.mk b/mk/rte.subdir.mk
index 256e64e..3bb3019 100644
--- a/mk/rte.subdir.mk
+++ b/mk/rte.subdir.mk
@@ -76,7 +76,7 @@  clean: _postclean
 # include .depdirs and define rules to order priorities between build
 # of directories.
 #
-include $(RTE_OUTPUT)/.depdirs
+-include $(RTE_OUTPUT)/.depdirs
 
 define depdirs_rule
 $(1): $(sort $(patsubst $(S)/%,%,$(LOCAL_DEPDIRS-$(S)/$(1))))
@@ -84,16 +84,15 @@  endef
 
 $(foreach d,$(DIRS-y),$(eval $(call depdirs_rule,$(d))))
 
+DEPDIRS = $(wildcard $(addprefix $(S)/,$(DIRS-y)))
 
 # use a "for" in a shell to process dependencies: we don't want this
 # task to be run in parallel.
-.PHONY: depdirs
-depdirs:
-	@for d in $(DIRS-y); do \
-		if [ -f $(SRCDIR)/$$d/Makefile ]; then \
-			$(MAKE) S=$S/$$d -f $(SRCDIR)/$$d/Makefile depdirs ; \
-		fi ; \
-	done
+.PHONY: depdirs $(DEPDIRS)
+depdirs: $(DEPDIRS)
+
+$(DEPDIRS):
+	@$(MAKE) S=$@ -f $(RTE_SRCDIR)/$@/Makefile depdirs
 
 .PHONY: depgraph
 depgraph: